Patchwork PR middle-end/52141: ICE due to asm statement

login
register
mail settings
Submitter Aldy Hernandez
Date Feb. 14, 2012, 4:46 p.m.
Message ID <4F3A8FF4.1070606@redhat.com>
Download mbox | patch
Permalink /patch/141139/
State New
Headers show

Comments

Aldy Hernandez - Feb. 14, 2012, 4:46 p.m.
I swear, these inline asms in transactions are going to be the death of me.

In the attached testcase diagnose_tm_blocks() and lower_tm() do not 
recognize the GIMPLE_ASM because early inlining has not yet run, so 
there is nothing to see.  However, by the time we run the IPA TM pass, 
the assembly statement has been inlined but we no longer detect 
GIMPLE_ASMs correctly:

   /* Now validate all tm_safe functions, and all atomic regions in
      other functions.  */
   for (node = cgraph_nodes; node; node = node->next)
     if (node->reachable && node->lowered
	&& cgraph_function_body_availability (node) >= AVAIL_OVERWRITABLE)
       {
	d = get_cg_data (&node, true);
	if (is_tm_safe (node->decl))		<-- !! FINDS NOTHING !!
	  ipa_tm_diagnose_tm_safe (node);
	else if (d->all_tm_regions)
	  ipa_tm_diagnose_transaction (node, d->all_tm_regions);
       }

The call to ipa_tm_diagnose_tm_safe() does nothing because there are no 
longer any calls in the function, since the function call has been inlined:

f ()
{
<bb 2>:
   __asm__ __volatile__("");
   return;

}

Perhaps we could issue the error when we notice the GIMPLE_ASM while 
scanning for irrevocable blocks earlier.  The attached patch does so, 
and fixes the PR.

What am I missing, cause I *know* there's a rat's nest somewhere.

Aldy
PR middle-end/52141
	* trans-mem.c (ipa_tm_scan_irr_block): Error on GIMPLE_ASM.
Richard Henderson - Feb. 14, 2012, 5:47 p.m.
On 02/14/2012 08:46 AM, Aldy Hernandez wrote:
> The call to ipa_tm_diagnose_tm_safe() does nothing because there are no longer any calls in the function, since the function call has been inlined:
> 
> f ()
> {
> <bb 2>:
>   __asm__ __volatile__("");
>   return;
> 
> }
> 
> Perhaps we could issue the error when we notice the GIMPLE_ASM while scanning for irrevocable blocks earlier.  The attached patch does so, and fixes the PR.
> 
> What am I missing, cause I *know* there's a rat's nest somewhere.

Ug.

Which means that the error message is all too likely simply be confusing
rather than anything else, since the asm isn't lexically present in the
transaction.

I wonder, not for the first time, if we shouldn't simply turn off early
inlining with TM, or at least of and into tm-related functions, such as
this.  I assume that the IPA inlining pass would take up the slack...


r~
Richard Guenther - Feb. 15, 2012, 9:48 a.m.
On Tue, Feb 14, 2012 at 6:47 PM, Richard Henderson <rth@redhat.com> wrote:
> On 02/14/2012 08:46 AM, Aldy Hernandez wrote:
>> The call to ipa_tm_diagnose_tm_safe() does nothing because there are no longer any calls in the function, since the function call has been inlined:
>>
>> f ()
>> {
>> <bb 2>:
>>   __asm__ __volatile__("");
>>   return;
>>
>> }
>>
>> Perhaps we could issue the error when we notice the GIMPLE_ASM while scanning for irrevocable blocks earlier.  The attached patch does so, and fixes the PR.
>>
>> What am I missing, cause I *know* there's a rat's nest somewhere.
>
> Ug.
>
> Which means that the error message is all too likely simply be confusing
> rather than anything else, since the asm isn't lexically present in the
> transaction.
>
> I wonder, not for the first time, if we shouldn't simply turn off early
> inlining with TM, or at least of and into tm-related functions, such as
> this.  I assume that the IPA inlining pass would take up the slack...

Hmm.  I think you rather want to teach local_pure_const about TM
properties you want to know and have them propagated properly
(of course unless pure/const which is about optimization and has an
easy fallback default yours wouldn't have that - you'd have to assume
the callee contains an invalid asm ...)

Richard.

>
> r~
Aldy Hernandez - Feb. 15, 2012, 2:30 p.m.
On 02/15/12 03:48, Richard Guenther wrote:
> On Tue, Feb 14, 2012 at 6:47 PM, Richard Henderson<rth@redhat.com>  wrote:
>> On 02/14/2012 08:46 AM, Aldy Hernandez wrote:
>>> The call to ipa_tm_diagnose_tm_safe() does nothing because there are no longer any calls in the function, since the function call has been inlined:
>>>
>>> f ()
>>> {
>>> <bb 2>:
>>>    __asm__ __volatile__("");
>>>    return;
>>>
>>> }
>>>
>>> Perhaps we could issue the error when we notice the GIMPLE_ASM while scanning for irrevocable blocks earlier.  The attached patch does so, and fixes the PR.
>>>
>>> What am I missing, cause I *know* there's a rat's nest somewhere.
>>
>> Ug.
>>
>> Which means that the error message is all too likely simply be confusing
>> rather than anything else, since the asm isn't lexically present in the
>> transaction.

That's why I also specified the calling function in the error message. 
But I do agree that the message has the potential of confusing.

>>
>> I wonder, not for the first time, if we shouldn't simply turn off early
>> inlining with TM, or at least of and into tm-related functions, such as
>> this.  I assume that the IPA inlining pass would take up the slack...

You will hear no complaints from me.  I'm tired of fixing the same bug 
over and over.

> Hmm.  I think you rather want to teach local_pure_const about TM
> properties you want to know and have them propagated properly
> (of course unless pure/const which is about optimization and has an
> easy fallback default yours wouldn't have that - you'd have to assume
> the callee contains an invalid asm ...)

Richard G., can you explain the parenthesized comment.  I'm not sure I 
follow you.
Richard Guenther - Feb. 15, 2012, 3:04 p.m.
On Wed, Feb 15, 2012 at 3:30 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
> On 02/15/12 03:48, Richard Guenther wrote:
>>
>> On Tue, Feb 14, 2012 at 6:47 PM, Richard Henderson<rth@redhat.com>  wrote:
>>>
>>> On 02/14/2012 08:46 AM, Aldy Hernandez wrote:
>>>>
>>>> The call to ipa_tm_diagnose_tm_safe() does nothing because there are no
>>>> longer any calls in the function, since the function call has been inlined:
>>>>
>>>> f ()
>>>> {
>>>> <bb 2>:
>>>>   __asm__ __volatile__("");
>>>>   return;
>>>>
>>>> }
>>>>
>>>> Perhaps we could issue the error when we notice the GIMPLE_ASM while
>>>> scanning for irrevocable blocks earlier.  The attached patch does so, and
>>>> fixes the PR.
>>>>
>>>> What am I missing, cause I *know* there's a rat's nest somewhere.
>>>
>>>
>>> Ug.
>>>
>>> Which means that the error message is all too likely simply be confusing
>>> rather than anything else, since the asm isn't lexically present in the
>>> transaction.
>
>
> That's why I also specified the calling function in the error message. But I
> do agree that the message has the potential of confusing.
>
>
>>>
>>> I wonder, not for the first time, if we shouldn't simply turn off early
>>> inlining with TM, or at least of and into tm-related functions, such as
>>> this.  I assume that the IPA inlining pass would take up the slack...
>
>
> You will hear no complaints from me.  I'm tired of fixing the same bug over
> and over.
>
>
>> Hmm.  I think you rather want to teach local_pure_const about TM
>> properties you want to know and have them propagated properly
>> (of course unless pure/const which is about optimization and has an
>> easy fallback default yours wouldn't have that - you'd have to assume
>> the callee contains an invalid asm ...)
>
>
> Richard G., can you explain the parenthesized comment.  I'm not sure I
> follow you.

Ok, I had a look at the bugreport and am curios why

__attribute__((always_inline))
static void asmfunc(void)
{
  __asm__ ("");
}

__attribute__((transaction_safe))
static void f(void)
{
  asmfunc();
}

you'd want a diagnostic when asmfunc is not inlined but OTOH do not
want to ICE when it is.  Why not diagnose GIMPLE_ASMs alongside
other 'unsafe function calls'.  Thus, why does __attribute__((transaction_safe))
not transitively cover all its callees (does it?)?

The comment of ipa_tm_diagnose_tm_safe says

/* Diagnose calls from transaction_safe functions to unmarked
   functions that are determined to not be safe.  */

and this "determined to not be safe" is what I was refering to with pointing
to how we handle pure/const "discovery".

For the asm above - how do you discover an asm is "unsafe"?  I suppose
you cannot, and you only consider asms lexically in a function marked
as transaction-safe as safe?  If so might I suggest to use a flag on the
GIMPLE_ASM stmt, set during initial tm lowering (or even during parsing maybe)?

Contrary to what rth says we _do_ have callgraph information available
during early optimization / inlining.  The callgraph is built over
GENERIC already.
So as you compute/propagate the tm_may_enter_irr flag only during
IPA TM you might do better (with deciding what to allow to inline during
early inlining, or other stuff) if you compute this function local property
(conservatively) late in the early optimization pipeline, close to
pass_local_pure_const (or simply integrated within it) - benefiting from
trivial propagation ensured by the way we walk functions in the callgraph
during early optimizations.  The IPA TM pass then simply would do the
propagation of the flag (no need to scan function bodies here, so this can
even work at LTO level!)

So, I guess the flag on the GIMPLE_ASM would be a way to fix 52141
(and probably the only sensible one?).  The hint at the local-pure-const
stuff would make IPA TM work at LTO level.

Hope that helps ;)

Richard.
Aldy Hernandez - Feb. 15, 2012, 4:53 p.m.
[rth, please correct me if I have misrepresented the TM semantics and 
rules].

On 02/15/12 09:04, Richard Guenther wrote:

> Ok, I had a look at the bugreport and am curios why
>
> __attribute__((always_inline))
> static void asmfunc(void)
> {
>    __asm__ ("");
> }
>
> __attribute__((transaction_safe))
> static void f(void)
> {
>    asmfunc();
> }
>
> you'd want a diagnostic when asmfunc is not inlined but OTOH do not
> want to ICE when it is.  Why not diagnose GIMPLE_ASMs alongside
> other 'unsafe function calls'.  Thus, why does __attribute__((transaction_safe))
> not transitively cover all its callees (does it?)?

No, transaction_safe does not transitively cover its callees.  A 
transaction safe function cannot call unsafe functions, nor can 
GIMPLE_ASMs appear in it.

(This was confusing for me at first too, because transaction_pure OTOH, 
does allow any type of call inside, or GIMPLE_ASM.  For 
transaction_pure, no instrumentation is done, and the function is 
outputted as is.)

>
> The comment of ipa_tm_diagnose_tm_safe says
>
> /* Diagnose calls from transaction_safe functions to unmarked
>     functions that are determined to not be safe.  */
>
> and this "determined to not be safe" is what I was refering to with pointing
> to how we handle pure/const "discovery".

Interesting.  I suppose we can add a cgraph bit for "this function may 
go irrevocable" and accumulate it during pure/const discovery.

> For the asm above - how do you discover an asm is "unsafe"?  I suppose
> you cannot, and you only consider asms lexically in a function marked
> as transaction-safe as safe?  If so might I suggest to use a flag on the
> GIMPLE_ASM stmt, set during initial tm lowering (or even during parsing maybe)?

No, AFAIK, asms aren't safe even in transaction_safe functions (don't 
look at me, I didn't write the standard).  But rth can correct me if I'm 
wrong.

> So as you compute/propagate the tm_may_enter_irr flag only during
> IPA TM you might do better (with deciding what to allow to inline during
> early inlining, or other stuff) if you compute this function local property
> (conservatively) late in the early optimization pipeline, close to
> pass_local_pure_const (or simply integrated within it) - benefiting from
> trivial propagation ensured by the way we walk functions in the callgraph
> during early optimizations.  The IPA TM pass then simply would do the
> propagation of the flag (no need to scan function bodies here, so this can
> even work at LTO level!)

Nice.  I like it.  I will play around with it.

> Hope that helps ;)

Definitely, thanks!
Aldy
Richard Henderson - Feb. 15, 2012, 5:29 p.m.
On 02/15/2012 01:48 AM, Richard Guenther wrote:
> Hmm.  I think you rather want to teach local_pure_const about TM
> properties you want to know and have them propagated properly
> (of course unless pure/const which is about optimization and has an
> easy fallback default yours wouldn't have that - you'd have to assume
> the callee contains an invalid asm ...)

That's more or less what we do ourselves during the tm ipa pass.

The trouble we have here is merely one of diagnostics.  The user
has written "transaction safe" and it's now our job to tell him
when he's done something that's not transaction safe.

The unfortunate thing of diagnostics is you'd like to be able to
tell the user what they did wrong at the source level, not after
10 levels of inlining.

And given that pass_local_pure_const still runs after pass_early_inline,
doing anything in there doesn't really help the problem at all.


r~
Jakub Jelinek - Feb. 15, 2012, 5:32 p.m.
On Wed, Feb 15, 2012 at 09:29:18AM -0800, Richard Henderson wrote:
> On 02/15/2012 01:48 AM, Richard Guenther wrote:
> > Hmm.  I think you rather want to teach local_pure_const about TM
> > properties you want to know and have them propagated properly
> > (of course unless pure/const which is about optimization and has an
> > easy fallback default yours wouldn't have that - you'd have to assume
> > the callee contains an invalid asm ...)
> 
> That's more or less what we do ourselves during the tm ipa pass.
> 
> The trouble we have here is merely one of diagnostics.  The user
> has written "transaction safe" and it's now our job to tell him
> when he's done something that's not transaction safe.
> 
> The unfortunate thing of diagnostics is you'd like to be able to
> tell the user what they did wrong at the source level, not after
> 10 levels of inlining.
> 
> And given that pass_local_pure_const still runs after pass_early_inline,
> doing anything in there doesn't really help the problem at all.

Use %K in error/warning fmt string, then the diagnostic will include the
full virtual backtrace.

	Jakub
Aldy Hernandez - Feb. 15, 2012, 5:59 p.m.
On 02/15/12 11:32, Jakub Jelinek wrote:
> On Wed, Feb 15, 2012 at 09:29:18AM -0800, Richard Henderson wrote:
>> On 02/15/2012 01:48 AM, Richard Guenther wrote:
>>> Hmm.  I think you rather want to teach local_pure_const about TM
>>> properties you want to know and have them propagated properly
>>> (of course unless pure/const which is about optimization and has an
>>> easy fallback default yours wouldn't have that - you'd have to assume
>>> the callee contains an invalid asm ...)
>>
>> That's more or less what we do ourselves during the tm ipa pass.
>>
>> The trouble we have here is merely one of diagnostics.  The user
>> has written "transaction safe" and it's now our job to tell him
>> when he's done something that's not transaction safe.
>>
>> The unfortunate thing of diagnostics is you'd like to be able to
>> tell the user what they did wrong at the source level, not after
>> 10 levels of inlining.
>>
>> And given that pass_local_pure_const still runs after pass_early_inline,
>> doing anything in there doesn't really help the problem at all.
>
> Use %K in error/warning fmt string, then the diagnostic will include the
> full virtual backtrace.
>
> 	Jakub

Hmmm, isn't %K for trees?  We're talking gimple, and FUNCTION_DECLs, 
which don't have a TREE_BLOCK, here.
Jakub Jelinek - Feb. 15, 2012, 6:07 p.m.
On Wed, Feb 15, 2012 at 11:59:15AM -0600, Aldy Hernandez wrote:
> Hmmm, isn't %K for trees?  We're talking gimple, and FUNCTION_DECLs,
> which don't have a TREE_BLOCK, here.

Gimple stmts have gimple_block, I guess you'd need to create some tree
and set its TREE_BLOCK and EXPR_LOCATION from gimple_block/gimple_location.
Or add something similar to %K that would take the same info from gimple
stmt and pass a stmt instead of a tree.

	Jakub

Patch

Index: testsuite/gcc.dg/tm/pr52141.c
===================================================================
--- testsuite/gcc.dg/tm/pr52141.c	(revision 0)
+++ testsuite/gcc.dg/tm/pr52141.c	(revision 0)
@@ -0,0 +1,21 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fgnu-tm -O1" } */
+
+inline void asmfunc(void)
+{
+  __asm__ (""); /* { dg-error "asm not allowed in .transaction_safe" } */
+}
+
+__attribute__((transaction_safe))
+static void f(void)
+{
+  asmfunc();
+}
+
+int main()
+{
+  __transaction_atomic {
+    f();
+  }
+  return 0;
+}
Index: trans-mem.c
===================================================================
--- trans-mem.c	(revision 184181)
+++ trans-mem.c	(working copy)
@@ -3736,6 +3736,10 @@  ipa_tm_scan_irr_block (basic_block bb)
 	     assembly statement is not relevant to the transaction
 	     is to wrap it in a __tm_waiver block.  This is not
 	     yet implemented, so we can't check for it.  */
+	  if (is_tm_safe (current_function_decl))
+	    error_at (gimple_location (stmt),
+		      "asm not allowed in %<transaction_safe%> function %qE",
+		      current_function_decl);
 	  return true;
 
 	default: