diff mbox series

Avoid UNSPEC_VOLATILE or volatile ASM_OPERANDS in debug insns (PR debug/83084)

Message ID 20171122092222.GI14653@tucnak
State New
Headers show
Series Avoid UNSPEC_VOLATILE or volatile ASM_OPERANDS in debug insns (PR debug/83084) | expand

Commit Message

Jakub Jelinek Nov. 22, 2017, 9:22 a.m. UTC 
Hi!

Seems at least the scheduler treats UNSPEC_VOLATILE and perhaps volatile asm
in DEBUG_INSNs as having side-effects and schedules differently depending on
that.  While we could tweak the scheduler not to do that, these will be
actually never useful in DEBUG_INSNs anyway, we don't know what it means so
we can't represent it in debug info.

So, the following patch just makes sure we don't add them into debug_insns,
instead reset those.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2017-11-22  Jakub Jelinek  <jakub@redhat.com>

	PR debug/83084
	* valtrack.c (propagate_for_debug_subst, propagate_for_debug): Reset
	debug insns if they would contain UNSPEC_VOLATILE or volatile asm.
	(dead_debug_insert_temp): Likewise, but also ignore even non-volatile
	asm.

	* g++.dg/opt/pr83084.C: New test.


	Jakub

Comments

Richard Biener Nov. 22, 2017, 9:42 a.m. UTC | #1 
On Wed, 22 Nov 2017, Jakub Jelinek wrote:

> Hi!
> 
> Seems at least the scheduler treats UNSPEC_VOLATILE and perhaps volatile asm
> in DEBUG_INSNs as having side-effects and schedules differently depending on
> that.  While we could tweak the scheduler not to do that, these will be
> actually never useful in DEBUG_INSNs anyway, we don't know what it means so
> we can't represent it in debug info.
> 
> So, the following patch just makes sure we don't add them into debug_insns,
> instead reset those.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok.

Do we have some RTL IL verifier where we could verify those don't appear?
Maybe ICE in dwarf2out.c?

Thanks,
Richard.

> 2017-11-22  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR debug/83084
> 	* valtrack.c (propagate_for_debug_subst, propagate_for_debug): Reset
> 	debug insns if they would contain UNSPEC_VOLATILE or volatile asm.
> 	(dead_debug_insert_temp): Likewise, but also ignore even non-volatile
> 	asm.
> 
> 	* g++.dg/opt/pr83084.C: New test.
> 
> --- gcc/valtrack.c.jj	2017-09-12 21:58:01.000000000 +0200
> +++ gcc/valtrack.c	2017-11-21 18:26:10.188009398 +0100
> @@ -171,10 +171,13 @@ propagate_for_debug_subst (rtx from, con
>  	if (REG_P (*iter) && ++cnt > 1)
>  	  {
>  	    rtx dval = make_debug_expr_from_rtl (old_rtx);
> +	    rtx to = pair->to;
> +	    if (volatile_insn_p (to))
> +	      to = gen_rtx_UNKNOWN_VAR_LOC ();
>  	    /* Emit a debug bind insn.  */
>  	    rtx bind
>  	      = gen_rtx_VAR_LOCATION (GET_MODE (old_rtx),
> -				      DEBUG_EXPR_TREE_DECL (dval), pair->to,
> +				      DEBUG_EXPR_TREE_DECL (dval), to,
>  				      VAR_INIT_STATUS_INITIALIZED);
>  	    rtx_insn *bind_insn = emit_debug_insn_before (bind, pair->insn);
>  	    df_insn_rescan (bind_insn);
> @@ -217,6 +220,8 @@ propagate_for_debug (rtx_insn *insn, rtx
>  					 dest, propagate_for_debug_subst, &p);
>  	  if (loc == INSN_VAR_LOCATION_LOC (insn))
>  	    continue;
> +	  if (volatile_insn_p (loc))
> +	    loc = gen_rtx_UNKNOWN_VAR_LOC ();
>  	  INSN_VAR_LOCATION_LOC (insn) = loc;
>  	  df_insn_rescan (insn);
>  	}
> @@ -660,6 +665,12 @@ dead_debug_insert_temp (struct dead_debu
>  		}
>  	      return 0;
>  	    }
> +	  /* Asm in DEBUG_INSN is never useful, we can't emit debug info for
> +	     that.  And for volatile_insn_p, it is actually harmful
> +	     - DEBUG_INSNs shouldn't have any side-effects.  */
> +	  else if (GET_CODE (src) == ASM_OPERANDS
> +		   || volatile_insn_p (src))
> +	    set = NULL_RTX;
>  	}
>  
>        /* ??? Should we try to extract it from a PARALLEL?  */
> --- gcc/testsuite/g++.dg/opt/pr83084.C.jj	2017-11-21 18:30:22.252935128 +0100
> +++ gcc/testsuite/g++.dg/opt/pr83084.C	2017-11-21 18:29:18.000000000 +0100
> @@ -0,0 +1,16 @@
> +// PR debug/83084
> +// { dg-do compile }
> +// { dg-options "-O2 -fcompare-debug -Wno-return-type" }
> +
> +enum E { F };
> +template <E = F> struct A {
> +  bool foo ();
> +  int b;
> +};
> +template <> bool A<>::foo () {
> +  int a;
> +  do
> +    if (a)
> +      return false;
> +  while (__atomic_compare_exchange_n (&b, &a, 0, 1, 4, 0));
> +}
> 
> 	Jakub
> 
>
Jakub Jelinek Nov. 22, 2017, 10 a.m. UTC | #2 
On Wed, Nov 22, 2017 at 10:42:13AM +0100, Richard Biener wrote:
> On Wed, 22 Nov 2017, Jakub Jelinek wrote:
> 
> > Hi!
> > 
> > Seems at least the scheduler treats UNSPEC_VOLATILE and perhaps volatile asm
> > in DEBUG_INSNs as having side-effects and schedules differently depending on
> > that.  While we could tweak the scheduler not to do that, these will be
> > actually never useful in DEBUG_INSNs anyway, we don't know what it means so
> > we can't represent it in debug info.
> > 
> > So, the following patch just makes sure we don't add them into debug_insns,
> > instead reset those.
> > 
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> Ok.
> 
> Do we have some RTL IL verifier where we could verify those don't appear?
> Maybe ICE in dwarf2out.c?

dwarf2out.c would ICE on UNSPEC_VOLATILE (but not ASM_OPERANDS), no idea why
it didn't make all the way up to there.
In any case, a more useful verifier would be something like we have for
GIMPLE verification, something that walks the whole IL after every pass and
ICEs if something fails verification.  We don't have that for RTL and I
think we should, it would e.g. catch the various cases where backends just
use invalid RTL in their patterns on which e.g. simplify-rtx.c could just
ICE if it made it there.

	Jakub
Richard Biener Nov. 22, 2017, 10:11 a.m. UTC | #3 
On Wed, 22 Nov 2017, Jakub Jelinek wrote:

> On Wed, Nov 22, 2017 at 10:42:13AM +0100, Richard Biener wrote:
> > On Wed, 22 Nov 2017, Jakub Jelinek wrote:
> > 
> > > Hi!
> > > 
> > > Seems at least the scheduler treats UNSPEC_VOLATILE and perhaps volatile asm
> > > in DEBUG_INSNs as having side-effects and schedules differently depending on
> > > that.  While we could tweak the scheduler not to do that, these will be
> > > actually never useful in DEBUG_INSNs anyway, we don't know what it means so
> > > we can't represent it in debug info.
> > > 
> > > So, the following patch just makes sure we don't add them into debug_insns,
> > > instead reset those.
> > > 
> > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> > 
> > Ok.
> > 
> > Do we have some RTL IL verifier where we could verify those don't appear?
> > Maybe ICE in dwarf2out.c?
> 
> dwarf2out.c would ICE on UNSPEC_VOLATILE (but not ASM_OPERANDS), no idea why
> it didn't make all the way up to there.
> In any case, a more useful verifier would be something like we have for
> GIMPLE verification, something that walks the whole IL after every pass and
> ICEs if something fails verification.  We don't have that for RTL and I
> think we should, it would e.g. catch the various cases where backends just
> use invalid RTL in their patterns on which e.g. simplify-rtx.c could just
> ICE if it made it there.

Yes.  Too bad we don't have such thing.

Richard.
diff mbox series

Patch

--- gcc/valtrack.c.jj	2017-09-12 21:58:01.000000000 +0200
+++ gcc/valtrack.c	2017-11-21 18:26:10.188009398 +0100
@@ -171,10 +171,13 @@  propagate_for_debug_subst (rtx from, con
 	if (REG_P (*iter) && ++cnt > 1)
 	  {
 	    rtx dval = make_debug_expr_from_rtl (old_rtx);
+	    rtx to = pair->to;
+	    if (volatile_insn_p (to))
+	      to = gen_rtx_UNKNOWN_VAR_LOC ();
 	    /* Emit a debug bind insn.  */
 	    rtx bind
 	      = gen_rtx_VAR_LOCATION (GET_MODE (old_rtx),
-				      DEBUG_EXPR_TREE_DECL (dval), pair->to,
+				      DEBUG_EXPR_TREE_DECL (dval), to,
 				      VAR_INIT_STATUS_INITIALIZED);
 	    rtx_insn *bind_insn = emit_debug_insn_before (bind, pair->insn);
 	    df_insn_rescan (bind_insn);
@@ -217,6 +220,8 @@  propagate_for_debug (rtx_insn *insn, rtx
 					 dest, propagate_for_debug_subst, &p);
 	  if (loc == INSN_VAR_LOCATION_LOC (insn))
 	    continue;
+	  if (volatile_insn_p (loc))
+	    loc = gen_rtx_UNKNOWN_VAR_LOC ();
 	  INSN_VAR_LOCATION_LOC (insn) = loc;
 	  df_insn_rescan (insn);
 	}
@@ -660,6 +665,12 @@  dead_debug_insert_temp (struct dead_debu
 		}
 	      return 0;
 	    }
+	  /* Asm in DEBUG_INSN is never useful, we can't emit debug info for
+	     that.  And for volatile_insn_p, it is actually harmful
+	     - DEBUG_INSNs shouldn't have any side-effects.  */
+	  else if (GET_CODE (src) == ASM_OPERANDS
+		   || volatile_insn_p (src))
+	    set = NULL_RTX;
 	}
 
       /* ??? Should we try to extract it from a PARALLEL?  */
--- gcc/testsuite/g++.dg/opt/pr83084.C.jj	2017-11-21 18:30:22.252935128 +0100
+++ gcc/testsuite/g++.dg/opt/pr83084.C	2017-11-21 18:29:18.000000000 +0100
@@ -0,0 +1,16 @@ 
+// PR debug/83084
+// { dg-do compile }
+// { dg-options "-O2 -fcompare-debug -Wno-return-type" }
+
+enum E { F };
+template <E = F> struct A {
+  bool foo ();
+  int b;
+};
+template <> bool A<>::foo () {
+  int a;
+  do
+    if (a)
+      return false;
+  while (__atomic_compare_exchange_n (&b, &a, 0, 1, 4, 0));
+}