On 10/20/2016 04:05 PM, Paul Eggert wrote:

I'll see if I can rewrite the code to remove the INT_MAX restriction, which is arbitrary here.

Done, by installing the attached patch into the development repository. I don't know whether this will pacify Clang, but at least it fixes some (mostly theoretical) bugs in zic. By the way, people may have some trouble accessing GitHub now, due to the ongoing denial-of-service attack on Dyn. I had to bypass DNS to access GitHub myself. See: Leyden J. Dyn dinged by DDoS: US DNS firm gives web a bad hair day. The Register 2016-10-21. http://www.theregister.co.uk/2016/10/21/dns_dyn_ddos/

Tom Lane said:

I suppose that clang's check must be calibrated to allow stuffing an unsigned value into a signed value of the same width, just not narrower widths.

Three things to note about this sort of stuff in C: (1) The range of values of an unsigned type has to include all the non-negative values of the corresponding signed type, but doesn't have to be any bigger. In other words, INT_MAX == UINT_MAX is allowed. (2) Just because two types with the same signedness have the same size as given by sizeof, they don't have to have the same range of values. So even if sizeof(int) == sizeof(long), INT_MAX != LONG_MAX is still allowed. (3) Just because two types have the same size, signedness, and range of values, doesn't mean they have the same representation. For example, int could be big-endian and long little-endian. -- Clive D.W. Feather | If you lie to the compiler, Email: clive@davros.org | it will get its revenge. Web: http://www.davros.org | - Henry Spencer Mobile: +44 7973 377646

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

Tom Lane wrote:

I'd change growalloc() so that its nitems-related arguments are defined as size_t not ptrdiff_t, and forget about this idea that ptrdiff_t has anything to do with the limit of what can be requested from malloc.

ptrdiff_t is relevant since zic subtracts pointers, and subtraction has undefined behavior if the result doesn't fit into ptrdiff_t. All other things being equal I prefer signed integer arithmetic to unsigned, because on some platforms overflow checking works with signed arithmetic and this can help find programming errors. This is why growalloc uses ptrdiff_t rather than size_t even though either would do.

What about doing the comparison in an #if?

#if is best avoided when possible (it can't always be). Robert Elz wrote:

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.

Yes, attempting to pacify all these analyzers can contort the code, which I'd rather avoid. I don't even like the INITIALIZE macro, and considered adding -Wno-maybe-uninitialized to GCC_DEBUG_FLAGS so that INITIALIZE can be removed. I kept INITIALIZE only because I use GCC and -Wmaybe-uninitialized is so useful in finding real bugs elsewhere. As far as Coverity etc. go, I would rather that people filed bug reports to get these other static analyzers fixed, as I try to do with GCC. I just now checked tzcode with Clang and found one recently-added bit of unnecessary code, which I fixed with the attached.