Date: Sun, 06 Nov 2016 11:00:02 -0500 From: Tom Lane <tgl@sss.pgh.pa.us> Message-ID: <22781.1478448002@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!!! kre