Disabled assert() could use its argument to avoid unused variable warnings #2868

terrelln · 2021-11-17T19:52:14Z

Test this construction for disabling assertions:

#define assert(condition)      \
    do {                       \
        if (0) {               \
            (void)(condition); \
        }                      \
    } while (0)

Additionally, do the same for RAWLOG() and DEBUGLOG() when DEBUGLEVEL < 2.

This would avoid unused variable warnings that show up in production.

The text was updated successfully, but these errors were encountered:

Cyan4973 · 2021-11-17T20:07:14Z

Note : this construction relies on the compiler doing a good DCE job.
While this is most likely the case for most modern and mainstream compilers using optimization settings,
it's more difficult to be sure about that with regards to the larger C compiler ecosystem.

An example of uncommon compiler (yet not completely fringe) which may have troubles with DCE could be tcc.

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] facebook/zstd#2838 [1] https://lore.kernel.org/linux-mm/[email protected]/T/ [2] facebook/zstd#2868 Reported-by: kernel test robot <[email protected]> Signed-off-by: Nick Terrell <[email protected]>

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. Lastly, I've chosen not to use __maybe_unused because all code in lib/zstd/ must eventually be upstreamed. Upstream zstd can't use __maybe_unused because it isn't portable across all compilers. [0] facebook/zstd#2838 [1] https://lore.kernel.org/linux-mm/[email protected]/T/ [2] facebook/zstd#2868 Link: https://lore.kernel.org/r/[email protected]/ Link: https://lore.kernel.org/r/[email protected]/ Reported-by: kernel test robot <[email protected]> Signed-off-by: Nick Terrell <[email protected]>

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 raspberrypi#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 raspberrypi#2868 to attempt this upstream. Lastly, I've chosen not to use __maybe_unused because all code in lib/zstd/ must eventually be upstreamed. Upstream zstd can't use __maybe_unused because it isn't portable across all compilers. [0] facebook/zstd#2838 [1] https://lore.kernel.org/linux-mm/[email protected]/T/ [2] facebook/zstd#2868 Link: https://lore.kernel.org/r/[email protected]/ Link: https://lore.kernel.org/r/[email protected]/ Reported-by: kernel test robot <[email protected]> Signed-off-by: Nick Terrell <[email protected]>