unsetenv() patch for TZ
Corinna Vinschen
vinschen@redhat.com
Mon Mar 30 19:48:00 GMT 2015
On Mar 30 21:30, Corinna Vinschen wrote:
> On Mar 30 11:38, Craig Howland wrote:
> > 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?
> > >>>
> > [...]
> > >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().
>
> That was the sole reason of discussing it. I proposed to remove it from
> setenv/unsetenv for the very reason that every usage of setenv/unsetenv
> pulls in tzset, even for applications not using time functions at all.
> Therefore calling tzset from setenv/unsetenv appears to me as a space
> consuming shortcut. I'm not concerned for Cygwin, obviously, but small
> targets might profit from removing these calls.
>
> > 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
>
> Thanks. The setenv change is ok, but I have a bit of a problem with the
> implemention in the time functions. Apart from the strftime 'z' case,
> all calls to tzset are outside the TZ_LOCK/TZ_UNLOCK bracket, but tzset
> calls TZ_LOCK/TZ_UNLOCK, too. As far as I can see it would be better to
> implement _tzset_unlocked/_tzset_unlocked_r and call these from inside
> the TZ_LOCK/TZ_UNLOCK bracket, wouldn't it?
Kind of like the below patch.
> Btw., was there some (historical?) reason that _setenv_r called tzset,
> and not _tzset_r?
Corinna
diff --git a/newlib/libc/time/local.h b/newlib/libc/time/local.h
index 5fc3ddc..628420b 100644
--- a/newlib/libc/time/local.h
+++ b/newlib/libc/time/local.h
@@ -23,6 +23,8 @@ int _EXFUN (__tzcalc_limits, (int __year));
extern _CONST int __month_lengths[2][MONSPERYEAR];
+_VOID _EXFUN(_tzset_unlocked_r, (struct _reent *));
+
/* locks for multi-threading */
#ifdef __SINGLE_THREAD__
#define TZ_LOCK
diff --git a/newlib/libc/time/tzset.c b/newlib/libc/time/tzset.c
index d847a26..3525a33 100644
--- a/newlib/libc/time/tzset.c
+++ b/newlib/libc/time/tzset.c
@@ -70,5 +70,7 @@ Supporting OS subroutine required: None
_VOID
_DEFUN_VOID (tzset)
{
- _tzset_r (_REENT);
+ TZ_LOCK;
+ _tzset_unlocked_r (_REENT);
+ TZ_UNLOCK;
}
diff --git a/newlib/libc/time/tzset_r.c b/newlib/libc/time/tzset_r.c
index 2c5b723..7f663b2 100644
--- a/newlib/libc/time/tzset_r.c
+++ b/newlib/libc/time/tzset_r.c
@@ -14,7 +14,7 @@ static char __tzname_dst[11];
static char *prev_tzenv = NULL;
_VOID
-_DEFUN (_tzset_r, (reent_ptr),
+_DEFUN (_tzset_unlocked_r, (reent_ptr),
struct _reent *reent_ptr)
{
char *tzenv;
@@ -25,22 +25,17 @@ _DEFUN (_tzset_r, (reent_ptr),
if ((tzenv = _getenv_r (reent_ptr, "TZ")) == NULL)
{
- TZ_LOCK;
_timezone = 0;
_daylight = 0;
_tzname[0] = "GMT";
_tzname[1] = "GMT";
free(prev_tzenv);
prev_tzenv = NULL;
- TZ_UNLOCK;
return;
}
- TZ_LOCK;
-
if (prev_tzenv != NULL && strcmp(tzenv, prev_tzenv) == 0)
{
- TZ_UNLOCK;
return;
}
@@ -54,10 +49,7 @@ _DEFUN (_tzset_r, (reent_ptr),
++tzenv;
if (sscanf (tzenv, "%10[^0-9,+-]%n", __tzname_std, &n) <= 0)
- {
- TZ_UNLOCK;
- return;
- }
+ return;
tzenv += n;
@@ -74,10 +66,7 @@ _DEFUN (_tzset_r, (reent_ptr),
ss = 0;
if (sscanf (tzenv, "%hu%n:%hu%n:%hu%n", &hh, &n, &mm, &n, &ss, &n) < 1)
- {
- TZ_UNLOCK;
- return;
- }
+ return;
tz->__tzrule[0].offset = sign * (ss + SECSPERMIN * mm + SECSPERHOUR * hh);
_tzname[0] = __tzname_std;
@@ -88,7 +77,6 @@ _DEFUN (_tzset_r, (reent_ptr),
_tzname[1] = _tzname[0];
_timezone = tz->__tzrule[0].offset;
_daylight = 0;
- TZ_UNLOCK;
return;
}
else
@@ -127,10 +115,7 @@ _DEFUN (_tzset_r, (reent_ptr),
{
if (sscanf (tzenv, "M%hu%n.%hu%n.%hu%n", &m, &n, &w, &n, &d, &n) != 3 ||
m < 1 || m > 12 || w < 1 || w > 5 || d > 6)
- {
- TZ_UNLOCK;
- return;
- }
+ return;
tz->__tzrule[i].ch = 'M';
tz->__tzrule[i].m = m;
@@ -198,6 +183,13 @@ _DEFUN (_tzset_r, (reent_ptr),
__tzcalc_limits (tz->__tzyear);
_timezone = tz->__tzrule[0].offset;
_daylight = tz->__tzrule[0].offset != tz->__tzrule[1].offset;
+}
+_VOID
+_DEFUN (_tzset_r, (reent_ptr),
+ struct _reent *reent_ptr)
+{
+ TZ_LOCK;
+ _tzset_unlocked_r (reent_ptr);
TZ_UNLOCK;
}
--
Corinna Vinschen
Cygwin Maintainer
Red Hat
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/newlib/attachments/20150330/7792557c/attachment.sig>
More information about the Newlib
mailing list