linux-kernel - RE: [PATCH v2 4/8] minmax: Simplify signedness check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <07727bf98df54d789ba9e8ac66a4190c@AcuMS.aculab.com>
Date: Sun, 28 Jul 2024 18:14:38 +0000
From: David Laight <David.Laight@...LAB.COM>
To: 'Linus Torvalds' <torvalds@...uxfoundation.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Jens Axboe
	<axboe@...nel.dk>, "Matthew Wilcox (Oracle)" <willy@...radead.org>,
	"Christoph Hellwig" <hch@...radead.org>, Andrew Morton
	<akpm@...ux-foundation.org>, "Andy Shevchenko"
	<andriy.shevchenko@...ux.intel.com>, Dan Carpenter
	<dan.carpenter@...aro.org>, Arnd Bergmann <arnd@...nel.org>,
	"Jason@...c4.com" <Jason@...c4.com>, "pedro.falcato@...il.com"
	<pedro.falcato@...il.com>, Mateusz Guzik <mjguzik@...il.com>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>, Lorenzo Stoakes
	<lorenzo.stoakes@...cle.com>
Subject: RE: [PATCH v2 4/8] minmax: Simplify signedness check

From: Linus Torvalds
> Sent: 28 July 2024 17:57
> 
> On Sun, 28 Jul 2024 at 07:21, David Laight <David.Laight@...lab.com> wrote:
> >
> > +/* Allow if both x and y are valid for either signed or unsigned compares. */
> > +#define __types_ok(x, y)                               \
> > +       ((__is_ok_signed(x) && __is_ok_signed(y)) ||    \
> > +        (__is_ok_unsigned(x) && __is_ok_unsigned(y)))
> 
> This seems horrendous, exactly because it expands both x and y twice.
> And the "expand multiple times" was really the fundamental problem.

This version is better than the previous one ;-)

> Why not just change the model to say it's a bitmask of "signedness
> bits", the bits are "signed ok" and "unsigned ok", and turn it into
> 
>   /* Signedness matches? */
>   #define __types_ok(x, y) \
>      (__signedness_bits(x) & __signedness_bits(y))

Something like that might work, but it would take some effort to get right.

It would be better to remove the 'low hanging fruit' of min(pointer_type)
and the places where a constant is needed first.
Both those require extra expansions and tend to make it all that much harder.

> and __signedness_ok() simply does something like "1 if unsigned type,
> 2 if signed type, 3 if signed positive integer".
> 
> Something like (very very handwavy, very very untested):
> 
>    __builtin_choose_expr(is_signed_type(typeof(x)),
>         2+__if_constexpr(x,(x)>0,0),
>         1)

You'd want to test '(x) >= 0' and the compiler is going to bleat
(with -Wall) if (x) is an unsigned type - even though the code isn't used.
Neither __builtin_choose_expr() or _Generic() help with that.
Unless you need the types to differ ?: is just as good.

> Actually, I think that "__if_constexpr()" could very well be "if known
> positive value", ie 'x' itself doesn't have to be constant, but "x>0"
> has to be a constant (the difference being that the compiler may be
> able to tell that some variable is always positive, even if it's a
> variable):
> 
>   #define statically_true(x) __builtin_constant_p((x),(x),0)
>   #define is_positive_value(x) statically_true((x)>=0)

I think that test could be done on __x (ie the local copy).
But then you can't use static_assert() and get a sane error message.
(But don't look at what clang outputs...)

> and then use
> 
>    __builtin_choose_expr(is_signed_type(typeof(x)),
>         2+is_positive_value(x), 1)
> 
> and yes, I realize I count zero as a positive value, but writing out
> "nonnegative()" is annoying and we never care.

I got annoyed earlier :-)
> 
> I guess we could say "is_unsigned_value()"?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)