Ignore thunks in ThreadSanitizer pass

[dvyukov, 2011-12-16 14:54:30]

This is for google-main branch.
Ignore thunks in ThreadSanitizer pass.

Index: gcc/tree-tsan.c
===================================================================
--- gcc/tree-tsan.c	(revision 182401)
+++ gcc/tree-tsan.c	(working copy)
@@ -426,6 +426,11 @@
       ignore_init = 1;
     }
 
+  /* Must be some artificial thunk function.  */
+  if (cfun->is_thunk
+      || (DECL_ARTIFICIAL (cfun->decl) && DECL_IGNORED_P (cfun->decl)))
+    return tsan_ignore_func;
+
   src_name = expand_location (cfun->function_start_locus).file;
   if (src_name == NULL)
     src_name = "";
Index: gcc/ChangeLog.google-main
===================================================================
--- gcc/ChangeLog.google-main	(revision 182401)
+++ gcc/ChangeLog.google-main	(working copy)
@@ -1,3 +1,8 @@
+2011-12-16   Dmitriy Vyukov  <dvyukov@google.com>
+
+        Ignore thunks in ThreadSanitizer pass.
+        * gcc/tree-tsan.c (tsan_ignore): Add check for thunks.
+
 2011-12-16  Dehao Chen  <dehao@google.com>
 
 	* ipa-inline-transform.c (dump_inline_decision): New function.

--
This patch is available for review at http://codereview.appspot.com/5492055

[Diego Novillo, 2011-12-16 16:00:06]

On 11-12-16 09:54 , Dmitriy Vyukov wrote:
> This is for google-main branch.
> Ignore thunks in ThreadSanitizer pass.
>
> Index: gcc/tree-tsan.c
> ===================================================================
> --- gcc/tree-tsan.c	(revision 182401)
> +++ gcc/tree-tsan.c	(working copy)
> @@ -426,6 +426,11 @@
>         ignore_init = 1;
>       }
>
> +  /* Must be some artificial thunk function.  */
> +  if (cfun->is_thunk
> +      || (DECL_ARTIFICIAL (cfun->decl)&&  DECL_IGNORED_P (cfun->decl)))
> +    return tsan_ignore_func;

Aren't thunks already DECL_ARTIFICIAL && DECL_IGNORED_P?

> +
>     src_name = expand_location (cfun->function_start_locus).file;
>     if (src_name == NULL)
>       src_name = "";
> Index: gcc/ChangeLog.google-main
> ===================================================================
> --- gcc/ChangeLog.google-main	(revision 182401)
> +++ gcc/ChangeLog.google-main	(working copy)
> @@ -1,3 +1,8 @@
> +2011-12-16   Dmitriy Vyukov<dvyukov@google.com>
> +
> +        Ignore thunks in ThreadSanitizer pass.

Not needed.  Just the description of the change to tree-tsan.c is enough.

> +        * gcc/tree-tsan.c (tsan_ignore): Add check for thunks.

File name should be relative to the directory where the ChangeLog file 
is located.  So, 'gcc/' is not needed.


Diego.

[dvyukov, 2011-12-16 17:35:02]

On 2011/12/16 16:00:06, Diego Novillo wrote:
> On 11-12-16 09:54 , Dmitriy Vyukov wrote:
> > This is for google-main branch.
> > Ignore thunks in ThreadSanitizer pass.
> >
> > Index: gcc/tree-tsan.c
> > ===================================================================
> > --- gcc/tree-tsan.c	(revision 182401)
> > +++ gcc/tree-tsan.c	(working copy)
> > @@ -426,6 +426,11 @@
> >         ignore_init = 1;
> >       }
> >
> > +  /* Must be some artificial thunk function.  */
> > +  if (cfun->is_thunk
> > +      || (DECL_ARTIFICIAL (cfun->decl)&&  DECL_IGNORED_P (cfun->decl)))
> > +    return tsan_ignore_func;
> 
> Aren't thunks already DECL_ARTIFICIAL && DECL_IGNORED_P?

I am not sure. On one path assemble_thunk () sets is_thunk and on another
artificial and ignored, but never both. However, when it sets is_thunk, it calls
targetm.asm_out.output_mi_thunk (), so as far as I understand my pass won't be
executed for the function (backend directly generates specialized asm for it,
right?):
http://gcc.gnu.org/viewcvs/trunk/gcc/cgraphunit.c?view=markup
So I am removing the is_thunk check.


> 
> > +
> >     src_name = expand_location (cfun->function_start_locus).file;
> >     if (src_name == NULL)
> >       src_name = "";
> > Index: gcc/ChangeLog.google-main
> > ===================================================================
> > --- gcc/ChangeLog.google-main	(revision 182401)
> > +++ gcc/ChangeLog.google-main	(working copy)
> > @@ -1,3 +1,8 @@
> > +2011-12-16   Dmitriy mailto:Vyukov<dvyukov@google.com>
> > +
> > +        Ignore thunks in ThreadSanitizer pass.
> 
> Not needed.  Just the description of the change to tree-tsan.c is enough.

Done.

> 
> > +        * gcc/tree-tsan.c (tsan_ignore): Add check for thunks.
> 
> File name should be relative to the directory where the ChangeLog file 
> is located.  So, 'gcc/' is not needed.

Done.

[dvyukov, 2011-12-16 17:36:44]

This is for google-main branch.
Ignore thunks in ThreadSanitizer pass.

Index: gcc/tree-tsan.c
===================================================================
--- gcc/tree-tsan.c	(revision 182401)
+++ gcc/tree-tsan.c	(working copy)
@@ -426,6 +426,10 @@
       ignore_init = 1;
     }
 
+  /* Must be some artificial thunk function.  */
+  if (DECL_ARTIFICIAL (cfun->decl) && DECL_IGNORED_P (cfun->decl))
+    return tsan_ignore_func;
+
   src_name = expand_location (cfun->function_start_locus).file;
   if (src_name == NULL)
     src_name = "";
Index: gcc/ChangeLog.google-main
===================================================================
--- gcc/ChangeLog.google-main	(revision 182401)
+++ gcc/ChangeLog.google-main	(working copy)
@@ -1,3 +1,7 @@
+2011-12-16   Dmitriy Vyukov  <dvyukov@google.com>
+
+        * tree-tsan.c (tsan_ignore): Add check for thunks.
+
 2011-12-16  Dehao Chen  <dehao@google.com>
 
 	* ipa-inline-transform.c (dump_inline_decision): New function.

--
This patch is available for review at http://codereview.appspot.com/5492055

[Diego Novillo, 2011-12-16 21:29:57]

On 11-12-16 12:35 , dvyukov@google.com wrote:
> On 2011/12/16 16:00:06, Diego Novillo wrote:
>> On 11-12-16 09:54 , Dmitriy Vyukov wrote:
>> > This is for google-main branch.
>> > Ignore thunks in ThreadSanitizer pass.
>> >
>> > Index: gcc/tree-tsan.c
>> > ===================================================================
>> > --- gcc/tree-tsan.c (revision 182401)
>> > +++ gcc/tree-tsan.c (working copy)
>> > @@ -426,6 +426,11 @@
>> > ignore_init = 1;
>> > }
>> >
>> > + /* Must be some artificial thunk function. */
>> > + if (cfun->is_thunk
>> > + || (DECL_ARTIFICIAL (cfun->decl)&& DECL_IGNORED_P
> (cfun->decl)))
>> > + return tsan_ignore_func;
>
>> Aren't thunks already DECL_ARTIFICIAL && DECL_IGNORED_P?
>
> I am not sure. On one path assemble_thunk () sets is_thunk and on
> another artificial and ignored, but never both. However, when it sets
> is_thunk, it calls targetm.asm_out.output_mi_thunk (), so as far as I
> understand my pass won't be executed for the function (backend directly
> generates specialized asm for it, right?):

Yes, but your pass may still see it.

> http://gcc.gnu.org/viewcvs/trunk/gcc/cgraphunit.c?view=markup
> So I am removing the is_thunk check.

OK, if you don't need the check, that's fine.  You can always add it 
back, if needed.


Diego.

[dvyukov, 2011-12-17 11:53:39]

On 2011/12/16 21:29:57, Diego Novillo wrote:
> On 11-12-16 12:35 , mailto:dvyukov@google.com wrote:
> > On 2011/12/16 16:00:06, Diego Novillo wrote:
> >> On 11-12-16 09:54 , Dmitriy Vyukov wrote:
> >> > This is for google-main branch.
> >> > Ignore thunks in ThreadSanitizer pass.
> >> >
> >> > Index: gcc/tree-tsan.c
> >> > ===================================================================
> >> > --- gcc/tree-tsan.c (revision 182401)
> >> > +++ gcc/tree-tsan.c (working copy)
> >> > @@ -426,6 +426,11 @@
> >> > ignore_init = 1;
> >> > }
> >> >
> >> > + /* Must be some artificial thunk function. */
> >> > + if (cfun->is_thunk
> >> > + || (DECL_ARTIFICIAL (cfun->decl)&& DECL_IGNORED_P
> > (cfun->decl)))
> >> > + return tsan_ignore_func;
> >
> >> Aren't thunks already DECL_ARTIFICIAL && DECL_IGNORED_P?
> >
> > I am not sure. On one path assemble_thunk () sets is_thunk and on
> > another artificial and ignored, but never both. However, when it sets
> > is_thunk, it calls targetm.asm_out.output_mi_thunk (), so as far as I
> > understand my pass won't be executed for the function (backend directly
> > generates specialized asm for it, right?):
> 
> Yes, but your pass may still see it.

Humm.. but that branch does not even generate GIMPLE representation for the
function...

> > http://gcc.gnu.org/viewcvs/trunk/gcc/cgraphunit.c?view=markup
> > So I am removing the is_thunk check.
> 
> OK, if you don't need the check, that's fine.  You can always add it 
> back, if needed.

Yes, for that particular function on Linux/amd64 I see artificial+ignored and
is_thunk==0.

So is it LGTY?

[Diego Novillo, 2011-12-17 14:02:34]

On Sat, Dec 17, 2011 at 06:53,  <dvyukov@google.com> wrote:

> Yes, for that particular function on Linux/amd64 I see
> artificial+ignored and is_thunk==0.
>
> So is it LGTY?

Yes.


Diego.