Another GLIBC build error with GCC6
Commit Message
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
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
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.
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
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.
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
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
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.
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
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
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
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
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
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
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(-)
@@ -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
@@ -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);
@@ -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);
@@ -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);
@@ -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