parse-datetime test failure on NetBSD

parse-datetime test failure on NetBSD

[Thomas Klausner]

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 (&result, "TZ=\"\\\\\"", &now)' failed
FAIL test-parse-datetime (exit status: 134)

Cheers,
 Thomas

Re: parse-datetime test failure on NetBSD

[Bruno Haible]

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

Re: parse-datetime test failure on NetBSD

[Thomas Klausner]

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

tzalloc (was: Re: parse-datetime test failure on NetBSD)

[Bruno Haible]

> FAIL: test-parse-datetime
> =========================
> 
> test-parse-datetime.c:434: assertion 'parse_datetime (&result, "TZ=\"\\\\\"", &now)' 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: tzalloc (was: Re: parse-datetime test failure on NetBSD)

[Paul Eggert]

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.

Re: tzalloc (was: Re: parse-datetime test failure on NetBSD)

[Bruno Haible]

[-- Attachment #1: Type: text/plain, Size: 1655 bytes --]

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

[-- Attachment #2: 0001-parse-datetime-tests-Avoid-a-test-failure-on-NetBSD.patch --]
[-- Type: text/x-patch, Size: 1772 bytes --]

From 35f8ff2e1162bf3ee60d99b6812f2ae10f3f2898 Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
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 <tk@giga.or.at> in
<https://lists.gnu.org/archive/html/bug-gnulib/2021-03/msg00069.html>.

* 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  <bruno@clisp.org>
+
+	parse-datetime tests: Avoid a test failure on NetBSD.
+	Reported by Thomas Klausner <tk@giga.or.at> in
+	<https://lists.gnu.org/archive/html/bug-gnulib/2021-03/msg00069.html>.
+	* tests/test-parse-datetime.c (main): Skip two tests on NetBSD.
+
 2021-03-10  Paul Eggert  <eggert@cs.ucla.edu>
 
 	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 (&result, "TZ=\"\"", &now));
   ASSERT (   parse_datetime (&result, "TZ=\"\" ", &now));
   ASSERT (   parse_datetime (&result, " TZ=\"\"", &now));
+  /* Exercise patterns which may be valid or invalid, depending on the
+     platform.  */
+#if !defined __NetBSD__
   ASSERT (   parse_datetime (&result, "TZ=\"\\\\\"", &now));
   ASSERT (   parse_datetime (&result, "TZ=\"\\\"\"", &now));
+#endif
 
   /* Outlandishly-long time zone abbreviations should not cause problems.  */
   {
-- 
2.7.4


[-- Attachment #3: 0002-time_rz-Put-reference-documentation-into-the-.h-file.patch --]
[-- Type: text/x-patch, Size: 4972 bytes --]

From 02a474ef457f470776031b3cc38ee6e891dbff17 Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
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  <bruno@clisp.org>
 
+	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  <bruno@clisp.org>
+
 	parse-datetime tests: Avoid a test failure on NetBSD.
 	Reported by Thomas Klausner <tk@giga.or.at> in
 	<https://lists.gnu.org/archive/html/bug-gnulib/2021-03/msg00069.html>.
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
+   represents the time zone that other functions would use it the TZ
+   environment variable was set to NAME.
+   If NAME is NULL, the result represents the time zone that other functions
+   would use it the TZ environment variable was unset.
+   May return NULL if NAME is invalid (this is platform dependent) or
+   upon memory allocation failure.  */
 _GL_FUNCDECL_SYS (tzalloc, timezone_t, (char const *__name));
 _GL_CXXALIAS_SYS (tzalloc, timezone_t, (char const *__name));
+
+/* tzfree (tz)
+   Frees a time zone object.
+   The argument must have been returned by tzalloc().  */
 _GL_FUNCDECL_SYS (tzfree, void, (timezone_t __tz));
 _GL_CXXALIAS_SYS (tzfree, void, (timezone_t __tz));
+
+/* localtime_rz (tz, &t, &result)
+   Converts an absolute time T to a broken-down time RESULT, assuming the
+   time zone TZ.
+   This function is like 'localtime_r', but relies on the argument TZ instead
+   of an implicit global time zone.  */
 _GL_FUNCDECL_SYS (localtime_rz, struct tm *,
                   (timezone_t __tz, time_t const *restrict __timer,
                    struct tm *restrict __result) _GL_ARG_NONNULL ((2, 3)));
 _GL_CXXALIAS_SYS (localtime_rz, struct tm *,
                   (timezone_t __tz, time_t const *restrict __timer,
                    struct tm *restrict __result));
+
+/* mktime_z (tz, &tm)
+   Normalizes the broken-down time TM and converts it to an absolute time,
+   assuming the time zone TZ.  Returns the absolute time.
+   This function is like 'mktime', but relies on the argument TZ instead
+   of an implicit global time zone.  */
 _GL_FUNCDECL_SYS (mktime_z, time_t,
-                  (timezone_t __tz, struct tm *restrict __result)
+                  (timezone_t __tz, struct tm *restrict __tm)
                   _GL_ARG_NONNULL ((2)));
 _GL_CXXALIAS_SYS (mktime_z, time_t,
-                  (timezone_t __tz, struct tm *restrict __result));
+                  (timezone_t __tz, struct tm *restrict __tm));
+
+/* Time zone abbreviation strings (returned by 'localtime_rz' or 'mktime_z'
+   in the 'tm_zone' member of 'struct tm') are valid as long as
+     - the 'struct tm' argument is not destroyed or overwritten,
+   and
+     - the 'timezone_t' argument is not freed through tzfree().  */
+
 # endif
 
 /* Convert TM to a time_t value, assuming UTC.  */
diff --git a/modules/time_rz b/modules/time_rz
index 6a2b23c..ebcef84 100644
--- a/modules/time_rz
+++ b/modules/time_rz
@@ -1,17 +1,6 @@
 Description:
 Reentrant time zone functions: localtime_rz, mktime_z, etc.
 
-Comment:
-This implements the NetBSD-inspired extensions to <time.h>, which
-defines a type timezone_t and associated allocation functions tzalloc
-and tzfree, along with two functions localtime_rz and mktime_z that
-are like localtime_r and mktime except they have a new leading
-timezone_t argument.  Time zone abbreviation strings have lifetimes
-equal to the corresponding struct tm or timezone_t object (whichever
-is less).  tzalloc (X) yields a time zone object equivalent to setting
-the TZ environment variable to X.  tzalloc (NULL) is the same as an
-unset TZ environment variable.  (timezone_t) 0 stands for UTC.
-
 Files:
 lib/time-internal.h
 lib/time_rz.c
-- 
2.7.4

Re: tzalloc (was: Re: parse-datetime test failure on NetBSD)

[Paul Eggert]

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

[Bruno Haible]

Paul Eggert wrote:
> My plan has been to add them eventually to glibc, where they would be 
> multithread-safe.

That would be nice, indeed!

Use cases that I can see:
  - A server process that talks to several users in different locales and
    time zones.
    (We have locale_t for several years already, but not yet timezone_t.)
  - A multithreaded mail agent that wants to sort mails by date, where
    each mail lists its own date in a format that includes the time zone.

Bruno