Message ID | 1351616337.4778.7.camel@otta.rchland.ibm.com |
---|---|
State | New |
Headers | show |
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
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
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
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
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
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 > > >
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
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
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
--- 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