Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 1/2] lib: test_scanf: Fix incorrect use of type_min() with
 unsigned types
To:     Rasmus Villemoes <linux@rasmusvillemoes.dk>, <pmladek@suse.com>,
        <rostedt@goodmis.org>, <sergey.senozhatsky@gmail.com>,
        <andriy.shevchenko@linux.intel.com>, <w@1wt.eu>, <lkml@sdf.org>,
        <davem@davemloft.net>, <kuba@kernel.org>
CC:     <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <patches@opensource.cirrus.com>
References: <20210524155941.16376-1-rf@opensource.cirrus.com>
 <20210524155941.16376-2-rf@opensource.cirrus.com>
 <a3396d45-4720-ee30-6493-b19f90c74e54@rasmusvillemoes.dk>
From:   Richard Fitzgerald <rf@opensource.cirrus.com>
Message-ID: <0650840d-1b7d-3bc0-c04f-3a22ddc1ced1@opensource.cirrus.com>
Date:   Tue, 25 May 2021 11:10:10 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.0
MIME-Version: 1.0
In-Reply-To: <a3396d45-4720-ee30-6493-b19f90c74e54@rasmusvillemoes.dk>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 25/05/2021 10:55, Rasmus Villemoes wrote:
> On 24/05/2021 17.59, Richard Fitzgerald wrote:
>> sparse was producing warnings of the form:
>>
>>   sparse: cast truncates bits from constant value (ffff0001 becomes 1)
>>
>> The problem was that value_representable_in_type() compared unsigned types
>> against type_min(). But type_min() is only valid for signed types because
>> it is calculating the value -type_max() - 1.

Ok, I see I was wrong about that. It does in fact work safely. Do you
want me to update the commit message to remove this?

> 
> ... and casts that to (T), so it does produce 0 as it should. E.g. for
> T==unsigned char, we get
> 
> #define type_min(T) ((T)((T)-type_max(T)-(T)1))
> (T)((T)-255 - (T)1)
> (T)(-256)
> 

sparse warns about those truncating casts.

> which is 0 of type unsigned char.
> 
> The minimum value of an
>> unsigned is obviously 0, so only type_max() need be tested.
> 
> That part is true.
> 
> But type_min and type_max have been carefully created to produce values
> of the appropriate type that actually represent the minimum/maximum
> representable in that type, without invoking UB. If this program doesn't
> produce the expected results for you, I'd be very interested in knowing
> your compiler version:
> 

 From the kernel test robot report:

compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce:
         wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross 
-O ~/bin/make.cross
         chmod +x ~/bin/make.cross
         # apt-get install sparse
         # sparse version: v0.6.3-341-g8af24329-dirty
         # 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=50f530e176eac808e64416732e54c0686ce2c39b
         git remote add linux-next 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
         git fetch --no-tags linux-next master
         git checkout 50f530e176eac808e64416732e54c0686ce2c39b
         # save the attached .config to linux build tree
         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cro

I get the same warnings with Linaro GCC 7.5-2019.12) and sparse
v0.6.3-184-g1b896707.

> #include <stdio.h>
> 
> #define is_signed_type(type)       (((type)(-1)) < (type)1)
> #define __type_half_max(type) ((type)1 << (8*sizeof(type) - 1 -
> is_signed_type(type)))
> #define type_max(T) ((T)((__type_half_max(T) - 1) + __type_half_max(T)))
> #define type_min(T) ((T)((T)-type_max(T)-(T)1))
> 
> int main(int argc, char *argv[])
> {
> #define p(T, PT, fmt) do {					\
> 		PT vmin = type_min(T);				\
> 		PT vmax = type_max(T);				\
> 		printf("min(%s) = "fmt", max(%s) = "fmt"\n",#T, vmin, #T, vmax); \
> 	} while (0)
> 
> 	p(_Bool, int, "%d");
> 	p(unsigned char, int, "%d");
> 	p(signed char, int, "%d");
> 	p(unsigned int, unsigned int, "%u");
> 	p(unsigned long long, unsigned long long, "%llu");
> 	p(signed long long, signed long long, "%lld");
> 	
> 	return 0;
> }
> 
> 
> 
>>   lib/test_scanf.c | 13 ++++++-------
>>   1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/test_scanf.c b/lib/test_scanf.c
>> index 8d577aec6c28..48ff5747a4da 100644
>> --- a/lib/test_scanf.c
>> +++ b/lib/test_scanf.c
>> @@ -187,8 +187,8 @@ static const unsigned long long numbers[] __initconst = {
>>   #define value_representable_in_type(T, val)					 \
>>   (is_signed_type(T)								 \
>>   	? ((long long)(val) >= type_min(T)) && ((long long)(val) <= type_max(T)) \
>> -	: ((unsigned long long)(val) >= type_min(T)) &&				 \
>> -	  ((unsigned long long)(val) <= type_max(T)))
>> +	: ((unsigned long long)(val) <= type_max(T)))
> 
> 
> With or without this, these tests are tautological when T is "long long"
> or "unsigned long long". I don't know if that is intended. But it won't,
> say, exclude ~0ULL if that is in the numbers[] array from being treated
> as fitting in a "long long".

I don't entirely understand your comment. But the point of the test is
to exclude values that can't be represented by a type shorter than
long long or unsigned long long.
> 
> Rasmus
>