[PATCH, i686] Fix for asan test failures with -m32 happened after EBX enabling in PIC mode

[PATCH, i686] Fix for asan test failures with -m32 happened after EBX enabling in PIC mode

[Zamyatin, Igor]

Hi!

Following patch (moving initialization of pic_offset_table_rtx earlier) fixes failures for asan tests on 32 bits in PIC mode mentioned here - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63534#c48


Bootstrapped/regtested on x86_64, i686

Is it ok for trunk?

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.

diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index 9bd6135..cf7d94a 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -1718,6 +1718,11 @@ expand_used_vars (void)
 
   init_vars_expansion ();
 
+  /* 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);
+
   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 1ef43c4..0bbcbbb 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -3464,11 +3464,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);

Re: [PATCH, i686] Fix for asan test failures with -m32 happened after EBX enabling in PIC mode

[Jeff Law]

On 10/31/14 09:34, Zamyatin, Igor wrote:
> Hi!
>
> Following patch (moving initialization of pic_offset_table_rtx
earlier) fixes failures for asan tests on 32 bits in PIC mode mentioned
here - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63534#c48
>
>
> Bootstrapped/regtested on x86_64, i686
>
> Is it ok for trunk?
>
> 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.

You should also reference the PR in the ChangeLog like this:

	PR target/63634

So get the analysis posted, and I'll likely approve after reviewing the 
background info.

Thanks,
Jeff

RE: [PATCH, i686] Fix for asan test failures with -m32 happened after EBX enabling in PIC mode

[Zamyatin, Igor]

> > Hi!
> >
> > Following patch (moving initialization of pic_offset_table_rtx
> earlier) fixes failures for asan tests on 32 bits in PIC mode mentioned here -
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63534#c48
> >
> >
> > Bootstrapped/regtested on x86_64, i686
> >
> > Is it ok for trunk?
> >
> > 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.

Thanks,
Igor

Re: [PATCH, i686] Fix for asan test failures with -m32 happened after EBX enabling in PIC mode

[H.J. Lu]

On Wed, Nov 5, 2014 at 7:17 AM, Zamyatin, Igor <igor.zamyatin@intel.com> wrote:
>> > Hi!
>> >
>> > Following patch (moving initialization of pic_offset_table_rtx
>> earlier) fixes failures for asan tests on 32 bits in PIC mode mentioned here -
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63534#c48
>> >
>> >
>> > Bootstrapped/regtested on x86_64, i686
>> >
>> > Is it ok for trunk?
>> >
>> > 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.


-- 
H.J.

RE: [PATCH, i686] Fix for asan test failures with -m32 happened after EBX enabling in PIC mode

[Zamyatin, Igor]

> >> >
> >> > 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;
+}
+

Re: [PATCH, i686] Fix for asan test failures with -m32 happened after EBX enabling in PIC mode

[H.J. Lu]

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?


-- 
H.J.

RE: [PATCH, i686] Fix for asan test failures with -m32 happened after EBX enabling in PIC mode

[Zamyatin, Igor]

 
> 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.

Re: [PATCH, i686] Fix for asan test failures with -m32 happened after EBX enabling in PIC mode

[Jakub Jelinek]

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

RE: [PATCH, i686] Fix for asan test failures with -m32 happened after EBX enabling in PIC mode

[Zamyatin, Igor]

> On Fri, Nov 14, 2014 at 05:05:57PM +0000, Zamyatin, Igor wrote:
> 
> 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 } } */

Thanks, updated.


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.dg/asan/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.dg/asan/pr63845.c b/gcc/testsuite/gcc.dg/asan/pr63845.c
new file mode 100644
index 0000000..a3fbe06
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/asan/pr63845.c
@@ -0,0 +1,17 @@
+/* PR sanitizer/63845 */
+/* { dg-do compile } */
+/* { dg-options "-fPIC" { target fpic }  } */
+
+int __attribute__ ((noinline, noclone))
+foo (void *p)
+{
+  return *(int*)p;
+}
+
+int main ()
+{
+  char a = 0;
+  foo (&a);
+  return 0;
+}

Igor

> 
> 	Jakub

Re: [PATCH, i686] Fix for asan test failures with -m32 happened after EBX enabling in PIC mode

[Jeff Law]

On 11/14/14 10:29, Zamyatin, Igor wrote:
>
>> On Fri, Nov 14, 2014 at 05:05:57PM +0000, Zamyatin, Igor wrote:
>>
>> 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 } } */
>
> Thanks, updated.
>
>
> 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.dg/asan/pr63845.c: New test.
Ok.
Jeff