On Tue, Oct 7, 2014, at 16:48, Arthur David Olson wrote:

Back in the 1980s (when this code had its origins), NULL pointers weren't guaranteed to be all zeroes; having a static, otherwise unused structure (which was guaranteed to be initialized correctly) for initialization purposes was cheap portability insurance. (The insurance was taken out consistently, both for uniformity and to avoid problems if a pointer was added to a structure later.) Given updates to the C standard and waning interest in supporting old systems, this insurance may well no longer be needed.

Just to be clear, the updates to the C standard regarding this issue consist of allowing one to initialize a non-static structure with the literal "{0}", rather than anything about being able to memset pointers or floats.

Thanks for reviewing the code; this found a real bug and prompted me to find another. Plus, you found some other things worth sprucing up. To my mind the most important fix in your patch is removing that stray initialization in leapcorr; this fixes a bug in the implementation of the new (2014g) functions time2posix_z and posix2time_z. Christos Zoulas wrote:

There is a bug fix there too; in one of the cases sp->ttis[1] is not initialized, and returns trash to the user.

Sorry, I don't see the bug there; can you explain? I assume you're talking about either the code labeled "only standard time" or the code labeled "so, we're off a little", and in both cases there is only one time type, so the rest of the code should never access sp->ttis[1] and there should be no need to initialize it. Although C99-and-later guarantees that memset(..., 0, ...) sets the struct ttinfo members (which are int, bool, and int_fast32_t) to zero, tzcode assumes only C89 which as I understand it does not provide that guarantee, so to be safe we should avoid memset here. It's easy enough to initialize the remaining structure members by hand. The change to use ssize_t won't fix bugs on modern POSIX platforms, since POSIX nowadays requires int to be at least 32 bits, but the change is probably worth doing for clarity anway, and also to port to ancient POSIX which allowed 16-bit int. However, POSIX weirdly says that ssize_t might not be able to store values less than -1, which means subtracting a value from ssize_t is a no-no if the result might be less than -1, which means we need to redo a subtraction that might compute such a result. Furthermore, the loop from 0 to nread would need an index type other than 'int', but there it's clearer just to use memmove (something that C89 does guarantee). In trying out the code under 'valgrind' I found that the command "zdump ''" tickled some other uninitialized-storage usages: the "so, we're off a little" code doesn't initialize charcnt, goback, goahead, or defaulttype. I hope the attached proposed patch fixes all these problems; please let us know if it misses anything.

On 10/08/2014 12:00 AM, Clive D.W. Feather wrote:

the view of WG14 was always that the representations of types stuff added in C99 was intended to have always applied to C90

Thanks, that's good to know if we run into this issue again in the future.

I can see that POSIX says "The type ssize_t shall be capable of storing values at least in the range [-1, {SSIZE_MAX}].", but that doesn't exempt it from the C rules concerning types.

Well, in *theory* ssize_t could be an extended integer type using one's complement and SSIZE_MAX could be 1, no? As far as I can tell nothing in the C standard prohibits that, and that would mean one couldn't necessarily store -2 into an ssize_t variable. Of course this is purely a theoretical objection, but I've always been suspicious of that POSIX rule about -1 and ssize_t -- the rule must be there for a reason, though they don't explain the reason -- and so prefer to avoid ssize_t except to hold the result of 'read'-like syscalls that are defined to return ssize_t.

On Oct 7, 10:11pm, eggert@cs.ucla.edu (Paul Eggert) wrote: -- Subject: Re: [tz] localtime.c patch | Sorry, I don't see the bug there; can you explain? I assume you're talking | about either the code labeled "only standard time" or the code labeled "so, | we're off a little", and in both cases there is only one time type, so the rest | of the code should never access sp->ttis[1] and there should be no need to | initialize it. Well, this was the case before we returned a pointer of that data to the user. I prefer not to return random bits from the heap to the user. | In trying out the code under 'valgrind' I found that the command "zdump ''" | tickled some other uninitialized-storage usages: the "so, we're off a little" | code doesn't initialize charcnt, goback, goahead, or defaulttype. Thanks, that is a good idea. | I hope the attached proposed patch fixes all these problems; please let us know I'll take a look, thanks! christos

On Oct 8, 11:24am, eggert@cs.ucla.edu (Paul Eggert) wrote: -- Subject: Re: [tz] localtime.c patch | Thanks, the attached proposed patch should address those issues. I've | left the issue of documenting return and errno values for another day. Thanks, this looks good. christos

Arthur David Olson said:

Back in the 1980s (when this code had its origins), NULL pointers weren't guaranteed to be all zeroes; having a static, otherwise unused structure (which was guaranteed to be initialized correctly) for initialization purposes was cheap portability insurance. (The insurance was taken out consistently, both for uniformity and to avoid problems if a pointer was added to a structure later.) Given updates to the C standard and waning interest in supporting old systems, this insurance may well no longer be needed.

Disagree. The standard still doesn't say that you can memset to 0 to get a NULL pointer. Embedded systems in particular may well have odd setups that mean that a NULL pointer isn't a zero bit pattern (I currently develop on a system where a char is 16 bits and there's something important at address zero). -- 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