Re: tzalloc (was: Re: parse-datetime test failure on NetBSD)
On 3/14/21 11:33 AM, Bruno Haible wrote: A close term is "multithread-safe". The API could be implemented in a multithread-safe way, but time_rz.c is not multithread-safe, due to the function 'change_env'. It is planned to provide a multithread-safe implementation at some point? My plan has been to add them eventually to glibc, where they would be multithread-safe. It'd be nice to also make them multithread-safe in Gnulib, so long as that doesn't make them harder to use with existing apps, all of which currently use these functions only in a single thread (which is why this is low priority). The only platform I know that currently has them is NetBSD, where I believe they're multithread-safe. I took the word "reentrant" from the "_r" suffix, which as you note is a bit of a misnomer here (though a widely used misnomer...).
Re: tzalloc (was: Re: parse-datetime test failure on NetBSD)
Paul Eggert wrote:
> > On NetBSD, tzalloc() is in libc, and tzalloc("\\") returns NULL.
> > On other platforms, tzalloc() comes from Gnulib, and tzalloc("\\") returns
> > non-NULL.
> >
> > Which behaviour is correct?
>
> Both. The set of supported TZ values is system-dependent.
OK, then we need to
- conditionally disable the respective parts of the parse-datetime test
(done through patch 1 below),
- update the documentation of tzalloc to clarify that it may return NULL
for arguments that the implementation considers as invalid.
(done through patch 2 below).
I also took the opportunity to move the documentation of the 'time_rz'
module from the module description to the .h file. Why?
- Because the usual places where people look for reference documentation are
the manual and the .h files. They don't look in the module description.
- Because the reference documentation refers to arguments of these functions,
by name. One cannot understand the documentation if the function
declarations are far away.
Another thing: The module summary reads
Reentrant time zone functions: localtime_rz, mktime_z, etc.
What does the word "reentrant" mean here? Since the functions localtime_rz,
mktime_z don't invoke themselves recursively with a different time zone
argument, nor do they take function pointer parameters (callback), I think
"reentrant" means nothing here.
A close term is "multithread-safe". The API could be implemented in a
multithread-safe way, but time_rz.c is not multithread-safe, due to the
function 'change_env'.
It is planned to provide a multithread-safe implementation at some point?
Bruno
>From 35f8ff2e1162bf3ee60d99b6812f2ae10f3f2898 Mon Sep 17 00:00:00 2001
From: Bruno Haible
Date: Sun, 14 Mar 2021 19:19:07 +0100
Subject: [PATCH 1/2] parse-datetime tests: Avoid a test failure on NetBSD.
Reported by Thomas Klausner in
.
* tests/test-parse-datetime.c (main): Skip two tests on NetBSD.
---
ChangeLog | 7 +++
tests/test-parse-datetime.c | 4
2 files changed, 11 insertions(+)
diff --git a/ChangeLog b/ChangeLog
index a5ba848..63aaa7b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2021-03-14 Bruno Haible
+
+ parse-datetime tests: Avoid a test failure on NetBSD.
+ Reported by Thomas Klausner in
+ .
+ * tests/test-parse-datetime.c (main): Skip two tests on NetBSD.
+
2021-03-10 Paul Eggert
libc-config: port to DragonFlyBSD 5.9
diff --git a/tests/test-parse-datetime.c b/tests/test-parse-datetime.c
index acca47c..b972e96 100644
--- a/tests/test-parse-datetime.c
+++ b/tests/test-parse-datetime.c
@@ -431,8 +431,12 @@ main (int argc _GL_UNUSED, char **argv)
ASSERT ( parse_datetime (, "TZ=\"\"", ));
ASSERT ( parse_datetime (, "TZ=\"\" ", ));
ASSERT ( parse_datetime (, " TZ=\"\"", ));
+ /* Exercise patterns which may be valid or invalid, depending on the
+ platform. */
+#if !defined __NetBSD__
ASSERT ( parse_datetime (, "TZ=\"\"", ));
ASSERT ( parse_datetime (, "TZ=\"\\\"\"", ));
+#endif
/* Outlandishly-long time zone abbreviations should not cause problems. */
{
--
2.7.4
>From 02a474ef457f470776031b3cc38ee6e891dbff17 Mon Sep 17 00:00:00 2001
From: Bruno Haible
Date: Sun, 14 Mar 2021 19:22:07 +0100
Subject: [PATCH 2/2] time_rz: Put reference documentation into the .h file.
* lib/time.in.h (timezone_t, tzalloc, tzfree, localtime_rz, mktime_z):
Add comments, based on modules/time_rz.
* modules/time_rz (Comment): Remove section.
---
ChangeLog | 7 +++
lib/time.in.h | 42 --
modules/time_rz | 11 ---
3 files changed, 47 insertions(+), 13 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 63aaa7b..b71c327 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,12 @@
2021-03-14 Bruno Haible
+ time_rz: Put reference documentation into the .h file.
+ * lib/time.in.h (timezone_t, tzalloc, tzfree, localtime_rz, mktime_z):
+ Add comments, based on modules/time_rz.
+ * modules/time_rz (Comment): Remove section.
+
+2021-03-14 Bruno Haible
+
parse-datetime tests: Avoid a test failure on NetBSD.
Reported by Thomas Klausner in
.
diff --git a/lib/time.in.h b/lib/time.in.h
index 4da1172..44483d0 100644
--- a/lib/time.in.h
+++ b/lib/time.in.h
@@ -340,22 +340,60 @@ _GL_CXXALIASWARN (strftime);
# endif
# if defined _GNU_SOURCE && @GNULIB_TIME_RZ@ && ! @HAVE_TIMEZONE_T@
+/* Functions that use a first-class time zone data type, instead of
+ relying on an implicit global time zone.
+ Inspired by NetBSD. */
+
+/* Represents a time zone.
+ (timezone_t) NULL stands for UTC. */
typedef struct tm_zone *timezone_t;
+
+/* tzalloc (name)
+ Returns a time zone object for the given time zone NAME. This object
+
Re: tzalloc (was: Re: parse-datetime test failure on NetBSD)
On 3/14/21 4:53 AM, Bruno Haible wrote:
On NetBSD, tzalloc() is in libc, and tzalloc("\\") returns NULL.
On other platforms, tzalloc() comes from Gnulib, and tzalloc("\\") returns
non-NULL.
Which behaviour is correct?
Both. The set of supported TZ values is system-dependent.
tzalloc (was: Re: parse-datetime test failure on NetBSD)
> FAIL: test-parse-datetime
> =
>
> test-parse-datetime.c:434: assertion 'parse_datetime (, "TZ=\"\"",
> )' failed
> FAIL test-parse-datetime (exit status: 134)
The problem comes from the tzalloc function.
On NetBSD, tzalloc() is in libc, and tzalloc("\\") returns NULL.
On other platforms, tzalloc() comes from Gnulib, and tzalloc("\\") returns
non-NULL.
Which behaviour is correct?
I think we should encode the expected behaviour of tzalloc() in the
(not yet existent) tests module for 'time_rz'.
Bruno
Re: parse-datetime test failure on NetBSD
On Sun, Mar 14, 2021 at 11:42:43AM +0100, Bruno Haible wrote: > Hi Thomas, > > > The recipe from that bug report fails for me on NetBSD 9.99.81/amd64 > > with gcc 9.3.0: > > With version of Bison are you using? 3.7.5 Thomas
Re: parse-datetime test failure on NetBSD
Hi Thomas, > The recipe from that bug report fails for me on NetBSD 9.99.81/amd64 > with gcc 9.3.0: With version of Bison are you using? Bruno
parse-datetime test failure on NetBSD
Hi!
I reported a bug in gnutls 3.7.1
https://gitlab.com/gnutls/gnutls/-/issues/1190#note_528802421
and was told it might be a bug in gnulib instead.
The recipe from that bug report fails for me on NetBSD 9.99.81/amd64
with gcc 9.3.0:
git clone --depth=1 https://git.sv.gnu.org/git/gnulib.git
cd gnulib
./gnulib-tool --create-testdir --dir=t parse-datetime
cd t
./configure && make && make check
gives
[1] Abort trap (core dumped) "${@}" >${log_file} 2>&1
FAIL: test-parse-datetime
from test-suite.log:
FAIL: test-parse-datetime
=
test-parse-datetime.c:434: assertion 'parse_datetime (, "TZ=\"\"",
)' failed
FAIL test-parse-datetime (exit status: 134)
Cheers,
Thomas
Re: [musl] Re: parse-datetime test failure
On 11/11/20 8:07 PM, Rich Felker wrote: Thanks. I believe you've just re-discovered a known bug that's fixed in musl commit 8ebc853d37c80f0f236cc7a92cb0acc6aace, which will be included in the upcoming 1.2.2 release. Yes, thanks, that looks exactly right. It's *so* nice to have bugs fixed before I even report them! Here's a URL for the musl patch, for the record: https://git.musl-libc.org/cgit/musl/commit/?id=8ebc853d37c80f0f236cc7a92cb0acc6aace
Re: [musl] Re: parse-datetime test failure
On Wed, Nov 11, 2020 at 07:38:00PM -0800, Paul Eggert wrote: > On 11/11/20 8:20 AM, Bruno Haible wrote: > >It works fine on Alpine Linux 3.7 (32-bit, 64-bit) and 3.9 (64-bit). > > > >On Alpine Linux 3.10 and 3.12 (64-bit) it fails: > >../../gltests/test-parse-datetime.c:448: assertion 'result.tv_sec == 1 * 60 > >* 60 + 2 * 60 + 3 && result.tv_nsec == 123456789' failed > >Aborted > > > >So, to me it looks like a regression between Alpine Linux 3.9 and 3.10. > > It's arguably a bug in the test case, since Alpine uses musl libc > which does not support time zone abbreviations longer than 6 bytes, > whereas the test case uses an time zone abbreviation of 2000 bytes > (to test a bug in an old Gnulib version when running on GNU/Linux). > POSIX does not define behavior if you go over the limit. > > I worked around the problem by changing the test case to not go over > the limit as determined by sysconf (_SC_TZNAME_MAX), in the first > attached patch. Plus I refactored and/or slightly improved the > Gnulib overflow checking while I was in the neighborhood (last two > attached patches). > > Arguably this is a quality-of-implementation issue here, since > Alpine and/or musl goes beserk with long timezone abbreviations > whereas every other implementation I know of either works or > silently substitutes localtime or UTC (which is good enough for this > test case). But I'll leave that issue to the Alpine and/or musl libc > folks. > > I'll cc this to the musl bug reporting list. Although the Gnulib > test failure has been fixed, it may be the symptom of a more-severe > bug in musl. For those new to the problem, this thread starts here: > > https://lists.gnu.org/r/bug-gnulib/2020-11/msg00039.html Thanks. I believe you've just re-discovered a known bug that's fixed in musl commit 8ebc853d37c80f0f236cc7a92cb0acc6aace, which will be included in the upcoming 1.2.2 release. Rich
Re: parse-datetime test failure
On 11/11/20 8:20 AM, Bruno Haible wrote:
It works fine on Alpine Linux 3.7 (32-bit, 64-bit) and 3.9 (64-bit).
On Alpine Linux 3.10 and 3.12 (64-bit) it fails:
../../gltests/test-parse-datetime.c:448: assertion 'result.tv_sec == 1 * 60 * 60 + 2 *
60 + 3 && result.tv_nsec == 123456789' failed
Aborted
So, to me it looks like a regression between Alpine Linux 3.9 and 3.10.
It's arguably a bug in the test case, since Alpine uses musl libc which does not
support time zone abbreviations longer than 6 bytes, whereas the test case uses
an time zone abbreviation of 2000 bytes (to test a bug in an old Gnulib version
when running on GNU/Linux). POSIX does not define behavior if you go over the limit.
I worked around the problem by changing the test case to not go over the limit
as determined by sysconf (_SC_TZNAME_MAX), in the first attached patch. Plus I
refactored and/or slightly improved the Gnulib overflow checking while I was in
the neighborhood (last two attached patches).
Arguably this is a quality-of-implementation issue here, since Alpine and/or
musl goes beserk with long timezone abbreviations whereas every other
implementation I know of either works or silently substitutes localtime or UTC
(which is good enough for this test case). But I'll leave that issue to the
Alpine and/or musl libc folks.
I'll cc this to the musl bug reporting list. Although the Gnulib test failure
has been fixed, it may be the symptom of a more-severe bug in musl. For those
new to the problem, this thread starts here:
https://lists.gnu.org/r/bug-gnulib/2020-11/msg00039.html
>From 4c9a3c65e279977af4e345748ba73ab0441dc04a Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Wed, 11 Nov 2020 19:08:27 -0800
Subject: [PATCH 1/3] parse-datetime-tests: port to Alpine Linux 3.12.1
* tests/test-parse-datetime.c: Include errno.h for errno,
and unistd.h for _SC_TZNAME_MAX and sysconf.
(main): In the outlandishly-long time zone abbreviation test,
do not exceed TZNAME_MAX as this has undefined behavior,
and on Alpine Linux 3.12.1 it makes the test fail.
---
ChangeLog | 9 +
tests/test-parse-datetime.c | 16 +---
2 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index a5999557b..e1828df64 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2020-11-11 Paul Eggert
+
+ parse-datetime-tests: port to Alpine Linux 3.12.1
+ * tests/test-parse-datetime.c: Include errno.h for errno,
+ and unistd.h for _SC_TZNAME_MAX and sysconf.
+ (main): In the outlandishly-long time zone abbreviation test,
+ do not exceed TZNAME_MAX as this has undefined behavior,
+ and on Alpine Linux 3.12.1 it makes the test fail.
+
2020-11-09 Pádraig Brady
mgetgroups: avoid warning with clang
diff --git a/tests/test-parse-datetime.c b/tests/test-parse-datetime.c
index 920c9ae84..187e7c703 100644
--- a/tests/test-parse-datetime.c
+++ b/tests/test-parse-datetime.c
@@ -20,9 +20,11 @@
#include "parse-datetime.h"
+#include
#include
#include
#include
+#include
#include "macros.h"
@@ -435,13 +437,21 @@ main (int argc _GL_UNUSED, char **argv)
/* Outlandishly-long time zone abbreviations should not cause problems. */
{
static char const bufprefix[] = "TZ=\"";
-enum { tzname_len = 2000 };
+long int tzname_max = -1;
+errno = 0;
+#ifdef _SC_TZNAME_MAX
+tzname_max = sysconf (_SC_TZNAME_MAX);
+#endif
+enum { tzname_alloc = 2000 };
+if (tzname_max < 0)
+ tzname_max = errno ? 6 : tzname_alloc;
+int tzname_len = tzname_alloc < tzname_max ? tzname_alloc : tzname_max;
static char const bufsuffix[] = "0\" 1970-01-01 01:02:03.123456789";
-enum { bufsize = sizeof bufprefix - 1 + tzname_len + sizeof bufsuffix };
+enum { bufsize = sizeof bufprefix - 1 + tzname_alloc + sizeof bufsuffix };
char buf[bufsize];
memcpy (buf, bufprefix, sizeof bufprefix - 1);
memset (buf + sizeof bufprefix - 1, 'X', tzname_len);
-strcpy (buf + bufsize - sizeof bufsuffix, bufsuffix);
+strcpy (buf + sizeof bufprefix - 1 + tzname_len, bufsuffix);
ASSERT (parse_datetime (, buf, ));
LOG (buf, now, result);
ASSERT (result.tv_sec == 1 * 60 * 60 + 2 * 60 + 3
--
2.25.1
>From 00ffb79c529942eab5c81568808bd317c753213a Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Wed, 11 Nov 2020 19:16:23 -0800
Subject: [PATCH 2/3] parse-datetime: streamline overflow checking
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
When parse-datetime.y’s overflow code was written, INT_ADD_WRAPV
did not work for unsigned destinations, and since time_t might
be unsigned that meant it did not work for time_t destinations.
This limitation of INT_ADD_WRAPV has been fixed, so we can
now streamline parse-datetime.y a bit.
* lib/parse-datetime.y: Do not include limits.h, as LONG_MAX
has not been used for a while.
(yylex, parse_datetime2): Assume C99 declarations after statements.
(yyles): Use
Re: parse-datetime test failure
Hi Simon, > I noticed the test-parse-datetime fails on Alpine Linux, and > probably has failed since 2017-04-26 when the "Outlandishly-long time > zone abbreviations" test was added. (It could also be a recent > regression, of course.) It works fine on Alpine Linux 3.7 (32-bit, 64-bit) and 3.9 (64-bit). On Alpine Linux 3.10 and 3.12 (64-bit) it fails: ../../gltests/test-parse-datetime.c:448: assertion 'result.tv_sec == 1 * 60 * 60 + 2 * 60 + 3 && result.tv_nsec == 123456789' failed Aborted So, to me it looks like a regression between Alpine Linux 3.9 and 3.10. Anyone has time to report it? Bruno
