On Mon, May 20, 2024 at 12:13 PM Paul Eggert <egg...@cs.ucla.edu> wrote:
> Bruno and I get defect reports from Coverity Scan for Gnulib. The most
> recent one has this new complaint:
>
> > *** CID 1588680: Incorrect expression (ASSERT_SIDE_EFFECT)
> > /gltests/test-byteswap.c: 43 in test_bswap_eval_once()
> > 37
> > 38 /* Test that the bswap functions evaluate their arguments once.
> */
> > 39 static void
> > 40 test_bswap_eval_once (void)
> > 41 {
> > 42 uint_least16_t value_1 = 0;
> >>>> >>> CID 1588680: Incorrect expression (ASSERT_SIDE_EFFECT)
> >>>> >>> Argument "value_1++" of ASSERT() has a side effect. The
> containing function might work differently in a non-debug build.
> > 43 ASSERT (bswap_16 (value_1++) == 0);
>
> I checked the glibc sources, and long ago glibc indeed had a bug in this
> area: bswap_16 and related function-like macros evaluated their
> arguments more than once. This bug was fixed in the following glibc
> commit dated Sat Aug 8 20:02:34 1998 +0000, and the fix appears in glibc
> 2.1 (1999).
>
>
> https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=7ce241a03e2c0b49482d9d05c8ddb765e89a01d9
>
> Gnulib does not support glibc 2.1.x and older, so this should not be a
> problem when porting to glibc. However, I worried that other platforms
> might have the bug, until I noticed that m4/byteswap.m4 already
> inadvertently tests for it. I installed the attached patch to document
> the test.
>
> We can ignore this Coverity defect report as a false positive, just as
> we ignore many other defect reports.
I think this is a valid finding. It will operate one way in release builds
(-DNDEBUG), and another way in debug builds (no NDEBUG).
When NDEBUG is in effect, the code `bswap_16 (value_1++) == 0` will be
removed.
Maybe the test should be rewritten to something like:
uint16_t x = bswap_16 (value_1++);
ASSERT(x == 0);
Jeff
