diff mbox

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

Message ID 4F42B843.7080408@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez Feb. 20, 2012, 9:16 p.m. UTC 
On 02/20/12 03:56, Richard Guenther wrote:
> On Thu, Feb 16, 2012 at 7:26 PM, Aldy Hernandez<aldyh@redhat.com>  wrote:
>> On 02/16/12 12:04, Jakub Jelinek wrote:
>>>
>>> On Thu, Feb 16, 2012 at 11:46:33AM -0600, Aldy Hernandez wrote:
>>>>
>>>> #GOOD
>>>> houston:/build/t2/gcc$ ./cc1 a.c -fgnu-tm -O0 -quiet -w
>>>> In function 'asmfunc',
>>>>      inlined from 'f' at a.c:13:10:
>>>> a.c:7:3: error: asm not allowed in 'transaction_safe' function
>>>>
>>>> #BAD
>>>> houston:/build/t2/gcc$ ./cc1 a.c -fgnu-tm -O1 -quiet -w
>>>> a.c: In function 'f':
>>>> a.c:7:3: error: asm not allowed in 'transaction_safe' function
>>>> houston:/build/t2/gcc$
>>>
>>>
>>> Even with -O1 -g ?  With -g0 we prune BLOCKs, so the backtraces
>>> don't work well.
>>
>>
>> Well with -O1 -g the backtrace works for this particular error.  But with
>> -O2 -g, we trigger another error "asm not allowed in atomic transaction",
>> and no amount of %K fiddling will get me a backtrace.
>>
>> But even so, it doesn't seem right to expect users to get a correct error
>> message only with -g, does it?
>
> Well, you can't expect diagnostics to be perfect with inlining in place.
> But you can try improving things by looking at what blocks tree-ssa-live.c
> prunes.
>
> Disabling early inlining for TM is a hack, I don't think you want to make
> people pay the optimizing penalty just because of asm diagnostics.

Fair enough.

So back to my original patch, but with %K which makes it much more 
readable with -O -g or with no optimization.

I checked and at -O[23], the error message is a different one (and not 
an ICE thankfully), and has been suffering from the same lack of context 
from many eons back, so no regressions there.

This patch fixes the ICE in the PR, and since the function attribute in 
the PR is always_inline, it displays a very informative message with 
either -O0 or -O -g.

So this patch fixes the PR in its entirety.

OK?
PR middle-end/52141
	* trans-mem.c (ipa_tm_scan_irr_block): Error out on GIMPLE_ASM's
	in a transaction safe function.

Comments

Richard Henderson Feb. 20, 2012, 10:34 p.m. UTC | #1 
On 02/20/12 13:16, Aldy Hernandez wrote:
> 	PR middle-end/52141
> 	* trans-mem.c (ipa_tm_scan_irr_block): Error out on GIMPLE_ASM's
> 	in a transaction safe function.

Ok.


r~
diff mbox

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,24 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fgnu-tm -O0 -w" } */
+
+__attribute__((always_inline))
+static 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;
+}
+
+/* { dg-message "inlined from \'f\'" "" { target *-*-* } 0 } */
Index: trans-mem.c
===================================================================
--- trans-mem.c	(revision 184272)
+++ trans-mem.c	(working copy)
@@ -3736,6 +3736,13 @@  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))
+	    {
+	      tree t = build1 (NOP_EXPR, void_type_node, size_zero_node);
+	      SET_EXPR_LOCATION (t, gimple_location (stmt));
+	      TREE_BLOCK (t) = gimple_block (stmt);
+	      error ("%Kasm not allowed in %<transaction_safe%> function", t);
+	    }
 	  return true;
 
 	default: