Re: [PATCH 1/2] lib: test_scanf: Fix incorrect use of type_min() with unsigned types

Richard Fitzgerald
2021-05-25T10:11:24Z

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
>