diff mbox

[4.7] Backport fix for PR tree-optimization/53708

Message ID 1351616337.4778.7.camel@otta.rchland.ibm.com
State New
Headers show

Commit Message

Peter Bergner Oct. 30, 2012, 4:58 p.m. UTC 
I'm hitting the same bug as in PR53708 when compiling GLIBC's dlfcn.c when
vectorization is enabled on powerpc64-linux.  A reduced test case is:

bergner@bns:~/gcc/BUGS> cat foo.i 
static void (*const init_array []) (void)
  __attribute__ ((section (".init_array"), aligned (sizeof (void *)), used))
= { 0 };

bergner@bns:~/gcc/BUGS> /home/bergner/gcc/build/gcc-fsf-4_7-base/gcc/xgcc
-B/home/bergner/gcc/build/gcc-fsf-4_7-base/gcc -S -m64 -O3 -maltivec foo.i -o
bad.s

bergner@bns:~/gcc/BUGS> /home/bergner/gcc/build/gcc-fsf-4_7-pr53708/gcc/xgcc
-B/home/bergner/gcc/build/gcc-fsf-4_7-pr53708/gcc -S -m64 -O3 -maltivec foo.i
-o good.s

bergner@bns:~/gcc/BUGS> diff -u bad.s good.s

Comments

Peter Bergner Oct. 30, 2012, 6:43 p.m. UTC | #1 
On Tue, 2012-10-30 at 11:58 -0500, Peter Bergner wrote:
> I'm hitting the same bug as in PR53708 when compiling GLIBC's dlfcn.c when
> vectorization is enabled on powerpc64-linux.  A reduced test case is:
> 
> bergner@bns:~/gcc/BUGS> cat foo.i 
> static void (*const init_array []) (void)
>   __attribute__ ((section (".init_array"), aligned (sizeof (void *)), used))
> = { 0 };
> 
> bergner@bns:~/gcc/BUGS> /home/bergner/gcc/build/gcc-fsf-4_7-base/gcc/xgcc
> -B/home/bergner/gcc/build/gcc-fsf-4_7-base/gcc -S -m64 -O3 -maltivec foo.i -o
> bad.s
> 
> bergner@bns:~/gcc/BUGS> /home/bergner/gcc/build/gcc-fsf-4_7-pr53708/gcc/xgcc
> -B/home/bergner/gcc/build/gcc-fsf-4_7-pr53708/gcc -S -m64 -O3 -maltivec foo.i
> -o good.s
> 
> bergner@bns:~/gcc/BUGS> diff -u bad.s good.s 
> --- bad.s    2012-10-30 10:41:15.000000000 -0500
> +++ good.s    2012-10-30 10:41:23.000000000 -0500
> @@ -2,7 +2,7 @@
>      .section    ".toc","aw"
>      .section    ".text"
>      .section    .init_array,"a"
> -    .align 4
> +    .align 3
>      .type    init_array, @object
>      .size    init_array, 8
>  init_array:
> 
> The above is bad, because the extra alignment causes the linker to add some
> null padding to the init_array and the loader isn't expecting that and ends
> up segv'ing.  I'd like to backport Richard's patch below to the 4.7 branch.
> The patch bootstrapped and regtested on powerpc64-linux with no regressions.

Commenting on Richard's question from the bugzilla:

  http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53708#c10

I suppose if attribute((__aligned__)) truly does just set a minimum alignment
value (and the documentation seems to say that) and the compiler is free to
arbitrarily increase it, then the GLIBC code to scan the init_array needs to
be tolerant of null values in init_array.

Does everyone agree with that assessment?

Peter
Jakub Jelinek Oct. 30, 2012, 6:55 p.m. UTC | #2 
On Tue, Oct 30, 2012 at 01:43:33PM -0500, Peter Bergner wrote:
> Commenting on Richard's question from the bugzilla:
> 
>   http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53708#c10
> 
> I suppose if attribute((__aligned__)) truly does just set a minimum alignment
> value (and the documentation seems to say that) and the compiler is free to
> arbitrarily increase it, then the GLIBC code to scan the init_array needs to
> be tolerant of null values in init_array.
> 
> Does everyone agree with that assessment?

I think the right test is for user supplied section name,
this approach is so common that I think we have to support it.
E.g. Linux kernel, glibc, prelink all use this, gcc itself too (e.g.
crtstuff.c).

	Jakub
Peter Bergner Oct. 30, 2012, 7:03 p.m. UTC | #3 
On Tue, 2012-10-30 at 19:55 +0100, Jakub Jelinek wrote:
> On Tue, Oct 30, 2012 at 01:43:33PM -0500, Peter Bergner wrote:
> > Commenting on Richard's question from the bugzilla:
> > 
> >   http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53708#c10
> > 
> > I suppose if attribute((__aligned__)) truly does just set a minimum alignment
> > value (and the documentation seems to say that) and the compiler is free to
> > arbitrarily increase it, then the GLIBC code to scan the init_array needs to
> > be tolerant of null values in init_array.
> > 
> > Does everyone agree with that assessment?
> 
> I think the right test is for user supplied section name,
> this approach is so common that I think we have to support it.
> E.g. Linux kernel, glibc, prelink all use this, gcc itself too (e.g.
> crtstuff.c).

Ok, then I'll bootstrap and regtest your suggested change while we
wait for richi to comment.  I'm fine with whatever you and richi
decide is best.  The ObjC guys should probably test it though too.

I assume you think we should change the current trunk code as well?

Peter
Jakub Jelinek Oct. 30, 2012, 7:37 p.m. UTC | #4 
On Tue, Oct 30, 2012 at 02:03:44PM -0500, Peter Bergner wrote:
> Ok, then I'll bootstrap and regtest your suggested change while we
> wait for richi to comment.  I'm fine with whatever you and richi
> decide is best.  The ObjC guys should probably test it though too.
> 
> I assume you think we should change the current trunk code as well?

Well, I haven't looked at the ObjC failures (guess they are darwin only
anyway), so have no idea whether those set section name or not.
So, if my proposed test instead of the trunk one doesn't work for darwin,
perhaps it could be DECL_PRESERVED_P (decl) || (DECL_SECTION_NAME (decl) &&
!...).  I think DECL_USER_ALIGN test is undesirable for that though, it is
just fine to increase alignment of anything, after all, it is still aligned
properly then.

	Jakub
Peter Bergner Oct. 30, 2012, 8:23 p.m. UTC | #5 
On Tue, 2012-10-30 at 20:37 +0100, Jakub Jelinek wrote:
> On Tue, Oct 30, 2012 at 02:03:44PM -0500, Peter Bergner wrote:
> > Ok, then I'll bootstrap and regtest your suggested change while we
> > wait for richi to comment.  I'm fine with whatever you and richi
> > decide is best.  The ObjC guys should probably test it though too.
> > 
> > I assume you think we should change the current trunk code as well?
> 
> Well, I haven't looked at the ObjC failures (guess they are darwin only
> anyway), so have no idea whether those set section name or not.
> So, if my proposed test instead of the trunk one doesn't work for darwin,
> perhaps it could be DECL_PRESERVED_P (decl) || (DECL_SECTION_NAME (decl) &&
> !...).  I think DECL_USER_ALIGN test is undesirable for that though, it is
> just fine to increase alignment of anything, after all, it is still aligned
> properly then.

I can confirm it bootstraps and regtests without any errors...and it
fixes my problem.

Peter
Richard Biener Oct. 31, 2012, 9:12 a.m. UTC | #6 
On Tue, Oct 30, 2012 at 9:23 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> On Tue, 2012-10-30 at 20:37 +0100, Jakub Jelinek wrote:
>> On Tue, Oct 30, 2012 at 02:03:44PM -0500, Peter Bergner wrote:
>> > Ok, then I'll bootstrap and regtest your suggested change while we
>> > wait for richi to comment.  I'm fine with whatever you and richi
>> > decide is best.  The ObjC guys should probably test it though too.
>> >
>> > I assume you think we should change the current trunk code as well?
>>
>> Well, I haven't looked at the ObjC failures (guess they are darwin only
>> anyway), so have no idea whether those set section name or not.
>> So, if my proposed test instead of the trunk one doesn't work for darwin,
>> perhaps it could be DECL_PRESERVED_P (decl) || (DECL_SECTION_NAME (decl) &&
>> !...).  I think DECL_USER_ALIGN test is undesirable for that though, it is
>> just fine to increase alignment of anything, after all, it is still aligned
>> properly then.
>
> I can confirm it bootstraps and regtests without any errors...and it
> fixes my problem.

I'm fine with that, but please give it some time on trunk before backporting it.

Thanks,
Richard.

> Peter
>
>
>
Peter Bergner Oct. 31, 2012, 1:53 p.m. UTC | #7 
On Wed, 2012-10-31 at 10:12 +0100, Richard Biener wrote:
> On Tue, Oct 30, 2012 at 9:23 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> > On Tue, 2012-10-30 at 20:37 +0100, Jakub Jelinek wrote:
> >> On Tue, Oct 30, 2012 at 02:03:44PM -0500, Peter Bergner wrote:
> >> > Ok, then I'll bootstrap and regtest your suggested change while we
> >> > wait for richi to comment.  I'm fine with whatever you and richi
> >> > decide is best.  The ObjC guys should probably test it though too.
> >> >
> >> > I assume you think we should change the current trunk code as well?
> >>
> >> Well, I haven't looked at the ObjC failures (guess they are darwin only
> >> anyway), so have no idea whether those set section name or not.
> >> So, if my proposed test instead of the trunk one doesn't work for darwin,
> >> perhaps it could be DECL_PRESERVED_P (decl) || (DECL_SECTION_NAME (decl) &&
> >> !...).  I think DECL_USER_ALIGN test is undesirable for that though, it is
> >> just fine to increase alignment of anything, after all, it is still aligned
> >> properly then.
> >
> > I can confirm it bootstraps and regtests without any errors...and it
> > fixes my problem.
> 
> I'm fine with that, but please give it some time on trunk before backporting it.

Great.  Jakub, were you going to commit your change or did you want me
to do that?

Peter
Jakub Jelinek Oct. 31, 2012, 1:55 p.m. UTC | #8 
On Wed, Oct 31, 2012 at 08:53:31AM -0500, Peter Bergner wrote:
> Great.  Jakub, were you going to commit your change or did you want me
> to do that?

I haven't tested it, you did, so please do that yourself.  Thanks.

	Jakub
Peter Bergner Oct. 31, 2012, 2:02 p.m. UTC | #9 
On Wed, 2012-10-31 at 14:55 +0100, Jakub Jelinek wrote:
> On Wed, Oct 31, 2012 at 08:53:31AM -0500, Peter Bergner wrote:
> > Great.  Jakub, were you going to commit your change or did you want me
> > to do that?
> 
> I haven't tested it, you did, so please do that yourself.  Thanks.

I tested it on FSF 4.7.  I'll do a quick trunk bootstrap/regtest and
commit it when it passes.  Thanks.

Peter
diff mbox

Patch

--- bad.s    2012-10-30 10:41:15.000000000 -0500
+++ good.s    2012-10-30 10:41:23.000000000 -0500
@@ -2,7 +2,7 @@ 
     .section    ".toc","aw"
     .section    ".text"
     .section    .init_array,"a"
-    .align 4
+    .align 3
     .type    init_array, @object
     .size    init_array, 8
 init_array:

The above is bad, because the extra alignment causes the linker to add some
null padding to the init_array and the loader isn't expecting that and ends
up segv'ing.  I'd like to backport Richard's patch below to the 4.7 branch.
The patch bootstrapped and regtested on powerpc64-linux with no regressions.

Is it ok for the 4.7 branch?

Peter


	Backport from mainline
	2012-06-19  Richard Guenther  <rguenther@suse.de>

	PR tree-optimization/53708
	* tree-vect-data-refs.c (vect_can_force_dr_alignment_p): Preserve
	user-supplied alignment and alignment of decls with the used
	attribute.
 
Index: gcc/tree-vect-data-refs.c
===================================================================
--- gcc/tree-vect-data-refs.c	(revision 192988)
+++ gcc/tree-vect-data-refs.c	(working copy)
@@ -4574,6 +4574,12 @@ 
   if (TREE_ASM_WRITTEN (decl))
     return false;
 
+  /* Do not override explicit alignment set by the user or the alignment
+     as specified by the ABI when the used attribute is set.  */
+  if (DECL_USER_ALIGN (decl)
+      || DECL_PRESERVE_P (decl))
+    return false;
+
   if (TREE_STATIC (decl))
     return (alignment <= MAX_OFILE_ALIGNMENT);
   else