| Message ID | 0EFAB2BDD0F67E4FB6CCC8B9F87D756969C5240E@IRSMSX101.ger.corp.intel.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Nov 14, 2014 at 8:59 AM, Zamyatin, Igor <igor.zamyatin@intel.com> wrote: >> >> > >> >> > ChangeLog: >> >> > >> >> > 2014-10-30 Igor Zamyatin <igor.zamyatin@intel.com> >> >> > >> >> > * function.c (assign_parms): Move init of pic_offset_table_rtx >> >> > from here to... >> >> > * cfgexpand.c (expand_used_vars): ...here. >> >> The patch is probably fine. However, it would be good to have the >> >> analysis why you want to move initialization of the PIC register earlier. >> > >> > Asan (and anybody else can) emits global variable(s) in expand_used_vars >> during function expanding while pic reg is currently initialized later, during >> expand_function_start in assign_parms thus to be late in asan case in PIC >> mode. >> > >> > So to avoid such cases we put pic reg initialization in the beginning of >> expand_used_vars. This seems to be early enough. >> > >> >> Please mention PR in ChangeLog and add a few testcases so that the fix will >> be tested on Linux. >> > > Bootstrapped and regtested on x86_64 and i686 incl pic mode. > Is it ok? > > Thanks, > Igor > > gcc/Changelog: > > 2014-11-14 Igor Zamyatin <igor.zamyatin@intel.com> > > PR sanitizer/63845 > * function.c (assign_parms): Move init of pic_offset_table_rtx > from here to... > * cfgexpand.c (expand_used_vars): ...here. > > gcc/testsuite/Changelog: > > 2014-11-14 Igor Zamyatin <igor.zamyatin@intel.com> > > PR sanitizer/63845 > * gcc.target/i386/pr63845.c: New test. > > > > diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c > index 15d7638..bcd3b35 100644 > --- a/gcc/cfgexpand.c > +++ b/gcc/cfgexpand.c > @@ -1722,6 +1722,9 @@ expand_used_vars (void) > > init_vars_expansion (); > > + if (targetm.use_pseudo_pic_reg ()) > + pic_offset_table_rtx = gen_reg_rtx (Pmode); > + > hash_map<tree, tree> ssa_name_decls; > for (i = 0; i < SA.map->num_partitions; i++) > { > diff --git a/gcc/function.c b/gcc/function.c > index ef98091..97e0b79 100644 > --- a/gcc/function.c > +++ b/gcc/function.c > @@ -3679,11 +3679,6 @@ assign_parms (tree fndecl) > > fnargs.release (); > > - /* Initialize pic_offset_table_rtx with a pseudo register > - if required. */ > - if (targetm.use_pseudo_pic_reg ()) > - pic_offset_table_rtx = gen_reg_rtx (Pmode); > - > /* Output all parameter conversion instructions (possibly including calls) > now that all parameters have been copied out of hard registers. */ > emit_insn (all.first_conversion_insn); > diff --git a/gcc/testsuite/gcc.target/i386/pr63845.c b/gcc/testsuite/gcc.target/i386/pr63845.c > new file mode 100644 > index 0000000..4b675e0 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/i386/pr63845.c > @@ -0,0 +1,20 @@ > +/* PR sanitizer/63845 */ > +/* { dg-do compile } */ > +/* { dg-require-effective-target ia32 } */ > +/* { dg-require-effective-target fpic } */ > +/* { dg-skip-if "No Windows PIC" { *-*-mingw* *-*-cygwin } { "*" } { "" } } */ > +/* { dg-options "-fPIC" } */ > + > +int __attribute__ ((noinline, noclone)) > +foo (void *p) > +{ > + return *(int*)p; > +} > + > +int main () > +{ > + char a = 0; > + foo (&a); > + return 0; > +} > + > Will this test fail on Linux without your fix? Doesn't testcase need -fsanitize=address to fail?
> On Fri, Nov 14, 2014 at 8:59 AM, Zamyatin, Igor <igor.zamyatin@intel.com> > wrote: > >> >> > > >> >> > ChangeLog: > >> >> > > >> >> > 2014-10-30 Igor Zamyatin <igor.zamyatin@intel.com> > >> >> > > >> >> > * function.c (assign_parms): Move init of pic_offset_table_rtx > >> >> > from here to... > >> >> > * cfgexpand.c (expand_used_vars): ...here. > >> >> The patch is probably fine. However, it would be good to have the > >> >> analysis why you want to move initialization of the PIC register earlier. > >> > > >> > Asan (and anybody else can) emits global variable(s) in > >> > expand_used_vars > >> during function expanding while pic reg is currently initialized > >> later, during expand_function_start in assign_parms thus to be late > >> in asan case in PIC mode. > >> > > >> > So to avoid such cases we put pic reg initialization in the > >> > beginning of > >> expand_used_vars. This seems to be early enough. > >> > > >> > >> Please mention PR in ChangeLog and add a few testcases so that the > >> fix will be tested on Linux. > >> > > > > Bootstrapped and regtested on x86_64 and i686 incl pic mode. > > Is it ok? > > > > Thanks, > > Igor > > > > gcc/Changelog: > > > > 2014-11-14 Igor Zamyatin <igor.zamyatin@intel.com> > > > > PR sanitizer/63845 > > * function.c (assign_parms): Move init of pic_offset_table_rtx > > from here to... > > * cfgexpand.c (expand_used_vars): ...here. > > > > gcc/testsuite/Changelog: > > > > 2014-11-14 Igor Zamyatin <igor.zamyatin@intel.com> > > > > PR sanitizer/63845 > > * gcc.target/i386/pr63845.c: New test. > > > > > > > > diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index 15d7638..bcd3b35 > > 100644 > > --- a/gcc/cfgexpand.c > > +++ b/gcc/cfgexpand.c > > @@ -1722,6 +1722,9 @@ expand_used_vars (void) > > > > init_vars_expansion (); > > > > + if (targetm.use_pseudo_pic_reg ()) > > + pic_offset_table_rtx = gen_reg_rtx (Pmode); > > + > > hash_map<tree, tree> ssa_name_decls; > > for (i = 0; i < SA.map->num_partitions; i++) > > { > > diff --git a/gcc/function.c b/gcc/function.c index ef98091..97e0b79 > > 100644 > > --- a/gcc/function.c > > +++ b/gcc/function.c > > @@ -3679,11 +3679,6 @@ assign_parms (tree fndecl) > > > > fnargs.release (); > > > > - /* Initialize pic_offset_table_rtx with a pseudo register > > - if required. */ > > - if (targetm.use_pseudo_pic_reg ()) > > - pic_offset_table_rtx = gen_reg_rtx (Pmode); > > - > > /* Output all parameter conversion instructions (possibly including calls) > > now that all parameters have been copied out of hard registers. */ > > emit_insn (all.first_conversion_insn); diff --git > > a/gcc/testsuite/gcc.target/i386/pr63845.c > > b/gcc/testsuite/gcc.target/i386/pr63845.c > > new file mode 100644 > > index 0000000..4b675e0 > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/i386/pr63845.c > > @@ -0,0 +1,20 @@ > > +/* PR sanitizer/63845 */ > > +/* { dg-do compile } */ > > +/* { dg-require-effective-target ia32 } */ > > +/* { dg-require-effective-target fpic } */ > > +/* { dg-skip-if "No Windows PIC" { *-*-mingw* *-*-cygwin } { "*" } { > > +"" } } */ > > +/* { dg-options "-fPIC" } */ > > + > > +int __attribute__ ((noinline, noclone)) foo (void *p) { > > + return *(int*)p; > > +} > > + > > +int main () > > +{ > > + char a = 0; > > + foo (&a); > > + return 0; > > +} > > + > > > > Will this test fail on Linux without your fix? Doesn't testcase need - > fsanitize=address to fail? Sure, you're right, will add it Thanks, Igor > > > -- > H.J.
On Fri, Nov 14, 2014 at 05:05:57PM +0000, Zamyatin, Igor wrote: > > > --- /dev/null > > > +++ b/gcc/testsuite/gcc.target/i386/pr63845.c > > > @@ -0,0 +1,20 @@ > > > +/* PR sanitizer/63845 */ > > > +/* { dg-do compile } */ > > > +/* { dg-require-effective-target ia32 } */ > > > +/* { dg-require-effective-target fpic } */ > > > +/* { dg-skip-if "No Windows PIC" { *-*-mingw* *-*-cygwin } { "*" } { > > > +"" } } */ > > > +/* { dg-options "-fPIC" } */ > > > + > > > +int __attribute__ ((noinline, noclone)) foo (void *p) { > > > + return *(int*)p; > > > +} > > > + > > > +int main () > > > +{ > > > + char a = 0; > > > + foo (&a); > > > + return 0; > > > +} > > > + > > > > > > > Will this test fail on Linux without your fix? Doesn't testcase need - > > fsanitize=address to fail? > > Sure, you're right, will add it It is not that easy, -fsanitize=address is not supported everywhere. Better if you stick it into testsuite/gcc.dg/asan/ No point adding effective-target ia32/fpic, there is nothing i?86 specific, not even ix86/x86_64 specific, nor pic specific in the test, just use /* { dg-additional-options "-fPIC" { target fpic } } */ Jakub
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index 15d7638..bcd3b35 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -1722,6 +1722,9 @@ expand_used_vars (void) init_vars_expansion (); + if (targetm.use_pseudo_pic_reg ()) + pic_offset_table_rtx = gen_reg_rtx (Pmode); + hash_map<tree, tree> ssa_name_decls; for (i = 0; i < SA.map->num_partitions; i++) { diff --git a/gcc/function.c b/gcc/function.c index ef98091..97e0b79 100644 --- a/gcc/function.c +++ b/gcc/function.c @@ -3679,11 +3679,6 @@ assign_parms (tree fndecl) fnargs.release (); - /* Initialize pic_offset_table_rtx with a pseudo register - if required. */ - if (targetm.use_pseudo_pic_reg ()) - pic_offset_table_rtx = gen_reg_rtx (Pmode); - /* Output all parameter conversion instructions (possibly including calls) now that all parameters have been copied out of hard registers. */ emit_insn (all.first_conversion_insn); diff --git a/gcc/testsuite/gcc.target/i386/pr63845.c b/gcc/testsuite/gcc.target/i386/pr63845.c new file mode 100644 index 0000000..4b675e0 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr63845.c @@ -0,0 +1,20 @@ +/* PR sanitizer/63845 */ +/* { dg-do compile } */ +/* { dg-require-effective-target ia32 } */ +/* { dg-require-effective-target fpic } */ +/* { dg-skip-if "No Windows PIC" { *-*-mingw* *-*-cygwin } { "*" } { "" } } */ +/* { dg-options "-fPIC" } */ + +int __attribute__ ((noinline, noclone)) +foo (void *p) +{ + return *(int*)p; +} + +int main () +{ + char a = 0; + foo (&a); + return 0; +} +