Another GLIBC build error with GCC6
Commit Message
On Tue, 2015-07-21 at 09:31 -0700, Steve Ellcey wrote:
> I just ran into this problem when compiling string/strchr in ILP32 mode
> on MIPS:
To be complete, I ran into 4 problems while building glibc with the
latest top-of-tree GCC.
strchrnul.c has the same problems as strchr.c. The definition of
DT_EXTRATAGIDX in elf/elf.h also gives a shift overflow warning and
elf/dl-dl-deps.c now gives an array out-of-bounds message. Here is a
not fully tested, not ready patch that allowed me to do a complete
build. I think this new definition of DT_EXTRATAGIDX would give the
same result as the old definition. I couldn't see anything to do with
dl-deps.c except ignore the warning, maybe someone else has a better
idea.
Steve Ellcey
sellcey@imgtec.com
Comments
On 07/21/2015 01:46 PM, Steve Ellcey wrote:
> On Tue, 2015-07-21 at 09:31 -0700, Steve Ellcey wrote:
>> I just ran into this problem when compiling string/strchr in ILP32 mode
>> on MIPS:
>
> To be complete, I ran into 4 problems while building glibc with the
> latest top-of-tree GCC.
>
> strchrnul.c has the same problems as strchr.c. The definition of
> DT_EXTRATAGIDX in elf/elf.h also gives a shift overflow warning and
> elf/dl-dl-deps.c now gives an array out-of-bounds message. Here is a
> not fully tested, not ready patch that allowed me to do a complete
> build. I think this new definition of DT_EXTRATAGIDX would give the
> same result as the old definition. I couldn't see anything to do with
> dl-deps.c except ignore the warning, maybe someone else has a better
> idea.
>
> Steve Ellcey
> sellcey@imgtec.com
>
>
> diff --git a/elf/dl-deps.c b/elf/dl-deps.c
> index eee146a..d4c5c00 100644
> --- a/elf/dl-deps.c
> +++ b/elf/dl-deps.c
> @@ -30,6 +30,8 @@
>
> #include <dl-dst.h>
>
> +#include <libc-internal.h>
> +
> /* Whether an shared object references one or more auxiliary objects
> is signaled by the AUXTAG entry in l_info. */
> #define AUXTAG (DT_NUM + DT_THISPROCNUM + DT_VERSIONTAGNUM \
> @@ -226,7 +228,11 @@ _dl_map_object_deps (struct link_map *map,
> needed = needed_space;
> }
>
> + /* Blah, blah. */
> + DIAG_PUSH_NEEDS_COMMENT;
> + DIAG_IGNORE_NEEDS_COMMENT (4.6, "-Warray-bounds");
> if (l->l_info[DT_NEEDED] || l->l_info[AUXTAG] || l->l_info[FILTERTAG])
> + DIAG_POP_NEEDS_COMMENT;
> {
> const char *strtab = (const void *) D_PTR (l, l_info[DT_STRTAB]);
> struct openaux_args args;
Any chance I could get you to file a GCC bug for this bogus warning?
Jeff
On Tue, 2015-07-21 at 14:10 -0600, Jeff Law wrote:
> > diff --git a/elf/dl-deps.c b/elf/dl-deps.c
> > index eee146a..d4c5c00 100644
> > --- a/elf/dl-deps.c
> > +++ b/elf/dl-deps.c
> > @@ -30,6 +30,8 @@
> >
> > #include <dl-dst.h>
> >
> > +#include <libc-internal.h>
> > +
> > /* Whether an shared object references one or more auxiliary objects
> > is signaled by the AUXTAG entry in l_info. */
> > #define AUXTAG (DT_NUM + DT_THISPROCNUM + DT_VERSIONTAGNUM \
> > @@ -226,7 +228,11 @@ _dl_map_object_deps (struct link_map *map,
> > needed = needed_space;
> > }
> >
> > + /* Blah, blah. */
> > + DIAG_PUSH_NEEDS_COMMENT;
> > + DIAG_IGNORE_NEEDS_COMMENT (4.6, "-Warray-bounds");
> > if (l->l_info[DT_NEEDED] || l->l_info[AUXTAG] || l->l_info[FILTERTAG])
> > + DIAG_POP_NEEDS_COMMENT;
> > {
> > const char *strtab = (const void *) D_PTR (l, l_info[DT_STRTAB]);
> > struct openaux_args args;
> Any chance I could get you to file a GCC bug for this bogus warning?
>
> Jeff
Is it bogus? I never followed the various macros down far enough to see
if the references were out of bounds or not. I will see if I can create
a cut-down test case.
Steve Ellcey
sellcey@imgtec.com
> Any chance I could get you to file a GCC bug for this bogus warning?
>
> Jeff
Here is a cutdown test case, but I am not sure exactly what
DT_EXTRATAGIDX is trying to accomplish so I can't explain why GCC should
(or should not) give a warning here. If you define MODIFIED you get
the code with my new definition of DT_EXTRATAGIDX and fewer warnings
but the if statement still gives a warning in either case.
Steve Ellcey
#include <stdint.h>
typedef uint32_t Elf32_Word;
typedef int32_t Elf32_Sword;
typedef struct
{
int x;
} Elf32_Dyn;
#define DT_NUM 34
#define DT_MIPS_NUM 0x36
#define DT_THISPROCNUM DT_MIPS_NUM
#define DT_VERSIONTAGNUM 16
#define DT_EXTRANUM 3
#define DT_VALNUM 12
#define DT_ADDRNUM 11
#define DT_AUXILIARY 0x7ffffffd
#define DT_FILTER 0x7fffffff
#ifndef MODIFIED
#define DT_EXTRATAGIDX(tag) ((Elf32_Word)-((Elf32_Sword) (tag) <<1>>1)-1) /* original version */
#else
#define DT_EXTRATAGIDX(tag) ((Elf32_Word)-((Elf32_Sword) (tag) & 0x7fffffff)-1) /* my new version */
#endif
#define DT_NEEDED 1
#define AUXTAG (DT_NUM + DT_THISPROCNUM + DT_VERSIONTAGNUM \
+ DT_EXTRATAGIDX (DT_AUXILIARY))
#define FILTERTAG (DT_NUM + DT_THISPROCNUM + DT_VERSIONTAGNUM \
+ DT_EXTRATAGIDX (DT_FILTER))
/* include/elf.h */
Elf32_Dyn *l_info[DT_NUM + DT_THISPROCNUM + DT_VERSIONTAGNUM
+ DT_EXTRANUM + DT_VALNUM + DT_ADDRNUM];
int foo()
{
if (l_info[DT_NEEDED] || l_info[AUXTAG] || l_info[FILTERTAG])
return 1;
else
return 0;
}
On 07/21/2015 02:16 PM, Steve Ellcey wrote:
> On Tue, 2015-07-21 at 14:10 -0600, Jeff Law wrote:
>
>>> diff --git a/elf/dl-deps.c b/elf/dl-deps.c
>>> index eee146a..d4c5c00 100644
>>> --- a/elf/dl-deps.c
>>> +++ b/elf/dl-deps.c
>>> @@ -30,6 +30,8 @@
>>>
>>> #include <dl-dst.h>
>>>
>>> +#include <libc-internal.h>
>>> +
>>> /* Whether an shared object references one or more auxiliary objects
>>> is signaled by the AUXTAG entry in l_info. */
>>> #define AUXTAG (DT_NUM + DT_THISPROCNUM + DT_VERSIONTAGNUM \
>>> @@ -226,7 +228,11 @@ _dl_map_object_deps (struct link_map *map,
>>> needed = needed_space;
>>> }
>>>
>>> + /* Blah, blah. */
>>> + DIAG_PUSH_NEEDS_COMMENT;
>>> + DIAG_IGNORE_NEEDS_COMMENT (4.6, "-Warray-bounds");
>>> if (l->l_info[DT_NEEDED] || l->l_info[AUXTAG] || l->l_info[FILTERTAG])
>>> + DIAG_POP_NEEDS_COMMENT;
>>> {
>>> const char *strtab = (const void *) D_PTR (l, l_info[DT_STRTAB]);
>>> struct openaux_args args;
>> Any chance I could get you to file a GCC bug for this bogus warning?
>>
>> Jeff
>
> Is it bogus? I never followed the various macros down far enough to see
> if the references were out of bounds or not. I will see if I can create
> a cut-down test case.
If it's not a false positive, then you're suppressing a warning on code
that's got undefined behaviour... That seems unwise.
Jeff
On Tue, 2015-07-21 at 15:22 -0600, Jeff Law wrote:
> >> Any chance I could get you to file a GCC bug for this bogus warning?
> >>
> >> Jeff
> >
> > Is it bogus? I never followed the various macros down far enough to see
> > if the references were out of bounds or not. I will see if I can create
> > a cut-down test case.
> If it's not a false positive, then you're suppressing a warning on code
> that's got undefined behaviour... That seems unwise.
>
> Jeff
Ok, the array out of bound error was due to my change to the
DT_EXTRATAGIDX macro which was invalid. If I undo that change the array
out of bounds message goes away but I get the shift-overflow messages:
#include <stdint.h>
#include <stdio.h>
int main()
{
printf("%d\n", 0x7ffffffd);
printf("%d\n", (0x7ffffffd << 1));
printf("%d\n", ((0x7ffffffd << 1) >> 1));
}
The latest GCC complains with:
z.c: In function 'main':
z.c:7:29: warning: result of '2147483645 << 1' requires 33 bits to
represent, but 'int' only has 32 bits [-Wshift-overflow=]
printf("%d\n", (0x7ffffffd << 1));
^
z.c:8:30: warning: result of '2147483645 << 1' requires 33 bits to
represent, but 'int' only has 32 bits [-Wshift-overflow=]
printf("%d\n", ((0x7ffffffd << 1) >> 1));
^
Would you say that it is a GCC bug to complain about this? I don't see
any reason why it should complain. shifting is going to lose some bits
off the end of the constant but isn't that what a shift does?
Steve Ellcey
sellcey@imgtec.com
Steve Ellcey <sellcey@imgtec.com> writes:
> Here is a cutdown test case, but I am not sure exactly what
> DT_EXTRATAGIDX is trying to accomplish
It's an elaborate (and undefined) way to write (DT_FILTER - (tag)).
> so I can't explain why GCC should
> (or should not) give a warning here. If you define MODIFIED you get
> the code with my new definition of DT_EXTRATAGIDX and fewer warnings
> but the if statement still gives a warning in either case.
Your new definition produces an overflow. The original one is merely
undefined.
Andreas.
In this case, 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). In
fact, 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.
Roland McGrath <roland@hack.frob.com> writes:
> In this case, 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). In
> fact, 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.
Only if you make 0x7fffffff unsigned.
Andreas.
@@ -30,6 +30,8 @@
#include <dl-dst.h>
+#include <libc-internal.h>
+
/* Whether an shared object references one or more auxiliary objects
is signaled by the AUXTAG entry in l_info. */
#define AUXTAG (DT_NUM + DT_THISPROCNUM + DT_VERSIONTAGNUM \
@@ -226,7 +228,11 @@ _dl_map_object_deps (struct link_map *map,
needed = needed_space;
}
+ /* Blah, blah. */
+ DIAG_PUSH_NEEDS_COMMENT;
+ DIAG_IGNORE_NEEDS_COMMENT (4.6, "-Warray-bounds");
if (l->l_info[DT_NEEDED] || l->l_info[AUXTAG] || l->l_info[FILTERTAG])
+ DIAG_POP_NEEDS_COMMENT;
{
const char *strtab = (const void *) D_PTR (l, l_info[DT_STRTAB]);
struct openaux_args args;
@@ -801,7 +801,7 @@ typedef struct
range. Be compatible. */
#define DT_AUXILIARY 0x7ffffffd /* Shared object to load before self */
#define DT_FILTER 0x7fffffff /* Shared object to get values from */
-#define DT_EXTRATAGIDX(tag) ((Elf32_Word)-((Elf32_Sword) (tag) <<1>>1)-1)
+#define DT_EXTRATAGIDX(tag) ((Elf32_Word)-((Elf32_Sword) (tag) & 0x7fffffff)-1)
#define DT_EXTRANUM 3
/* Values of `d_un.d_val' in the DT_FLAGS entry. */
@@ -60,22 +60,19 @@ 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 ();
- }
- /* Set up a longword, each of whose bytes is C. */
+#if __WORDSIZE == 32
+ magic_bits = 0x7efefeffL;
charmask = c | (c << 8);
charmask |= charmask << 16;
- if (sizeof (longword) > 4)
- /* Do the shift in two steps to avoid a warning if long has 32 bits. */
- charmask |= (charmask << 16) << 16;
- if (sizeof (longword) > 8)
- abort ();
+#elif __WORDSIZE == 64
+ magic_bits = ((0x7efefefeL << 16) << 16) | 0xfefefeffL;
+ charmask = c | (c << 8);
+ charmask |= charmask << 16;
+ charmask |= (charmask << 16) << 16;
+#else
+ #error unexpected integer size strchr()
+#endif
/* Instead of the traditional loop which tests each character,
we will test a longword at a time. The tricky part is testing
@@ -66,22 +66,19 @@ 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 ();
- }
- /* Set up a longword, each of whose bytes is C. */
+#if __WORDSIZE == 32
+ magic_bits = 0x7efefeffL;
charmask = c | (c << 8);
charmask |= charmask << 16;
- if (sizeof (longword) > 4)
- /* Do the shift in two steps to avoid a warning if long has 32 bits. */
- charmask |= (charmask << 16) << 16;
- if (sizeof (longword) > 8)
- abort ();
+#elif __WORDSIZE == 64
+ magic_bits = ((0x7efefefeL << 16) << 16) | 0xfefefeffL;
+ charmask = c | (c << 8);
+ charmask |= charmask << 16;
+ charmask |= (charmask << 16) << 16;
+#else
+ #error unexpected integer size strchr()
+#endif
/* Instead of the traditional loop which tests each character,
we will test a longword at a time. The tricky part is testing