[tz] Compiler warning in tzcode2016g about SIZE_MAX being wider than int
kre at munnari.OZ.AU
Sun Nov 6 18:32:06 UTC 2016
Date: Sun, 06 Nov 2016 11:00:02 -0500
From: Tom Lane <tgl at sss.pgh.pa.us>
Message-ID: <22781.1478448002 at sss.pgh.pa.us>
| I installed this update into Postgres, and now our weekly run of Coverity
| is unhappy:
After tzcode2016i was added to NetBSD this broke the NetBSD builds, because
NetBSD (for whatever reason) uses almost every gcc (or clang) warning that
exists (just a couple that are almost always bogus are excluded) and
also has -Werror - so any warnings are fatal they all need to be cleaned up.
| *** CID 1373152: Control flow issues (DEADCODE)
That's really to be expected, the code is attempting to make a run time
decision about which of ptrdiff_t and size_t has more bits - for any
particular system, the answer will be fixed, and hence the run time testing
must have dead code (we just don't know before compiling which branch it
will be.) Best would be just to disable that warning for this line.
| 451 ptrdiff_t nitems_max = PTRDIFF_MAX - WORK_AROUND_QTBUG_53071;
| 452 ptrdiff_t amax = nitems_max < SIZE_MAX ? nitems_max : SIZE_MAX;
The problem NetBSD experienced (initially) is because of the comparison
itself, nitems_max is signed, and SIZE_MAX is unsigned, and gcc warns
about comparing signed values to unsigned (as the comparison promotes the
signed value to unsigned first, which makes small negative values seem big,
and often uncovers code bugs). Here however, even though nitems_max is
a signed type, it has a known positive value, so the bug really can't happen,
but gcc (at least) is just too stupid to work that out.
Currently that's "fixed" (in NetBSD) by casting nitems_max to a size_t
explicitly for the comparison, but that actually breaks the algorithm
(it is fine for NetBSD as size_t and ptrdiff_t contain the same number
of bits, but if a ptrdiff_t was a wider type than size_t, which some
arguments would suggest it should be, then the cast reduces the magnitide
of nitems_max and would likely (well, possibly) lead to incorrect results.
| This is sort of the same thing as the original clang complaint, although
| expressed differently: the test against SIZE_MAX is still useless because
| it doesn't fit in ptrdiff_t any more than it did in int.
That depends upon the number of bits in a ptrdiff_t - consider a 64 bit
ptrdiff_t with a 32 bit size_t for example.
The code is actually very clever, and if it weren't for all these (not
nearly so clever) analysers attempting to second guess what is happening
and getting it wrong, it would be fine as it is.
I had been wondering about the need for nitems_max as a variable though
and wondered if perhaps
#define NITEMS_MAX (PTRDIFF_MAX - WORK_AROUND_QTBUG_53071)
ptrdiff_t amax = NITEMS_MAX < SIZE_MAX ? NITEMS_MAX : SIZE_MAX;
might not work just as well, and avoid (at least) the gcc warning
(though I am yet to actually test this hypothesis). It will probably
fall foul of the "constant comparison" warning though. Ugh!!!
More information about the tz