[PATCH, CHKP] Fix ipa-comdats for instrumentation thunks

[PATCH, CHKP] Fix ipa-comdats for instrumentation thunks

[Ilya Enkovich]

Hi,

With r221574 (https://gcc.gnu.org/ml/gcc-cvs/2015-03/msg00495.html) thunks don't get comdat groups assigned and this causes a failure in cgraph checker for instrumentation thunks.  It happens because instrumentation thunk may reference local symbol in comdat not being in comdat by itself.  This patch fixes the problem.  Doesn't affect not instrumented code.  Testing is in progress.  Does it look OK?

Thanks,
Ilya
--
gcc/

2015-04-02  Ilya Enkovich  <ilya.enkovich@intel.com>

	* ipa-comdats.c (ipa_comdats): Visit all instrumentation
	thunks to set proper comdat group.

gcc/testsuite/

2015-04-02  Ilya Enkovich  <ilya.enkovich@intel.com>

	* gcc.target/i386/mpx/chkp-thunk-comdat-1.cc: New.
	* gcc.target/i386/mpx/chkp-thunk-comdat-2.cc: New.


diff --git a/gcc/ipa-comdats.c b/gcc/ipa-comdats.c
index f349f9f..30bcad8 100644
--- a/gcc/ipa-comdats.c
+++ b/gcc/ipa-comdats.c
@@ -348,10 +348,9 @@ ipa_comdats (void)
     }
 
   /* Finally assign symbols to the sections.  */
-
+  cgraph_node *fun;
   FOR_EACH_DEFINED_SYMBOL (symbol)
     {
-      struct cgraph_node *fun;
       symbol->aux = NULL; 
       if (!symbol->get_comdat_group ()
 	  && !symbol->alias
@@ -388,6 +387,20 @@ ipa_comdats (void)
 		   true);
 	}
     }
+
+  /* Instrumentation thunks reference original node and thus
+     need to be in the same comdat group.  Otherwise we may
+     get a local instrumented symbol in a comdat group and
+     the referencing original node outside of it.  */
+  FOR_EACH_DEFINED_FUNCTION (fun)
+    if (fun->instrumentation_clone
+	&& fun->instrumented_version
+	&& !fun->instrumented_version->alias
+	&& fun->get_comdat_group ()
+	&& !fun->instrumented_version->get_comdat_group ())
+      fun->instrumented_version->call_for_symbol_and_aliases
+	(set_comdat_group_1, fun, true);
+
   return 0;
 }
 
diff --git a/gcc/testsuite/gcc.target/i386/mpx/chkp-thunk-comdat-1.cc b/gcc/testsuite/gcc.target/i386/mpx/chkp-thunk-comdat-1.cc
new file mode 100644
index 0000000..26d3c48
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/mpx/chkp-thunk-comdat-1.cc
@@ -0,0 +1,18 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fcheck-pointer-bounds -mmpx" } */
+
+namespace
+{
+  template <int dim>
+  int __attribute__((noinline))
+  f1 ()
+  {
+    return dim;
+  }
+}
+
+int
+test ()
+{
+  return f1<3> ();
+}
diff --git a/gcc/testsuite/gcc.target/i386/mpx/chkp-thunk-comdat-2.cc b/gcc/testsuite/gcc.target/i386/mpx/chkp-thunk-comdat-2.cc
new file mode 100644
index 0000000..2b1abe9
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/mpx/chkp-thunk-comdat-2.cc
@@ -0,0 +1,26 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fcheck-pointer-bounds -mmpx" } */
+
+class c1
+{
+public:
+  virtual int test1 (const char *);
+};
+
+class c2
+{
+public:
+  int test2 (const char *);
+};
+
+int
+c1::test1 (const char *)
+{
+  return 0;
+}
+
+int
+c2::test2 (const char *)
+{
+  return 0;
+}

Re: [PATCH, CHKP] Fix ipa-comdats for instrumentation thunks

[Jan Hubicka]

> Hi,
> 
> With r221574 (https://gcc.gnu.org/ml/gcc-cvs/2015-03/msg00495.html) thunks don't get comdat groups assigned and this causes a failure in cgraph checker for instrumentation thunks.  It happens because instrumentation thunk may reference local symbol in comdat not being in comdat by itself.  This patch fixes the problem.  Doesn't affect not instrumented code.  Testing is in progress.  Does it look OK?
> 
> Thanks,
> Ilya
> --
> gcc/
> 
> 2015-04-02  Ilya Enkovich  <ilya.enkovich@intel.com>
> 
> 	* ipa-comdats.c (ipa_comdats): Visit all instrumentation
> 	thunks to set proper comdat group.
> 
> gcc/testsuite/
> 
> 2015-04-02  Ilya Enkovich  <ilya.enkovich@intel.com>
> 
> 	* gcc.target/i386/mpx/chkp-thunk-comdat-1.cc: New.
> 	* gcc.target/i386/mpx/chkp-thunk-comdat-2.cc: New.
> 
> 
> diff --git a/gcc/ipa-comdats.c b/gcc/ipa-comdats.c
> index f349f9f..30bcad8 100644
> --- a/gcc/ipa-comdats.c
> +++ b/gcc/ipa-comdats.c
> @@ -348,10 +348,9 @@ ipa_comdats (void)
>      }
>  
>    /* Finally assign symbols to the sections.  */
> -
> +  cgraph_node *fun;
>    FOR_EACH_DEFINED_SYMBOL (symbol)
>      {
> -      struct cgraph_node *fun;
>        symbol->aux = NULL; 
>        if (!symbol->get_comdat_group ()
>  	  && !symbol->alias
> @@ -388,6 +387,20 @@ ipa_comdats (void)
>  		   true);
>  	}
>      }
> +
> +  /* Instrumentation thunks reference original node and thus
> +     need to be in the same comdat group.  Otherwise we may
> +     get a local instrumented symbol in a comdat group and
> +     the referencing original node outside of it.  */
> +  FOR_EACH_DEFINED_FUNCTION (fun)
> +    if (fun->instrumentation_clone
> +	&& fun->instrumented_version
> +	&& !fun->instrumented_version->alias
> +	&& fun->get_comdat_group ()
> +	&& !fun->instrumented_version->get_comdat_group ())
> +      fun->instrumented_version->call_for_symbol_and_aliases
> +	(set_comdat_group_1, fun, true);

I think you want to handle them same way as the aliases&thunks are handled.
This fix is symptomatic: the code may assign them to different comdat groups
and may propagate that furhter.

Honza

Re: [PATCH, CHKP] Fix ipa-comdats for instrumentation thunks

[Ilya Enkovich]

2015-04-02 20:04 GMT+03:00 Jan Hubicka <hubicka@ucw.cz>:
>> Hi,
>>
>> With r221574 (https://gcc.gnu.org/ml/gcc-cvs/2015-03/msg00495.html) thunks don't get comdat groups assigned and this causes a failure in cgraph checker for instrumentation thunks.  It happens because instrumentation thunk may reference local symbol in comdat not being in comdat by itself.  This patch fixes the problem.  Doesn't affect not instrumented code.  Testing is in progress.  Does it look OK?
>>
>> Thanks,
>> Ilya
>> --
>> gcc/
>>
>> 2015-04-02  Ilya Enkovich  <ilya.enkovich@intel.com>
>>
>>       * ipa-comdats.c (ipa_comdats): Visit all instrumentation
>>       thunks to set proper comdat group.
>>
>> gcc/testsuite/
>>
>> 2015-04-02  Ilya Enkovich  <ilya.enkovich@intel.com>
>>
>>       * gcc.target/i386/mpx/chkp-thunk-comdat-1.cc: New.
>>       * gcc.target/i386/mpx/chkp-thunk-comdat-2.cc: New.
>>
>>
>> diff --git a/gcc/ipa-comdats.c b/gcc/ipa-comdats.c
>> index f349f9f..30bcad8 100644
>> --- a/gcc/ipa-comdats.c
>> +++ b/gcc/ipa-comdats.c
>> @@ -348,10 +348,9 @@ ipa_comdats (void)
>>      }
>>
>>    /* Finally assign symbols to the sections.  */
>> -
>> +  cgraph_node *fun;
>>    FOR_EACH_DEFINED_SYMBOL (symbol)
>>      {
>> -      struct cgraph_node *fun;
>>        symbol->aux = NULL;
>>        if (!symbol->get_comdat_group ()
>>         && !symbol->alias
>> @@ -388,6 +387,20 @@ ipa_comdats (void)
>>                  true);
>>       }
>>      }
>> +
>> +  /* Instrumentation thunks reference original node and thus
>> +     need to be in the same comdat group.  Otherwise we may
>> +     get a local instrumented symbol in a comdat group and
>> +     the referencing original node outside of it.  */
>> +  FOR_EACH_DEFINED_FUNCTION (fun)
>> +    if (fun->instrumentation_clone
>> +     && fun->instrumented_version
>> +     && !fun->instrumented_version->alias
>> +     && fun->get_comdat_group ()
>> +     && !fun->instrumented_version->get_comdat_group ())
>> +      fun->instrumented_version->call_for_symbol_and_aliases
>> +     (set_comdat_group_1, fun, true);
>
> I think you want to handle them same way as the aliases&thunks are handled.
> This fix is symptomatic: the code may assign them to different comdat groups
> and may propagate that furhter.

Currently ipa_comdats doesn't set comdat groups for thunks. At the
same time all references to local symbol should be within one comdat
group. That's why I need this fix. Assigning function and its
instrumentation thunks to different comdat groups would be an error
because both nodes represent the same function.

Thanks,
Ilya

>
> Honza

Re: [PATCH, CHKP] Fix ipa-comdats for instrumentation thunks

[Jan Hubicka]

> 2015-04-02 20:04 GMT+03:00 Jan Hubicka <hubicka@ucw.cz>:
> >> Hi,
> >>
> >> With r221574 (https://gcc.gnu.org/ml/gcc-cvs/2015-03/msg00495.html) thunks don't get comdat groups assigned and this causes a failure in cgraph checker for instrumentation thunks.  It happens because instrumentation thunk may reference local symbol in comdat not being in comdat by itself.  This patch fixes the problem.  Doesn't affect not instrumented code.  Testing is in progress.  Does it look OK?
> >>
> >> Thanks,
> >> Ilya
> >> --
> >> gcc/
> >>
> >> 2015-04-02  Ilya Enkovich  <ilya.enkovich@intel.com>
> >>
> >>       * ipa-comdats.c (ipa_comdats): Visit all instrumentation
> >>       thunks to set proper comdat group.
> >>
> >> gcc/testsuite/
> >>
> >> 2015-04-02  Ilya Enkovich  <ilya.enkovich@intel.com>
> >>
> >>       * gcc.target/i386/mpx/chkp-thunk-comdat-1.cc: New.
> >>       * gcc.target/i386/mpx/chkp-thunk-comdat-2.cc: New.
> >>
> >>
> >> diff --git a/gcc/ipa-comdats.c b/gcc/ipa-comdats.c
> >> index f349f9f..30bcad8 100644
> >> --- a/gcc/ipa-comdats.c
> >> +++ b/gcc/ipa-comdats.c
> >> @@ -348,10 +348,9 @@ ipa_comdats (void)
> >>      }
> >>
> >>    /* Finally assign symbols to the sections.  */
> >> -
> >> +  cgraph_node *fun;
> >>    FOR_EACH_DEFINED_SYMBOL (symbol)
> >>      {
> >> -      struct cgraph_node *fun;
> >>        symbol->aux = NULL;
> >>        if (!symbol->get_comdat_group ()
> >>         && !symbol->alias
> >> @@ -388,6 +387,20 @@ ipa_comdats (void)
> >>                  true);
> >>       }
> >>      }
> >> +
> >> +  /* Instrumentation thunks reference original node and thus
> >> +     need to be in the same comdat group.  Otherwise we may
> >> +     get a local instrumented symbol in a comdat group and
> >> +     the referencing original node outside of it.  */
> >> +  FOR_EACH_DEFINED_FUNCTION (fun)
> >> +    if (fun->instrumentation_clone
> >> +     && fun->instrumented_version
> >> +     && !fun->instrumented_version->alias
> >> +     && fun->get_comdat_group ()
> >> +     && !fun->instrumented_version->get_comdat_group ())
> >> +      fun->instrumented_version->call_for_symbol_and_aliases
> >> +     (set_comdat_group_1, fun, true);
> >
> > I think you want to handle them same way as the aliases&thunks are handled.
> > This fix is symptomatic: the code may assign them to different comdat groups
> > and may propagate that furhter.
> 
> Currently ipa_comdats doesn't set comdat groups for thunks. At the

I see, that is a bug.  It is supposed to keep thunks in the same section
as their target (thunks doesn't really work across sections on some target,
like PPC, because there is no way to produce a tailcall)
Does the following fix the problem?
Index: ipa-comdats.c
===================================================================
--- ipa-comdats.c       (revision 221857)
+++ ipa-comdats.c       (working copy)
@@ -377,12 +377,12 @@
              fprintf (dump_file, "To group: %s\n", IDENTIFIER_POINTER (group));
            }
          if (is_a <cgraph_node *> (symbol))
-          dyn_cast <cgraph_node *>(symbol)->call_for_symbol_and_aliases
+          dyn_cast <cgraph_node *>(symbol)->call_for_symbol_thunks_and_aliases
                  (set_comdat_group_1,
                   *comdat_head_map.get (group),
                   true);
          else
-          symbol->call_for_symbol_and_aliases
+          symbol->call_for_symbol_thunks_and_aliases
                  (set_comdat_group,
                   *comdat_head_map.get (group),
                   true);

> same time all references to local symbol should be within one comdat
> group. That's why I need this fix. Assigning function and its
> instrumentation thunks to different comdat groups would be an error
> because both nodes represent the same function.
> 
> Thanks,
> Ilya
> 
> >
> > Honza

Re: [PATCH, CHKP] Fix ipa-comdats for instrumentation thunks

[Ilya Enkovich]

2015-04-03 20:12 GMT+03:00 Jan Hubicka <hubicka@ucw.cz>:
>>
>> Currently ipa_comdats doesn't set comdat groups for thunks. At the
>
> I see, that is a bug.  It is supposed to keep thunks in the same section
> as their target (thunks doesn't really work across sections on some target,
> like PPC, because there is no way to produce a tailcall)
> Does the following fix the problem?

I believe this should help.  Will try it.

Thanks,
Ilya

> Index: ipa-comdats.c
> ===================================================================
> --- ipa-comdats.c       (revision 221857)
> +++ ipa-comdats.c       (working copy)
> @@ -377,12 +377,12 @@
>               fprintf (dump_file, "To group: %s\n", IDENTIFIER_POINTER (group));
>             }
>           if (is_a <cgraph_node *> (symbol))
> -          dyn_cast <cgraph_node *>(symbol)->call_for_symbol_and_aliases
> +          dyn_cast <cgraph_node *>(symbol)->call_for_symbol_thunks_and_aliases
>                   (set_comdat_group_1,
>                    *comdat_head_map.get (group),
>                    true);
>           else
> -          symbol->call_for_symbol_and_aliases
> +          symbol->call_for_symbol_thunks_and_aliases
>                   (set_comdat_group,
>                    *comdat_head_map.get (group),
>                    true);
>

Re: [PATCH, CHKP] Fix ipa-comdats for instrumentation thunks

[Jan Hubicka]

> 2015-04-03 20:12 GMT+03:00 Jan Hubicka <hubicka@ucw.cz>:
> >>
> >> Currently ipa_comdats doesn't set comdat groups for thunks. At the
> >
> > I see, that is a bug.  It is supposed to keep thunks in the same section
> > as their target (thunks doesn't really work across sections on some target,
> > like PPC, because there is no way to produce a tailcall)
> > Does the following fix the problem?
> 
> I believe this should help.  Will try it.
If it passes testing, the patch is preapproved.

Thanks!
Honza
> 
> Thanks,
> Ilya
> 
> > Index: ipa-comdats.c
> > ===================================================================
> > --- ipa-comdats.c       (revision 221857)
> > +++ ipa-comdats.c       (working copy)
> > @@ -377,12 +377,12 @@
> >               fprintf (dump_file, "To group: %s\n", IDENTIFIER_POINTER (group));
> >             }
> >           if (is_a <cgraph_node *> (symbol))
> > -          dyn_cast <cgraph_node *>(symbol)->call_for_symbol_and_aliases
> > +          dyn_cast <cgraph_node *>(symbol)->call_for_symbol_thunks_and_aliases
> >                   (set_comdat_group_1,
> >                    *comdat_head_map.get (group),
> >                    true);
> >           else
> > -          symbol->call_for_symbol_and_aliases
> > +          symbol->call_for_symbol_thunks_and_aliases
> >                   (set_comdat_group,
> >                    *comdat_head_map.get (group),
> >                    true);
> >

Re: [PATCH, CHKP] Fix ipa-comdats for instrumentation thunks

[Ilya Enkovich]

On 06 Apr 20:41, Jan Hubicka wrote:
> > 2015-04-03 20:12 GMT+03:00 Jan Hubicka <hubicka@ucw.cz>:
> > >>
> > >> Currently ipa_comdats doesn't set comdat groups for thunks. At the
> > >
> > > I see, that is a bug.  It is supposed to keep thunks in the same section
> > > as their target (thunks doesn't really work across sections on some target,
> > > like PPC, because there is no way to produce a tailcall)
> > > Does the following fix the problem?
> > 
> > I believe this should help.  Will try it.
> If it passes testing, the patch is preapproved.
> 
> Thanks!
> Honza

Here is a committed version.

Thanks,
Ilya
--
diff --git a/gcc/ipa-comdats.c b/gcc/ipa-comdats.c
index f349f9f..e24359c 100644
--- a/gcc/ipa-comdats.c
+++ b/gcc/ipa-comdats.c
@@ -377,7 +377,7 @@ ipa_comdats (void)
 	      fprintf (dump_file, "To group: %s\n", IDENTIFIER_POINTER (group));
 	    }
 	  if (is_a <cgraph_node *> (symbol))
-	   dyn_cast <cgraph_node *>(symbol)->call_for_symbol_and_aliases
+	   dyn_cast <cgraph_node *>(symbol)->call_for_symbol_thunks_and_aliases
 		  (set_comdat_group_1,
 		   *comdat_head_map.get (group),
 		   true);
diff --git a/gcc/testsuite/gcc.target/i386/mpx/chkp-thunk-comdat-1.cc b/gcc/testsuite/gcc.target/i386/mpx/chkp-thunk-comdat-1.cc
new file mode 100644
index 0000000..26d3c48
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/mpx/chkp-thunk-comdat-1.cc
@@ -0,0 +1,18 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fcheck-pointer-bounds -mmpx" } */
+
+namespace
+{
+  template <int dim>
+  int __attribute__((noinline))
+  f1 ()
+  {
+    return dim;
+  }
+}
+
+int
+test ()
+{
+  return f1<3> ();
+}
diff --git a/gcc/testsuite/gcc.target/i386/mpx/chkp-thunk-comdat-2.cc b/gcc/testsuite/gcc.target/i386/mpx/chkp-thunk-comdat-2.cc
new file mode 100644
index 0000000..2b1abe9
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/mpx/chkp-thunk-comdat-2.cc
@@ -0,0 +1,26 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fcheck-pointer-bounds -mmpx" } */
+
+class c1
+{
+public:
+  virtual int test1 (const char *);
+};
+
+class c2
+{
+public:
+  int test2 (const char *);
+};
+
+int
+c1::test1 (const char *)
+{
+  return 0;
+}
+
+int
+c2::test2 (const char *)
+{
+  return 0;
+}