linux-kernel - Re: [RESEND PATCH v3 1/2] compiler.h: add _static

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <2fd6212c-6406-4435-8cde-8a07aa16d933@wanadoo.fr>
Date: Thu, 14 Nov 2024 02:44:10 +0900
From: Vincent Mailhol <mailhol.vincent@...adoo.fr>
To: Yury Norov <yury.norov@...il.com>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
 Rasmus Villemoes <linux@...musvillemoes.dk>,
 Luc Van Oostenryck <luc.vanoostenryck@...il.com>,
 linux-kernel@...r.kernel.org, linux-sparse@...r.kernel.org,
 Rikard Falkeborn <rikard.falkeborn@...il.com>
Subject: Re: [RESEND PATCH v3 1/2] compiler.h: add _static_assert()

On 13/11/2024 at 05:26, Yury Norov wrote:
> On Wed, Nov 13, 2024 at 04:08:39AM +0900, Vincent Mailhol wrote:
>> __builtin_constant_p() is known for not always being able to produce
>> constant expression [1] which lead to the introduction of
>> __is_constexpr() [2]. Because of its dependency on
>> __builtin_constant_p(), statically_true() suffers from the same
>> issues.
>>
>> For example:
>>
>>    void foo(int a)
>>    {
>>    	 /* fail on GCC */
>>    	BUILD_BUG_ON_ZERO(statically_true(a));
>>
>>    	 /* fail both clang and GCC */
>>    	static char arr[statically_true(a) ? 1 : 2];
>>    }
>>
>> For the same reasons why __is_constexpr() was created to cover
>> __builtin_constant_p() edge cases, __is_constexpr() can be used to
>> resolve statically_true() limitations.
>>
>> Note that, somehow, GCC is not always able to fold this:
>>
>>    __is_constexpr(x) && (x)
>>
>> It is OK in BUILD_BUG_ON_ZERO() but not in array declarations or in
>> static_assert():
>>
>>    void bar(int a)
>>    {
>>    	/* success */
>>    	BUILD_BUG_ON_ZERO(__is_constexpr(a) && (a));
>>
>>    	/* fail on GCC */
>>    	static char arr[__is_constexpr(a) && (a) ? 1 : 2];
>>
>>    	/* fail on GCC */
>>    	static_assert(__is_constexpr(a) && (a));
>>    }
>>
>> Encapsulating the expression in a __builtin_choose_expr() switch
>> resolves all these failed test.
>>
>> Declare a new _statically_true() macro which, by making use of the
>> __builtin_choose_expr() and __is_constexpr(x) combo, always produces a
>> constant expression.
> So, maybe name it const_true() then?


OK. I pretty like the _statically_true() because the link with 
statically_true() was obvious and the _ underscore prefix hinted that 
this variant was "special". But I have to admit that the const_true() is 
also really nice, and I finally adopted it in the v4.

>> It should be noted that statically_true() still produces better
>> folding:
>>
>>    statically_true(!(var * 8 % 8))
>>
>> always evaluates to true even if var is unknown, whereas
>>
>>    _statically_true(!(var * 8 % 8))
>>
>> fails to fold the expression and return false.
>>
>> For this reason, usage of _statically_true() be should the exception.
>> Reflect in the documentation that _statically_true() is less powerful
>> and that statically_true() is the overall preferred solution.
>>
>> [1] __builtin_constant_p cannot resolve to const when optimizing
>> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19449
>>
>> [2] commit 3c8ba0d61d04 ("kernel.h: Retain constant expression output for max()/min()")
>>
>> Signed-off-by: Vincent Mailhol <mailhol.vincent@...adoo.fr>
>> ---
>> Bonuses:
>>
>>    - above examples, and a bit more:
>>
>>        https://godbolt.org/z/zzqM1ajPj
>>
>>    - a proof that statically_true() does better constant folding than _statically_true()
>>
>>        https://godbolt.org/z/vK6KK4hMG
>> ---
>>   include/linux/compiler.h | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
>> index 4d4e23b6e3e7..c76db8b50202 100644
>> --- a/include/linux/compiler.h
>> +++ b/include/linux/compiler.h
>> @@ -308,6 +308,20 @@ static inline void *offset_to_ptr(const int *off)
>>    */
>>   #define statically_true(x) (__builtin_constant_p(x) && (x))
>>   
>> +/*
>> + * Similar to statically_true() but produces a constant expression
>> + *
>> + * To be used in conjunction with macros, such as BUILD_BUG_ON_ZERO(),
>> + * which require their input to be a constant expression and for which
>> + * statically_true() would otherwise fail.
>> + *
>> + * This is a tradeoff: _statically_true() is less efficient at
>> + * constant folding and will fail to optimize any expressions in which
>> + * at least one of the subcomponent is not constant. For the general
>> + * case, statically_true() is better.
> I agree with Rasmus. Would be nice to have examples where should I use
> one vs another right here in the comment.


I rewrote the full set of examples in v4. I added the godbolt link in 
the patch description and I cherry picked what seems to me the two most 
meaningful examples and put them in the macro comment. Let me know what 
you think.

Yours sincerely,
Vincent Mailhol