linux-kernel - Re: [PATCH] lib: zstd: Add cast to silence clang's -Wbitwise-instead-of-logical

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]

Date:   Fri, 5 Nov 2021 21:06:51 +0000
From:   Nick Terrell <terrelln@...com>
To:     Nathan Chancellor <nathan@...nel.org>
CC:     Nick Desaulniers <ndesaulniers@...gle.com>,
        LKML <linux-kernel@...r.kernel.org>,
        "llvm@...ts.linux.dev" <llvm@...ts.linux.dev>
Subject: Re: [PATCH] lib: zstd: Add cast to silence clang's
 -Wbitwise-instead-of-logical



> On Oct 25, 2021, at 6:17 PM, Nick Terrell <terrelln@...com> wrote:
> 
> 
> 
>> On Oct 21, 2021, at 1:23 PM, Nathan Chancellor <nathan@...nel.org> wrote:
>> 
>> A new warning in clang warns that there is an instance where boolean
>> expressions are being used with bitwise operators instead of logical
>> ones:
>> 
>> lib/zstd/decompress/huf_decompress.c:890:25: warning: use of bitwise '&' with boolean operands [-Wbitwise-instead-of-logical]
>>                       (BIT_reloadDStreamFast(&bitD1) == BIT_DStream_unfinished)
>>                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> 
>> zstd does this frequently to help with performance, as logical operators
>> have branches whereas bitwise ones do not.
>> 
>> To fix this warning in other cases, the expressions were placed on
>> separate lines with the '&=' operator; however, this particular instance
>> was moved away from that so that it could be surrounded by LIKELY, which
>> is a macro for __builtin_expect(), to help with a performance
>> regression, according to upstream zstd pull #1973.
>> 
>> Aside from switching to logical operators, which is likely undesirable
>> in this instance, or disabling the warning outright, the solution is
>> casting one of the expressions to an integer type to make it clear to
>> clang that the author knows what they are doing. Add a cast to U32 to
>> silence the warning. The first U32 cast is to silence an instance of
>> -Wshorten-64-to-32 because __builtin_expect() returns long so it cannot
>> be moved.
>> 
>> Link: https://github.com/ClangBuiltLinux/linux/issues/1486
>> Link: https://github.com/facebook/zstd/pull/1973
>> Reported-by: Nick Desaulniers <ndesaulniers@...gle.com>
>> Signed-off-by: Nathan Chancellor <nathan@...nel.org>
> 
> Thanks for the fix!
> 
> I’ll apply this patch to my linux-next tree and port it to upstream zstd.

Applied to my linux-next tree [1] and backported upstream [2].

Thanks again for the patch!

-Nick

[1] https://github.com/terrelln/linux/tree/zstd-1.4.10
[2] https://github.com/facebook/zstd/pull/2849

> Best,
> Nick Terrell
> 
>> ---
>> lib/zstd/decompress/huf_decompress.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/lib/zstd/decompress/huf_decompress.c b/lib/zstd/decompress/huf_decompress.c
>> index 05570ed5f8be..5105e59ac04a 100644
>> --- a/lib/zstd/decompress/huf_decompress.c
>> +++ b/lib/zstd/decompress/huf_decompress.c
>> @@ -886,7 +886,7 @@ HUF_decompress4X2_usingDTable_internal_body(
>>            HUF_DECODE_SYMBOLX2_0(op2, &bitD2);
>>            HUF_DECODE_SYMBOLX2_0(op3, &bitD3);
>>            HUF_DECODE_SYMBOLX2_0(op4, &bitD4);
>> -            endSignal = (U32)LIKELY(
>> +            endSignal = (U32)LIKELY((U32)
>>                        (BIT_reloadDStreamFast(&bitD1) == BIT_DStream_unfinished)
>>                      & (BIT_reloadDStreamFast(&bitD2) == BIT_DStream_unfinished)
>>                      & (BIT_reloadDStreamFast(&bitD3) == BIT_DStream_unfinished)
>> -- 
>> 2.33.1.637.gf443b226ca