Re: [PATCH v2 1/3] lib: zstd: Fix unused variable warning

[Nick Terrell]

> On Nov 17, 2021, at 11:50 PM, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> 
> Hi Nick,
> 
> On Wed, Nov 17, 2021 at 9:08 PM Nick Terrell <nickrterrell@xxxxxxxxx> wrote:
>> The variable `litLengthSum` is only used by an `assert()`, so when
>> asserts are disabled the compiler doesn't see any usage and warns.
>> 
>> This issue is already fixed upstream by PR #2838 [0]. It was reported
>> by the Kernel test robot in [1].
>> 
>> Another approach would be to change zstd's disabled `assert()`
>> definition to use the argument in a disabled branch, instead of
>> ignoring the argument. I've avoided this approach because there are
>> some small changes necessary to get zstd to build, and I would
>> want to thoroughly re-test for performance, since that is slightly
>> changing the code in every function in zstd. It seems like a
>> trivial change, but some functions are pretty sensitive to small
>> changes. However, I think it is a valid approach that I would
>> like to see upstream take, so I've opened Issue #2868 to attempt
>> this upstream.
>> 
>> [0] https://github.com/facebook/zstd/pull/2838
>> [1] https://lore.kernel.org/linux-mm/202111120312.833wII4i-lkp@xxxxxxxxx/T/
>> [2] https://github.com/facebook/zstd/issues/2868
>> 
>> Reported-by: kernel test robot <lkp@xxxxxxxxx>
>> Signed-off-by: Nick Terrell <terrelln@xxxxxx>
> 
> Thanks for your patch!
> 
>> --- a/lib/zstd/compress/zstd_compress_superblock.c
>> +++ b/lib/zstd/compress/zstd_compress_superblock.c
>> @@ -411,6 +411,8 @@ static size_t ZSTD_seqDecompressedSize(seqStore_t const* seqStore, const seqDef*
>>     const seqDef* sp = sstart;
>>     size_t matchLengthSum = 0;
>>     size_t litLengthSum = 0;
>> +    /* Only used by assert(), suppress unused variable warnings in production. */
>> +    (void)litLengthSum;
> 
> The Linux way-to do this is to add __maybe_unused.
> But perhaps you don't want to introduce that in the upstream codebase.

Upstream zstd can't use __maybe_unused (or its own version) because
not every compiler supports it. So compilers that don't support it may emit
unused variable warnings. We like using attributes (like fallthrough) when
applicable, and when there is a portable fallback. In this case, there isn’t
really a way to write __maybe_unused that works portably.

One of the design goals of lib/zstd/ in the kernel is to not maintain any
long-term patches on top of upstream. That is so we can automatically
import upstream into the kernel, so it is easy to keep up to date. We
can accept short-term fixes, but all patches in lib/zstd/ in the kernel
need to be ported upstream before the next import.

That does incur non-linux style in lib/zstd/. However, we mitigate that
by providing a linux-style wrapper API in <linux/zstd.h>, so that the
callers of zstd don’t get “infected” with zstd’s upstream style.

Best,
Nick Terrell

>>     while (send-sp > 0) {
>>         ZSTD_sequenceLength const seqLen = ZSTD_getSequenceLength(seqStore, sp);
>>         litLengthSum += seqLen.litLength;
> 
> Gr{oetje,eeting}s,
> 
>                        Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                -- Linus Torvalds