diff mbox

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

Message ID 4F3D40F9.2050800@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez Feb. 16, 2012, 5:46 p.m. UTC 
On 02/15/12 12:07, Jakub Jelinek wrote:
> 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

This may not actually work.  I am getting a different backtrace 
depending on whether I use -O0 or -O[123].  With the attached patch I 
get a proper backtrace with -O0, but none whatsoever with -O1:

#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$

With -O1, the TREE_BLOCK looks like this:

(gdb) ptg block
BLOCK #3
   SUPERCONTEXT: BLOCK #0
   ABSTRACT_ORIGIN: BLOCK #0

and the BLOCK_SUPERCONTEXT looks like this:

(gdb) ptg block
BLOCK #0
   SUPERCONTEXT: f
   SUBBLOCKS: BLOCK #3

Notice there is no BLOCK_ABSTRACT_ORIGIN in this last one, so we don't 
print any context.

I would rather have a consistent error message (though possibly 
confusing), than have an error message that sometimes makes sense.

To make matters worse, I have just noticed that at -O2 and -O3 we 
inlined all the functions into main, so the GIMPLE_ASM ends up in the 
transaction itself (see test), not in f(), so the compiler is able to 
determine we have a GIMPLE_ASM inside a transaction and issues a 
different error:

houston:/build/t2/gcc$ ./cc1 a.c -fgnu-tm -O2 -quiet -w
a.c:7:3: error: asm not allowed in atomic transaction

Here also we could've used the %K format modifier, if indeed we had full 
backtrace information.  But, like -O1, we don't.

I think I'm back to learning towards rth's suggestion of disabling early 
inlining.

Thoughts?

Comments

Jakub Jelinek Feb. 16, 2012, 6:04 p.m. UTC | #1 
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.

	Jakub
Aldy Hernandez Feb. 16, 2012, 6:26 p.m. UTC | #2 
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?
Richard Biener Feb. 20, 2012, 9:56 a.m. UTC | #3 
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.

Richard.
Jakub Jelinek Feb. 20, 2012, 9:58 a.m. UTC | #4 
On Mon, Feb 20, 2012 at 10:56:30AM +0100, Richard Guenther wrote:
> 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.

Yeah, it is unfortunate that without -g the diagnostic will be less verbose,
but even from the -g0 diagnostics IMHO anybody can figure that issue out.

	Jakub
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: