Re: unsetenv() patch for TZ

[Craig Howland <howland at LGSInnovations dot com>]

On 06/07/2013 06:23 AM, Corinna Vinschen wrote:
> Hi Craig,
>
> On Jun  6 17:33, Craig Howland wrote:
> > On 07/09/2011 08:19 AM, Corinna Vinschen wrote:
> > > Having said that, shouldn't the tzset() call better be removed in
> > > _setenv_r and called in the affected functions?
> > Corinna:
> >
> > It's been a while, but to return to this since I'm running into it
> > again and would like to get away from a local workaround:
> >
> > Good question.  But, no, I suggest that it is better to make the
> > addition to unsetenv() rather than to use your suggestion for
> > footprint reasons (in addition to my original
> > less-likely-to-cause-backwards-compatibility reason).
> >
> > If we were to remove tzset() from setenv and add it to ctime(),
> > localtime(), mktime(), and strftime(), we would be forcing the
> > environment functions to be linked when any of those 4 functions
> > were called.
> > [...]
> > Having tzset() called by unsetenv() and setenv() can equally be
> > claimed to be pulling tzset() in when perhaps it might not be
> > wanted, but this seems a more likely case than the former.  That is,
> > if you are using environment variables, it seems much more likely
> > that you'd also be using time functions at the same time than the
> > flip case.
> I disagree.  The fact that some target uses environment functions
> is no conclusive reason to believe that the application uses time
> functions *and* needs TZ.
>
> > [...]
> > In summary, I think that technically either calling tzset() from the
> > related time functions or from the environment functions could work
> > properly and be standard-compliant.  But I think that the
> > environment method better fits newlib's desire for small footprints,
> > and suggest that route.
> Let's have a look into the footprints of the object files.  I took the
> two targets I have handy all the time.  The size is the combined size
> of text, data, rdata and bss segments, in bytes (ctime_r.o and lcltime_r.o
> are irrelevant; they just call other functions):
>
> 		 i686    x86_64
>
>    setenv_r.o      844      880
>    getenv_r.o      288      256
>
>    mktime.o       2064     2224
>    mktm_r.o       1808     2000
>    strftime.o     8004     8416
>
>    tzset_r.o      1388     1600
>
> Tzset alone is bigger than setenv/getenv combined, so by adding a tzset
> call you raise the size of an executable using the environment functions
> by more than 100%.
setenv() already has the tzset() call.  Adding it to unsetenv(), also, to fix 
the latent bug will make no size difference to anyone's build other than the 
patch itself, as you would not be using unsetenv() without having used setenv().
> ...
>
> So, to me it looks much more reasonable to call tzset from the time
> functions.
> ...
>
>
> Corinna
     OK.  While I disagree that this is the best approach, it is your 
prerogative and attached is a patch for the 
ctime()/localtime()/mktime()/strftime() approach.  It does have the benefit of 
also fixing the problem identified by Yaakov in "[PATCH] strftime: use tzname if 
TM_ZONE is NULL" (https://sourceware.org/ml/newlib/2015/msg00321.html).
     Since ctime() calls localtime(), it is not directly patched (i.e. the 
patch only directly adds to 3 of the 4 functions mentioned).  The strftime.c 
patch also gets the wcsftime() function due to the shared source.
     I made a minor size/efficiency call in strftime(), and put in two tzset() 
calls, one for %z and one for %Z, so that tzset() will only be called if it is 
needed, at the expense of one more function call in the overall size.  (Which is 
what Corinna says GLIBC does.0
                Craig

Attachment: tzset.ChangeLog.txt
Description: Text document

Attachment: tzset.patch.txt
Description: Text document