diff mbox

Another GLIBC build error with GCC6

Message ID 55AF30A1.2030402@cs.ucla.edu
State Committed
Headers show

Commit Message

Paul Eggert July 22, 2015, 5:56 a.m. UTC
Roland McGrath wrote:
> the left shift doesn't even lose any bits, because the high
> bit is already zero (and it's a constant, so the compiler knows that).

But it does lose a bit.  It loses a zero bit, which can't be recovered the way 
that a non-overflowing signed left shift is recovered by shifting right the same 
amount.

> that's the whole point of the thing: to sign-extend the 31-bit value
> to 32 bits.  I don't think it should complain about 0x7fffffff << 1.

Such complaints are useful for compilers that take advantage of C's overflow 
rules to generate efficient-but-weird code when you violate the rules (something 
that describes GCC more and more nowadays...).

Anyway, if I understand this one correctly it should be easy enough to fix 
portably.  Something like the attached patch, say.  (I haven't tested it.)

Comments

Steve Ellcey July 22, 2015, 3:26 p.m. UTC | #1
On Tue, 2015-07-21 at 22:56 -0700, Paul Eggert wrote:

> Anyway, if I understand this one correctly it should be easy enough to fix 
> portably.  Something like the attached patch, say.  (I haven't tested it.)

This doesn't address the problem with the DT_EXTRATAGIDX macro.  That is
the one that I am not sure how to fix.

The macro is:

#define DT_EXTRATAGIDX(tag)     ((Elf32_Word)-((Elf32_Sword) (tag) <<1>>1)-1)


and it is called with tag being a constant value of either 0x7fffffff or
0x7ffffffd.

Steve Ellcey
sellcey@imgtec.com
Andreas Schwab July 22, 2015, 4:09 p.m. UTC | #2
Steve Ellcey <sellcey@imgtec.com> writes:

> This doesn't address the problem with the DT_EXTRATAGIDX macro.  That is
> the one that I am not sure how to fix.

Didn't the simplified definition work?

Andreas.
Steve Ellcey July 22, 2015, 4:19 p.m. UTC | #3
On Wed, 2015-07-22 at 18:09 +0200, Andreas Schwab wrote:
> Steve Ellcey <sellcey@imgtec.com> writes:
> 
> > This doesn't address the problem with the DT_EXTRATAGIDX macro.  That is
> > the one that I am not sure how to fix.
> 
> Didn't the simplified definition work?
> 
> Andreas.

Do you mean my simplified version where I tried changing:

#define DT_EXTRATAGIDX(tag)     ((Elf32_Word)-((Elf32_Sword) (tag) <<1>>1)-1)

into:

#define DT_EXTRATAGIDX(tag) ((Elf32_Word)-((Elf32_Sword) (tag) & 0x7fffffff)-1)

Then, no that did not work.  The two macros are not equivalent because
my version just zeros out the sign bit whereas the original version
extended the sign bit into the next bit.  I.e. I thought the original
was doing a logical right shift but it was doing an arithmetic right
shift so they do not give the same results.

Steve Ellcey
sellcey@imgtec.com
Andreas Schwab July 22, 2015, 4:30 p.m. UTC | #4
Steve Ellcey <sellcey@imgtec.com> writes:

> On Wed, 2015-07-22 at 18:09 +0200, Andreas Schwab wrote:
>> Steve Ellcey <sellcey@imgtec.com> writes:
>> 
>> > This doesn't address the problem with the DT_EXTRATAGIDX macro.  That is
>> > the one that I am not sure how to fix.
>> 
>> Didn't the simplified definition work?
>> 
>> Andreas.
>
> Do you mean my simplified version where I tried changing:

No, the one I gave.

Andreas.
Steve Ellcey July 22, 2015, 4:36 p.m. UTC | #5
On Wed, 2015-07-22 at 18:30 +0200, Andreas Schwab wrote:
> Steve Ellcey <sellcey@imgtec.com> writes:
> 
> > On Wed, 2015-07-22 at 18:09 +0200, Andreas Schwab wrote:
> >> Steve Ellcey <sellcey@imgtec.com> writes:
> >> 
> >> > This doesn't address the problem with the DT_EXTRATAGIDX macro.  That is
> >> > the one that I am not sure how to fix.
> >> 
> >> Didn't the simplified definition work?
> >> 
> >> Andreas.
> >
> > Do you mean my simplified version where I tried changing:
> 
> No, the one I gave.
> 
> Andreas.

I can't seem to find it in my email, can you send it again?

Steve
Steve Ellcey Aug. 18, 2015, 4:12 p.m. UTC | #6
On Tue, 2015-07-21 at 22:56 -0700, Paul Eggert wrote:
> Roland McGrath wrote:
> > the left shift doesn't even lose any bits, because the high
> > bit is already zero (and it's a constant, so the compiler knows that).
> 
> But it does lose a bit.  It loses a zero bit, which can't be recovered the way 
> that a non-overflowing signed left shift is recovered by shifting right the same 
> amount.
> 
> > that's the whole point of the thing: to sign-extend the 31-bit value
> > to 32 bits.  I don't think it should complain about 0x7fffffff << 1.
> 
> Such complaints are useful for compilers that take advantage of C's overflow 
> rules to generate efficient-but-weird code when you violate the rules (something 
> that describes GCC more and more nowadays...).
> 
> Anyway, if I understand this one correctly it should be easy enough to fix 
> portably.  Something like the attached patch, say.  (I haven't tested it.)

Paul,

This patch looks good to me and works fine on MIPS, now that the GLIBC
trunk is unfrozen do you want to check it in?  I would like to get GLIBC
building again with the latest GCC and this is one of the things that
needs fixing in order to get rid of the warnings.

Steve Ellcey
sellcey@imgtec.com
Joseph Myers Aug. 18, 2015, 4:20 p.m. UTC | #7
On Tue, 18 Aug 2015, Steve Ellcey wrote:

> This patch looks good to me and works fine on MIPS, now that the GLIBC

Have you verified that the generated code (with --disable-werror or older 
GCC) is unchanged before and after the patch (as it should be)?  If so, I 
also think it should be checked in.

Maybe eventually these implementations can be replaced using Ondřej's 
generic string function infrastructure.
Steve Ellcey Aug. 18, 2015, 4:25 p.m. UTC | #8
On Tue, 2015-08-18 at 16:20 +0000, Joseph Myers wrote:
> On Tue, 18 Aug 2015, Steve Ellcey wrote:
> 
> > This patch looks good to me and works fine on MIPS, now that the GLIBC
> 
> Have you verified that the generated code (with --disable-werror or older 
> GCC) is unchanged before and after the patch (as it should be)?  If so, I 
> also think it should be checked in.
> 
> Maybe eventually these implementations can be replaced using Ondřej's 
> generic string function infrastructure.

I didn't look at the entire glibc functions, but I put the old and new
code segments in a small test program and compiled it on MIPS (32 and 64
bit modes) and verified that the generated code was identical for both
versions in that setting.

Steve Ellcey
sellcey@imgtec.com
Paul Eggert Aug. 18, 2015, 5:04 p.m. UTC | #9
Steve Ellcey wrote:
> I put the old and new
> code segments in a small test program and compiled it on MIPS (32 and 64
> bit modes) and verified that the generated code was identical for both
> versions in that setting.

That's good enough for me.  I installed the patch.

Wasn't there a similar problem with DT_EXTRATAGIDX?  See, for example:

https://sourceware.org/ml/libc-alpha/2015-07/msg00742.html
Steve Ellcey Aug. 18, 2015, 5:13 p.m. UTC | #10
On Tue, 2015-08-18 at 10:04 -0700, Paul Eggert wrote:
> Steve Ellcey wrote:
> > I put the old and new
> > code segments in a small test program and compiled it on MIPS (32 and 64
> > bit modes) and verified that the generated code was identical for both
> > versions in that setting.
> 
> That's good enough for me.  I installed the patch.
> 
> Wasn't there a similar problem with DT_EXTRATAGIDX?  See, for example:
> 
> https://sourceware.org/ml/libc-alpha/2015-07/msg00742.html

For some reason, I am no longer seeing this problem.  I don't see any
changes in GLIBC or GCC that look like they would have made this problem
go away so I am not sure what is going on.

Steve Ellcey
sellcey@imgtec.com
Marek Polacek Aug. 18, 2015, 5:18 p.m. UTC | #11
On Tue, Aug 18, 2015 at 10:13:00AM -0700, Steve Ellcey wrote:
> On Tue, 2015-08-18 at 10:04 -0700, Paul Eggert wrote:
> > Steve Ellcey wrote:
> > > I put the old and new
> > > code segments in a small test program and compiled it on MIPS (32 and 64
> > > bit modes) and verified that the generated code was identical for both
> > > versions in that setting.
> > 
> > That's good enough for me.  I installed the patch.
> > 
> > Wasn't there a similar problem with DT_EXTRATAGIDX?  See, for example:
> > 
> > https://sourceware.org/ml/libc-alpha/2015-07/msg00742.html
> 
> For some reason, I am no longer seeing this problem.  I don't see any
> changes in GLIBC or GCC that look like they would have made this problem
> go away so I am not sure what is going on.

If this is about shift overflow warnings:
I've changed the -Wshift-overflow warning to not warn when left-shifting 1
into the sign bit.  It caused way too much noise.  If you still want to see
even those, use -Wshift-overflow=2.
<https://gcc.gnu.org/ml/gcc-patches/2015-08/msg00617.html>

	Marek
Jeff Law Aug. 18, 2015, 5:25 p.m. UTC | #12
On 08/18/2015 11:18 AM, Marek Polacek wrote:
> On Tue, Aug 18, 2015 at 10:13:00AM -0700, Steve Ellcey wrote:
>> On Tue, 2015-08-18 at 10:04 -0700, Paul Eggert wrote:
>>> Steve Ellcey wrote:
>>>> I put the old and new
>>>> code segments in a small test program and compiled it on MIPS (32 and 64
>>>> bit modes) and verified that the generated code was identical for both
>>>> versions in that setting.
>>>
>>> That's good enough for me.  I installed the patch.
>>>
>>> Wasn't there a similar problem with DT_EXTRATAGIDX?  See, for example:
>>>
>>> https://sourceware.org/ml/libc-alpha/2015-07/msg00742.html
>>
>> For some reason, I am no longer seeing this problem.  I don't see any
>> changes in GLIBC or GCC that look like they would have made this problem
>> go away so I am not sure what is going on.
>
> If this is about shift overflow warnings:
> I've changed the -Wshift-overflow warning to not warn when left-shifting 1
> into the sign bit.  It caused way too much noise.  If you still want to see
> even those, use -Wshift-overflow=2.
> <https://gcc.gnu.org/ml/gcc-patches/2015-08/msg00617.html>
And note that left shifting 1 into the sign bit may be downgraded from 
undefined to implementation defined behaviour for C (it's already 
changed for C++14).

Jeff
Steve Ellcey Aug. 18, 2015, 5:26 p.m. UTC | #13
On Tue, 2015-08-18 at 19:18 +0200, Marek Polacek wrote:
> On Tue, Aug 18, 2015 at 10:13:00AM -0700, Steve Ellcey wrote:
> > On Tue, 2015-08-18 at 10:04 -0700, Paul Eggert wrote:
> > > Steve Ellcey wrote:
> > > > I put the old and new
> > > > code segments in a small test program and compiled it on MIPS (32 and 64
> > > > bit modes) and verified that the generated code was identical for both
> > > > versions in that setting.
> > > 
> > > That's good enough for me.  I installed the patch.
> > > 
> > > Wasn't there a similar problem with DT_EXTRATAGIDX?  See, for example:
> > > 
> > > https://sourceware.org/ml/libc-alpha/2015-07/msg00742.html
> > 
> > For some reason, I am no longer seeing this problem.  I don't see any
> > changes in GLIBC or GCC that look like they would have made this problem
> > go away so I am not sure what is going on.
> 
> If this is about shift overflow warnings:
> I've changed the -Wshift-overflow warning to not warn when left-shifting 1
> into the sign bit.  It caused way too much noise.  If you still want to see
> even those, use -Wshift-overflow=2.
> <https://gcc.gnu.org/ml/gcc-patches/2015-08/msg00617.html>
> 
> 	Marek

Yes, that was the issue.  It might still be worth cleaning up the
definition of this macro as was discussed in the email thread even
though we aren't getting warnings any more.  I will see if I can do that
sometime in the next few weeks.

Steve Ellcey
sellcey@imgtec.com
diff mbox

Patch

From 64586c5448f6f5537bd36bb5f271324790055c52 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 21 Jul 2015 22:50:29 -0700
Subject: [PATCH] Port the 0x7efe...feff pattern to GCC 6.

See Steve Ellcey's bug report in:
https://sourceware.org/ml/libc-alpha/2015-07/msg00673.html
* string/memrchr.c (MEMRCHR):
* string/rawmemchr.c (RAWMEMCHR):
* string/strchr.c (strchr):
* string/strchrnul.c (STRCHRNUL):
Rewrite code to avoid issues with signed shift overflow.
---
 ChangeLog          | 11 +++++++++++
 string/memrchr.c   | 11 ++---------
 string/rawmemchr.c | 11 ++---------
 string/strchr.c    |  9 ++-------
 string/strchrnul.c |  9 ++-------
 5 files changed, 19 insertions(+), 32 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 652d968..9f87cd9 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@ 
+2015-07-21  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Port the 0x7efe...feff pattern to GCC 6.
+	See Steve Ellcey's bug report in:
+	https://sourceware.org/ml/libc-alpha/2015-07/msg00673.html
+	* string/memrchr.c (MEMRCHR):
+	* string/rawmemchr.c (RAWMEMCHR):
+	* string/strchr.c (strchr):
+	* string/strchrnul.c (STRCHRNUL):
+	Rewrite code to avoid issues with signed shift overflow.
+
 2015-07-22  Mike Frysinger  <vapier@gentoo.org>
 
 	* sysdeps/unix/sysv/linux/ia64/bits/msq.h: Change sys/types.h include
diff --git a/string/memrchr.c b/string/memrchr.c
index 0c8fd84..86cd5b9 100644
--- a/string/memrchr.c
+++ b/string/memrchr.c
@@ -96,15 +96,8 @@  MEMRCHR
 
      The 1-bits make sure that carries propagate to the next 0-bit.
      The 0-bits provide holes for carries to fall into.  */
-
-  if (sizeof (longword) != 4 && sizeof (longword) != 8)
-    abort ();
-
-#if LONG_MAX <= LONG_MAX_32_BITS
-  magic_bits = 0x7efefeff;
-#else
-  magic_bits = ((unsigned long int) 0x7efefefe << 32) | 0xfefefeff;
-#endif
+  magic_bits = -1;
+  magic_bits = magic_bits / 0xff * 0xfe << 1 >> 1 | 1;
 
   /* Set up a longword, each of whose bytes is C.  */
   charmask = c | (c << 8);
diff --git a/string/rawmemchr.c b/string/rawmemchr.c
index 05b22be..228ca9d 100644
--- a/string/rawmemchr.c
+++ b/string/rawmemchr.c
@@ -86,15 +86,8 @@  RAWMEMCHR (s, c_in)
 
      The 1-bits make sure that carries propagate to the next 0-bit.
      The 0-bits provide holes for carries to fall into.  */
-
-  if (sizeof (longword) != 4 && sizeof (longword) != 8)
-    abort ();
-
-#if LONG_MAX <= LONG_MAX_32_BITS
-  magic_bits = 0x7efefeff;
-#else
-  magic_bits = ((unsigned long int) 0x7efefefe << 32) | 0xfefefeff;
-#endif
+  magic_bits = -1;
+  magic_bits = magic_bits / 0xff * 0xfe << 1 >> 1 | 1;
 
   /* Set up a longword, each of whose bytes is C.  */
   charmask = c | (c << 8);
diff --git a/string/strchr.c b/string/strchr.c
index 5f90075..f13b2b3 100644
--- a/string/strchr.c
+++ b/string/strchr.c
@@ -60,13 +60,8 @@  strchr (const char *s, int c_in)
 
      The 1-bits make sure that carries propagate to the next 0-bit.
      The 0-bits provide holes for carries to fall into.  */
-  switch (sizeof (longword))
-    {
-    case 4: magic_bits = 0x7efefeffL; break;
-    case 8: magic_bits = ((0x7efefefeL << 16) << 16) | 0xfefefeffL; break;
-    default:
-      abort ();
-    }
+  magic_bits = -1;
+  magic_bits = magic_bits / 0xff * 0xfe << 1 >> 1 | 1;
 
   /* Set up a longword, each of whose bytes is C.  */
   charmask = c | (c << 8);
diff --git a/string/strchrnul.c b/string/strchrnul.c
index 2678f1d..daf0b3f 100644
--- a/string/strchrnul.c
+++ b/string/strchrnul.c
@@ -66,13 +66,8 @@  STRCHRNUL (s, c_in)
 
      The 1-bits make sure that carries propagate to the next 0-bit.
      The 0-bits provide holes for carries to fall into.  */
-  switch (sizeof (longword))
-    {
-    case 4: magic_bits = 0x7efefeffL; break;
-    case 8: magic_bits = ((0x7efefefeL << 16) << 16) | 0xfefefeffL; break;
-    default:
-      abort ();
-    }
+  magic_bits = -1;
+  magic_bits = magic_bits / 0xff * 0xfe << 1 >> 1 | 1;
 
   /* Set up a longword, each of whose bytes is C.  */
   charmask = c | (c << 8);
-- 
2.1.0