FYI: s390x: Require GCC 7.1 or later to build glibc due to __builtin_add_overflow

FYI: s390x: Require GCC 7.1 or later to build glibc due to __builtin_add_overflow

[Stefan Liebler]

Hi gnulib-developers,

just as information, I've committed the glibc patch

"s390x: Require GCC 7.1 or later to build glibc."
http://sourceware.org/git/?p=glibc.git;a=commit;h=844b4d8b4b937fe6943d2c0c80ce7d871cdb1eb5

as if build with gcc 6.5.0, __builtin_add_overflow incorrectly detects
overflow on s390x.

Please find the link to gcc-Bug on the discussion on libc-alpha:

On 12/17/20 6:35 PM, Adhemerval Zanella wrote:
> On 15/12/2020 11:18, Stefan Liebler via Libc-alpha wrote:
>> GCC 6.5 fails to correctly build ldconfig with recent ld.so.cache
>> commits, e.g.:
>> 785969a047ad2f23f758901c6816422573544453
>> elf: Implement a string table for ldconfig, with tail merging
>>
>> If glibc is build with gcc 6.5.0:
>> __builtin_add_overflow is used in
>> <glibc>/elf/stringtable.c:stringtable_finalize()
>> which leads to ldconfig failing with "String table is too large".
>> This is also recognizable in following tests:
>> FAIL: elf/tst-glibc-hwcaps-cache
>> FAIL: elf/tst-glibc-hwcaps-prepend-cache
>> FAIL: elf/tst-ldconfig-X
>> FAIL: elf/tst-ldconfig-bad-aux-cache
>> FAIL: elf/tst-ldconfig-ld_so_conf-update
>> FAIL: elf/tst-stringtable
>>
>> See gcc "Bug 98269 - gcc 6.5.0 __builtin_add_overflow() with small
>> uint32_t values incorrectly detects overflow"
>> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98269)
> 
> In any case, it would be good to alert gnulib developers about this
> potential issue.

Bye,
Stefan

Re: FYI: s390x: Require GCC 7.1 or later to build glibc due to __builtin_add_overflow

[Bruno Haible]

Stefan Liebler wrote:
> just as information, I've committed the glibc patch
> 
> "s390x: Require GCC 7.1 or later to build glibc."
> http://sourceware.org/git/?p=glibc.git;a=commit;h=844b4d8b4b937fe6943d2c0c80ce7d871cdb1eb5
> 
> as if build with gcc 6.5.0, __builtin_add_overflow incorrectly detects
> overflow on s390x.
> 
> Please find the link to gcc-Bug on the discussion on libc-alpha:
> ...
> >> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98269)
> > 
> > In any case, it would be good to alert gnulib developers about this
> > potential issue.

Thanks for the heads-up. Indeed, what I understand from this bug report is that
the condition code handling (required by __builtin_add_overflow on many
platforms) was unreliable before 2017-01-27, i.e. in GCC versions < 7.
So, gnulib shouldn't make use of it in these GCC versions.


2020-12-18  Bruno Haible  <bruno@clisp.org>

	intprops: Avoid potentially buggy __builtin_add_overflow in GCC 5, 6.
	Reported by Stefan Liebler <stli@linux.ibm.com> in
	<https://lists.gnu.org/archive/html/bug-gnulib/2020-12/msg00152.html>.
	* lib/intprops.h (_GL_HAS_BUILTIN_ADD_OVERFLOW): Don't define for
	GCC 5.x and 6.x.
	* lib/glob.c (size_add_wrapv): Don't use __builtin_add_overflow for
	GCC 5.x and 6.x.

diff --git a/lib/intprops.h b/lib/intprops.h
index 8cc8138..2472962 100644
--- a/lib/intprops.h
+++ b/lib/intprops.h
@@ -226,7 +226,9 @@
 
 /* True if __builtin_add_overflow (A, B, P) and __builtin_sub_overflow
    (A, B, P) work when P is non-null.  */
-#if 5 <= __GNUC__ && !defined __ICC
+/* __builtin_{add,sub}_overflow exists but is not reliable in GCC 5.x and 6.x,
+   see <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98269>.  */
+#if 7 <= __GNUC__ && !defined __ICC
 # define _GL_HAS_BUILTIN_ADD_OVERFLOW 1
 #elif defined __has_builtin
 # define _GL_HAS_BUILTIN_ADD_OVERFLOW __has_builtin (__builtin_add_overflow)
diff --git a/lib/glob.c b/lib/glob.c
index 32e2e11..3c444d3 100644
--- a/lib/glob.c
+++ b/lib/glob.c
@@ -227,7 +227,7 @@ glob_lstat (glob_t *pglob, int flags, const char *fullname)
 static bool
 size_add_wrapv (size_t a, size_t b, size_t *r)
 {
-#if 5 <= __GNUC__ && !defined __ICC
+#if 7 <= __GNUC__ && !defined __ICC
   return __builtin_add_overflow (a, b, r);
 #else
   *r = a + b;

Re: FYI: s390x: Require GCC 7.1 or later to build glibc due to __builtin_add_overflow

[Paul Eggert]

On 12/18/20 4:14 AM, Bruno Haible wrote:
> Thanks for the heads-up. Indeed, what I understand from this bug report is that
> the condition code handling (required by __builtin_add_overflow on many
> platforms) was unreliable before 2017-01-27, i.e. in GCC versions < 7.
> So, gnulib shouldn't make use of it in these GCC versions.

So the problem is on all platforms, not just s390x? If so, shouldn't GCC 7 
be required for building glibc on all platforms, not just s390x?

Even if we propagate the updated Gnulib intprops.h to glibc, glibc uses 
__builtin_add_overflow in several other places.

Re: FYI: s390x: Require GCC 7.1 or later to build glibc due to __builtin_add_overflow

[Bruno Haible]

Paul Eggert wrote:
> > Thanks for the heads-up. Indeed, what I understand from this bug report is that
> > the condition code handling (required by __builtin_add_overflow on many
> > platforms) was unreliable before 2017-01-27, i.e. in GCC versions < 7.
> > So, gnulib shouldn't make use of it in these GCC versions.
> 
> So the problem is on all platforms, not just s390x?

That's my understanding, from the cited
<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98269>.
Even if I'm wrong, given that GCC 5 and 6 are in decreasing use, I feel it's
better to err on the safe side.

Bruno

Re: FYI: s390x: Require GCC 7.1 or later to build glibc due to __builtin_add_overflow

[Paul Eggert]

On 12/18/20 10:32 AM, Bruno Haible wrote:

> Even if I'm wrong, given that GCC 5 and 6 are in decreasing use, I feel it's
> better to err on the safe side.

Thanks, I've followed up on this in the glibc side, here:

https://sourceware.org/pipermail/libc-alpha/2020-December/120876.html

Re: FYI: s390x: Require GCC 7.1 or later to build glibc due to __builtin_add_overflow

[Stefan Liebler]

On 12/18/20 7:32 PM, Bruno Haible wrote:
> Paul Eggert wrote:
>>> Thanks for the heads-up. Indeed, what I understand from this bug report is that
>>> the condition code handling (required by __builtin_add_overflow on many
>>> platforms) was unreliable before 2017-01-27, i.e. in GCC versions < 7.
>>> So, gnulib shouldn't make use of it in these GCC versions.
>>
>> So the problem is on all platforms, not just s390x?
> 
> That's my understanding, from the cited
> <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98269>.
> Even if I'm wrong, given that GCC 5 and 6 are in decreasing use, I feel it's
> better to err on the safe side.
> 
> Bruno
> 

According to Andreas comment in the gcc-bugzilla 98269, the issue was
fixed with a common-code patch. On s390x this issue was hidden due to a
s390-specific patch (committed before the common-code fix).

Thus, I also think, that this bug could happen on all platforms, but I
don't know if this really happens anywhere.

The gcc-bugzilla 78559 (=> common-code patch) was reported on aarch64
for gcc > 6.2.1 && <= 7.0. (If you have an aarch64 system, you could try it)

I've just build and run tst-gcc-addoverflow.c (attached to the
gcc-bugzilla 98269) on x86_64 with
gcc version 6.5.0 20181026 (Ubuntu 6.5.0-2ubuntu1~18.04)
=> __builtin_add_overflow() SUCCEED

Florian Weimer added the usage of __builtin_add_overflow which fails
with gcc 6.5 on s390x. After reporting the issue, he said, that he did
not recognize the issue on other platforms. But I don't know which gcc
versions or platforms he has used for testing.

Thanks,
Stefan