Patchwork [testsuite] : Fix PR target/48055; FAIL: gcc.c-torture/execute/builtins/memcpy-chk.c compilation, -O2 -flto

login
register
mail settings
Submitter Uros Bizjak
Date March 10, 2011, 12:19 p.m.
Message ID <AANLkTinAEXYRQ_zk3u1dkb7U7DB20XJVxi0hmxKzBfn2@mail.gmail.com>
Download mbox | patch
Permalink /patch/86259/
State New
Headers show

Comments

Uros Bizjak - March 10, 2011, 12:19 p.m.
Hello!

Using binutils-2.21, a couple of
gcc.c-torture/execute/builtins/__-chk.c testcases fail on
alphaev68-pc-linux-gnu (-lto) with:

/usr/lib/gcc/alpha-unknown-linux-gnu/4.4.5/../../../../alpha-unknown-linux-gnu/bin/ld:
Warning: alignment 8 of symbol `buf5' in
/tmp/ccgnDykf.ltrans1.ltrans.o is smaller than 16 in
/tmp/ccc3QsSw.o.ironly
/usr/lib/gcc/alpha-unknown-linux-gnu/4.4.5/../../../../alpha-unknown-linux-gnu/bin/ld:
Warning: alignment 8 of symbol `buf7' in
/tmp/ccgnDykf.ltrans1.ltrans.o is smaller than 16 in
/tmp/ccc3QsSw.o.ironly
/usr/lib/gcc/alpha-unknown-linux-gnu/4.4.5/../../../../alpha-unknown-linux-gnu/bin/ld:
Warning: alignment 8 of symbol `buf1' in
/tmp/ccgnDykf.ltrans1.ltrans.o is smaller than 16 in
/tmp/ccc3QsSw.o.ironly

Attached patch fixes these failures.

2011-03-10  Uros Bizjak  <ubizjak@gmail.com>

	PR testsuite/48055
	* gcc.c-torture/execute/builtins/memcpy-chk.c (buf1, buf5, buf7):
	Declare as static.
	* gcc.c-torture/execute/builtins/mempcpy-chk.c (buf1, buf5, buf7):
	Ditto.
	* gcc.c-torture/execute/builtins/memmove-chk.c (buf1, buf5, buf7):
	Ditto.

Tested on alphaev68-pc-linux-gnu with binutils-2.21. OK for mainline
and release branches?

Uros
Richard Guenther - March 10, 2011, 12:30 p.m.
On Thu, Mar 10, 2011 at 1:19 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> Hello!
>
> Using binutils-2.21, a couple of
> gcc.c-torture/execute/builtins/__-chk.c testcases fail on
> alphaev68-pc-linux-gnu (-lto) with:
>
> /usr/lib/gcc/alpha-unknown-linux-gnu/4.4.5/../../../../alpha-unknown-linux-gnu/bin/ld:
> Warning: alignment 8 of symbol `buf5' in
> /tmp/ccgnDykf.ltrans1.ltrans.o is smaller than 16 in
> /tmp/ccc3QsSw.o.ironly
> /usr/lib/gcc/alpha-unknown-linux-gnu/4.4.5/../../../../alpha-unknown-linux-gnu/bin/ld:
> Warning: alignment 8 of symbol `buf7' in
> /tmp/ccgnDykf.ltrans1.ltrans.o is smaller than 16 in
> /tmp/ccc3QsSw.o.ironly
> /usr/lib/gcc/alpha-unknown-linux-gnu/4.4.5/../../../../alpha-unknown-linux-gnu/bin/ld:
> Warning: alignment 8 of symbol `buf1' in
> /tmp/ccgnDykf.ltrans1.ltrans.o is smaller than 16 in
> /tmp/ccc3QsSw.o.ironly
>
> Attached patch fixes these failures.

I think this needs more investigation as there are no conflicting
definitions of those vars that would warrant this kind of diagnostic from ld.

We probably bring the vars local by making them hidden and distribute
them to multiple ltrans units (with only one definiton obviously and multiple
externs).

Richard.

> 2011-03-10  Uros Bizjak  <ubizjak@gmail.com>
>
>        PR testsuite/48055
>        * gcc.c-torture/execute/builtins/memcpy-chk.c (buf1, buf5, buf7):
>        Declare as static.
>        * gcc.c-torture/execute/builtins/mempcpy-chk.c (buf1, buf5, buf7):
>        Ditto.
>        * gcc.c-torture/execute/builtins/memmove-chk.c (buf1, buf5, buf7):
>        Ditto.
>
> Tested on alphaev68-pc-linux-gnu with binutils-2.21. OK for mainline
> and release branches?
>
> Uros
>
Uros Bizjak - March 10, 2011, 2:44 p.m.
On Thu, Mar 10, 2011 at 1:30 PM, Richard Guenther
<richard.guenther@gmail.com> wrote:

>> Using binutils-2.21, a couple of
>> gcc.c-torture/execute/builtins/__-chk.c testcases fail on
>> alphaev68-pc-linux-gnu (-lto) with:
>>
>> /usr/lib/gcc/alpha-unknown-linux-gnu/4.4.5/../../../../alpha-unknown-linux-gnu/bin/ld:
>> Warning: alignment 8 of symbol `buf5' in
>> /tmp/ccgnDykf.ltrans1.ltrans.o is smaller than 16 in
>> /tmp/ccc3QsSw.o.ironly
>> /usr/lib/gcc/alpha-unknown-linux-gnu/4.4.5/../../../../alpha-unknown-linux-gnu/bin/ld:
>> Warning: alignment 8 of symbol `buf7' in
>> /tmp/ccgnDykf.ltrans1.ltrans.o is smaller than 16 in
>> /tmp/ccc3QsSw.o.ironly
>> /usr/lib/gcc/alpha-unknown-linux-gnu/4.4.5/../../../../alpha-unknown-linux-gnu/bin/ld:
>> Warning: alignment 8 of symbol `buf1' in
>> /tmp/ccgnDykf.ltrans1.ltrans.o is smaller than 16 in
>> /tmp/ccc3QsSw.o.ironly
>>
>> Attached patch fixes these failures.
>
> I think this needs more investigation as there are no conflicting
> definitions of those vars that would warrant this kind of diagnostic from ld.
>
> We probably bring the vars local by making them hidden and distribute
> them to multiple ltrans units (with only one definiton obviously and multiple
> externs).

Jan claims that this problem is a linker bug, so perhaps the patch is
still suitable for the gcc testsuite. The patch doesn't alter tests in
any way, while it avoids false positives due to linker bugs.

The linker problem was filled as binutils PR 12564 [1] against ld 2.21.

[1] http://sourceware.org/bugzilla/show_bug.cgi?id=12564

Uros.
Richard Guenther - March 10, 2011, 2:59 p.m.
On Thu, Mar 10, 2011 at 3:44 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Thu, Mar 10, 2011 at 1:30 PM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>
>>> Using binutils-2.21, a couple of
>>> gcc.c-torture/execute/builtins/__-chk.c testcases fail on
>>> alphaev68-pc-linux-gnu (-lto) with:
>>>
>>> /usr/lib/gcc/alpha-unknown-linux-gnu/4.4.5/../../../../alpha-unknown-linux-gnu/bin/ld:
>>> Warning: alignment 8 of symbol `buf5' in
>>> /tmp/ccgnDykf.ltrans1.ltrans.o is smaller than 16 in
>>> /tmp/ccc3QsSw.o.ironly
>>> /usr/lib/gcc/alpha-unknown-linux-gnu/4.4.5/../../../../alpha-unknown-linux-gnu/bin/ld:
>>> Warning: alignment 8 of symbol `buf7' in
>>> /tmp/ccgnDykf.ltrans1.ltrans.o is smaller than 16 in
>>> /tmp/ccc3QsSw.o.ironly
>>> /usr/lib/gcc/alpha-unknown-linux-gnu/4.4.5/../../../../alpha-unknown-linux-gnu/bin/ld:
>>> Warning: alignment 8 of symbol `buf1' in
>>> /tmp/ccgnDykf.ltrans1.ltrans.o is smaller than 16 in
>>> /tmp/ccc3QsSw.o.ironly
>>>
>>> Attached patch fixes these failures.
>>
>> I think this needs more investigation as there are no conflicting
>> definitions of those vars that would warrant this kind of diagnostic from ld.
>>
>> We probably bring the vars local by making them hidden and distribute
>> them to multiple ltrans units (with only one definiton obviously and multiple
>> externs).
>
> Jan claims that this problem is a linker bug, so perhaps the patch is
> still suitable for the gcc testsuite. The patch doesn't alter tests in
> any way, while it avoids false positives due to linker bugs.

Are you sure?  Making the vars static enables folding the zero initialization.

I don't think we should make testsuite changes to paper over bugs
elsewhere.

Richard.

> The linker problem was filled as binutils PR 12564 [1] against ld 2.21.
>
> [1] http://sourceware.org/bugzilla/show_bug.cgi?id=12564
>
> Uros.
>
Uros Bizjak - March 10, 2011, 3:48 p.m.
On Thu, Mar 10, 2011 at 3:59 PM, Richard Guenther
<richard.guenther@gmail.com> wrote:

>> <richard.guenther@gmail.com> wrote:
>>
>>>> Using binutils-2.21, a couple of
>>>> gcc.c-torture/execute/builtins/__-chk.c testcases fail on
>>>> alphaev68-pc-linux-gnu (-lto) with:
>>>>
>>>> /usr/lib/gcc/alpha-unknown-linux-gnu/4.4.5/../../../../alpha-unknown-linux-gnu/bin/ld:
>>>> Warning: alignment 8 of symbol `buf5' in
>>>> /tmp/ccgnDykf.ltrans1.ltrans.o is smaller than 16 in
>>>> /tmp/ccc3QsSw.o.ironly
>>>> /usr/lib/gcc/alpha-unknown-linux-gnu/4.4.5/../../../../alpha-unknown-linux-gnu/bin/ld:
>>>> Warning: alignment 8 of symbol `buf7' in
>>>> /tmp/ccgnDykf.ltrans1.ltrans.o is smaller than 16 in
>>>> /tmp/ccc3QsSw.o.ironly
>>>> /usr/lib/gcc/alpha-unknown-linux-gnu/4.4.5/../../../../alpha-unknown-linux-gnu/bin/ld:
>>>> Warning: alignment 8 of symbol `buf1' in
>>>> /tmp/ccgnDykf.ltrans1.ltrans.o is smaller than 16 in
>>>> /tmp/ccc3QsSw.o.ironly
>>>>
>>>> Attached patch fixes these failures.
>>>
>>> I think this needs more investigation as there are no conflicting
>>> definitions of those vars that would warrant this kind of diagnostic from ld.
>>>
>>> We probably bring the vars local by making them hidden and distribute
>>> them to multiple ltrans units (with only one definiton obviously and multiple
>>> externs).
>>
>> Jan claims that this problem is a linker bug, so perhaps the patch is
>> still suitable for the gcc testsuite. The patch doesn't alter tests in
>> any way, while it avoids false positives due to linker bugs.
>
> Are you sure?  Making the vars static enables folding the zero initialization.

I was looking at other similar testcases (snprintf-chk.c,
vsprintf-chk.c), where uninitialized buffer is declared as static (and
it didn't fail lto tests). Anyway, let's ask the author of the test
(CC'd).

> I don't think we should make testsuite changes to paper over bugs
> elsewhere.

How far do we want to reach in case the test uncovers the problem in
(sort of...) unrelated product?

Uros.
Jakub Jelinek - March 10, 2011, 3:57 p.m.
On Thu, Mar 10, 2011 at 04:48:48PM +0100, Uros Bizjak wrote:
> > Are you sure?  Making the vars static enables folding the zero initialization.
> 
> I was looking at other similar testcases (snprintf-chk.c,
> vsprintf-chk.c), where uninitialized buffer is declared as static (and
> it didn't fail lto tests). Anyway, let's ask the author of the test
> (CC'd).

The tests weren't written with LTO in mind, after all LTO wasn't supported
by GCC at that point.  But I agree with Richard, we shouldn't working around
buggy ld in the gcc testsuite, it is good to know that you have a buggy
linker...

	Jakub
Uros Bizjak - March 10, 2011, 4:06 p.m.
On Thu, Mar 10, 2011 at 4:57 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Mar 10, 2011 at 04:48:48PM +0100, Uros Bizjak wrote:
>> > Are you sure?  Making the vars static enables folding the zero initialization.
>>
>> I was looking at other similar testcases (snprintf-chk.c,
>> vsprintf-chk.c), where uninitialized buffer is declared as static (and
>> it didn't fail lto tests). Anyway, let's ask the author of the test
>> (CC'd).
>
> The tests weren't written with LTO in mind, after all LTO wasn't supported
> by GCC at that point.  But I agree with Richard, we shouldn't working around
> buggy ld in the gcc testsuite, it is good to know that you have a buggy
> linker...

AFAICS, the linker is not buggy, resulting executables still work OK.

But I agree, and won't push this minor issue any further.

Thanks,
Uros.

Patch

Index: gcc.c-torture/execute/builtins/memcpy-chk.c
===================================================================
--- gcc.c-torture/execute/builtins/memcpy-chk.c	(revision 170823)
+++ gcc.c-torture/execute/builtins/memcpy-chk.c	(working copy)
@@ -78,10 +78,10 @@ 
     abort ();
 }
 
-long buf1[64];
+static long buf1[64];
 char *buf2 = (char *) (buf1 + 32);
-long buf5[20];
-char buf7[20];
+static long buf5[20];
+static char buf7[20];
 
 void
 __attribute__((noinline))
Index: gcc.c-torture/execute/builtins/memmove-chk.c
===================================================================
--- gcc.c-torture/execute/builtins/memmove-chk.c	(revision 170823)
+++ gcc.c-torture/execute/builtins/memmove-chk.c	(working copy)
@@ -81,10 +81,10 @@ 
     abort ();
 }
 
-long buf1[64];
+static long buf1[64];
 char *buf2 = (char *) (buf1 + 32);
-long buf5[20];
-char buf7[20];
+static long buf5[20];
+static char buf7[20];
 
 void
 __attribute__((noinline))
Index: gcc.c-torture/execute/builtins/mempcpy-chk.c
===================================================================
--- gcc.c-torture/execute/builtins/mempcpy-chk.c	(revision 170823)
+++ gcc.c-torture/execute/builtins/mempcpy-chk.c	(working copy)
@@ -84,10 +84,10 @@ 
   mempcpy_disallowed = 0;
 }
 
-long buf1[64];
+static long buf1[64];
 char *buf2 = (char *) (buf1 + 32);
-long buf5[20];
-char buf7[20];
+static long buf5[20];
+static char buf7[20];
 
 void
 __attribute__((noinline))