Re: testing for signed char / short / int addition overflow
On 3/14/21 7:59 PM, Bruno Haible wrote: The functions my_signed1_overflow, my_signed2_overflow in the attached file produce better machine code than the corresponding functions signed1_overflow, signed2_overflow that use intprops.h primitives. That wasn't true on my platform (Ubuntu 20.10 x86-64, GCC 10.2.0-13ubuntu1 with -O2). But I expect that doesn't matter much since the signed1_overflow and signed2_overflow functions were probably not what you wanted. For example, signed1_overflow simply did this: endbr64 xorl%eax, %eax ret That is, it always returned 'false', which is a correct implementation of the macro since adding two signed char values cannot overflow on my platform. I expect you meant something more like the attached foo1.c. On my platform, its signed1_overflow seems more efficient than my_signed1_overflow as it is 6 rather than 9 insns (no branches in either case). In contrast, signed2_overflow (typically 8 insns with one typically-not-taken conditional branch) seems in the same ballparck as as my_signed2_overflow (9 insns, no branches). And signed4_overflow (4 insns, no branches) is definitely faster than my_signed4_overflow (9 insns, no branches). If I switch to clang 11.0.0-2 on the same platform (which omits a check that GCC includes), the numbers are: signed1_overflow: 3 insns my_signed1_overflow: 6 insns signed2_overflow: 3 insns my_signed2_overflow: 6 insns signed4_overflow: 3 insns my_signed4_overflow: 8 insns so here, intprops.h seems the clear winner overall. I'd say that for these platforms, intprops.h is OK as-is. Perhaps on other compilers it could be improved significantly, but I'm not sure how much we should worry about optimizing for compilers other than GCC. I installed the attached commentary patch to intprops.h to try to clear up some of the confusion you mentioned in this and your other email. If the GCC-vs-clang inefficiency is of concern perhaps someone could file a GCC bug report. From 6ab946ed2c06ecea1d7b67335bd70edbb3440a5a Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Sun, 14 Mar 2021 21:28:40 -0700 Subject: [PATCH] intprops: improve commentary * lib/intprops.h: Improve comments about promotion etc. --- ChangeLog | 5 + lib/intprops.h | 18 ++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/ChangeLog b/ChangeLog index b71c327f9..f89738061 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +2021-03-14 Paul Eggert + + intprops: improve commentary + * lib/intprops.h: Improve comments about promotion etc. + 2021-03-14 Bruno Haible time_rz: Put reference documentation into the .h file. diff --git a/lib/intprops.h b/lib/intprops.h index 967e32ea0..9d10028a5 100644 --- a/lib/intprops.h +++ b/lib/intprops.h @@ -133,7 +133,8 @@ operators might not yield numerically correct answers due to arithmetic overflow. They do not rely on undefined or implementation-defined behavior. Their implementations are simple - and straightforward, but they are a bit harder to use than the + and straightforward, but they are harder to use and may be less + efficient than the INT__WRAPV, INT__OK, and INT__OVERFLOW macros described below. Example usage: @@ -158,6 +159,9 @@ must have minimum value MIN and maximum MAX. Unsigned types should use a zero MIN of the proper type. + Because all arguments are subject to integer promotions, these + macros typically do not work on types narrower than 'int'. + These macros are tuned for constant MIN and MAX. For commutative operations such as A + B, they are also tuned for constant B. */ @@ -339,9 +343,15 @@ arguments should not have side effects. The WRAPV macros are not constant expressions. They support only - +, binary -, and *. Because the WRAPV macros convert the result, - they report overflow in different circumstances than the OVERFLOW - macros do. + +, binary -, and *. + + Because the WRAPV macros convert the result, they report overflow + in different circumstances than the OVERFLOW macros do. For + example, in the typical case with 16-bit 'short' and 32-bit 'int', + if A, B and R are all of type 'short' then INT_ADD_OVERFLOW (A, B) + returns false because the addition cannot overflow after A and B + are converted to 'int', whereas INT_ADD_WRAPV (A, B, ) returns + true or false depending on whether the sum fits into 'short'. These macros are tuned for their last input argument being a constant. -- 2.27.0
testing for signed char / short / int addition overflow
Hi Paul,
When one needs a fast test whether an addition of two 'signed char' or 'short'
overflows, the macros in intprops.h yield a valid answer, but it is not so
well optimized. The functions my_signed1_overflow, my_signed2_overflow
in the attached file produce better machine code than the corresponding
functions signed1_overflow, signed2_overflow that use intprops.h primitives.
Similarly, on 64-bit platforms, my_signed4_overflow produces slightly better
machine code (no conditional branch) than signed4_overflow. On 32-bit platforms
it depends: on SPARC my_signed4_overflow is good as well, but not on i386
(because 64-bit computations on 32-bit CPUs needs many registers, and i386
has few registers).
Would it be possible to include some of these tricks into intprops.h?
Bruno
#include "intprops.h"
int signed1_overflow (signed char a, signed char b)
{
//return INT_ADD_RANGE_OVERFLOW (a, b, (signed char) 0x80, (signed char) 0x7F);
return _GL_ADD_OVERFLOW (a, b, (signed char) 0x80, (signed char) 0x7F);
}
int my_signed1_overflow (signed char a, signed char b)
{
return (((int) (signed char) ((unsigned char) a + (unsigned char) b) - (int) a) ^ (int) b) < 0;
}
int signed2_overflow (short a, short b)
{
//return INT_ADD_RANGE_OVERFLOW (a, b, (short) 0x8000, (short) 0x7FFF);
return _GL_ADD_OVERFLOW (a, b, (short) 0x8000, (short) 0x7FFF);
}
int my_signed2_overflow (short a, short b)
{
return (((int) (short) ((unsigned short) a + (unsigned short) b) - (int) a) ^ (int) b) < 0;
}
int signed4_overflow (int a, int b)
{
//return INT_ADD_RANGE_OVERFLOW (a, b, (int) 0x8000, (int) 0x7FFF);
//return _GL_ADD_OVERFLOW (a, b, (int) 0x8000, (int) 0x7FFF);
return INT_ADD_OVERFLOW (a, b);
}
int my_signed4_overflow (int a, int b)
{
return (((long long) (int) ((unsigned int) a + (unsigned int) b) - (long long) a) ^ (long long) b) < 0;
}
#ifdef TEST
#include
int main ()
{
int u, v;
/* Verify that signed1_overflow and my_signed1_overflow agree. */
for (u = -0x8; u <= 0x7; u++)
for (v = -0x8; v <= 0x7; v++)
{
int a = u << 4;
int b = v << 4;
int x = signed1_overflow (a, b);
int y = my_signed1_overflow (a, b);
if (x != y)
printf ("signed1 mistake: a=%d b=%d x=%d y=%d\n", a, b, x, y);
}
/* Verify that signed2_overflow and my_signed2_overflow agree. */
for (u = -0x8; u <= 0x7; u++)
for (v = -0x8; v <= 0x7; v++)
{
int a = u << 12;
int b = v << 12;
int x = signed2_overflow (a, b);
int y = my_signed2_overflow (a, b);
if (x != y)
printf ("signed2 mistake: a=%d b=%d x=%d y=%d\n", a, b, x, y);
}
/* Verify that signed4_overflow and my_signed4_overflow agree. */
for (u = -0x8; u <= 0x7; u++)
for (v = -0x8; v <= 0x7; v++)
{
int a = u << 28;
int b = v << 28;
int x = signed4_overflow (a, b);
int y = my_signed4_overflow (a, b);
if (x != y)
printf ("signed4 mistake: a=%d b=%d x=%d y=%d\n", a, b, x, y);
}
return 0;
}
#endif
.file "foo.c"
.section.text.unlikely,"ax",@progbits
.LCOLDB0:
.text
.LHOTB0:
.p2align 4,,15
.globl signed1_overflow
.type signed1_overflow, @function
signed1_overflow:
.LFB0:
.cfi_startproc
testb %sil, %sil
js .L5
movsbl %sil, %esi
movl$127, %eax
movsbl %dil, %edi
subl%esi, %eax
cmpl%edi, %eax
setl%al
movzbl %al, %eax
ret
.p2align 4,,10
.p2align 3
.L5:
movsbl %sil, %esi
movl$-128, %eax
movsbl %dil, %edi
subl%esi, %eax
cmpl%eax, %edi
setl%al
movzbl %al, %eax
ret
.cfi_endproc
.LFE0:
.size signed1_overflow, .-signed1_overflow
.section.text.unlikely
.LCOLDE0:
.text
.LHOTE0:
.section.text.unlikely
.LCOLDB1:
.text
.LHOTB1:
.p2align 4,,15
.globl my_signed1_overflow
.type my_signed1_overflow, @function
my_signed1_overflow:
.LFB1:
.cfi_startproc
leal(%rsi,%rdi), %eax
movsbl %dil, %edi
movsbl %sil, %esi
movsbl %al, %eax
subl%edi, %eax
xorl%esi, %eax
shrl$31, %eax
ret
.cfi_endproc
.LFE1:
.size my_signed1_overflow, .-my_signed1_overflow
.section.text.unlikely
.LCOLDE1:
.text
.LHOTE1:
.section.text.unlikely
.LCOLDB2:
.text
.LHOTB2:
.p2align 4,,15
.globl signed2_overflow
.type signed2_overflow, @function
signed2_overflow:
.LFB2:
.cfi_startproc
testw %si, %si
js .L10
movswl %si, %esi
movl$32767, %eax
movswl %di, %edi
subl%esi, %eax
cmpl%edi, %eax
setl
remark about INT_ADD_OVERFLOW
It might be worth mentioning in the comments in intprops.h that for short a, b; INT_ADD_OVERFLOW (a, b) tests whether the C expression (a + b) overflows; it does *not* test whether the assignment short sum = a + b; overflows. Bruno
Re: tzalloc
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
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
