Message ID | 55AF30A1.2030402@cs.ucla.edu |
---|---|
State | New |
Headers | show |
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(-) 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