diff mbox

Improve debugging info (PR debug/53948 P1 regression)

Message ID 51152DBB.2090007@redhat.com
State New
Headers show

Commit Message

Jeff Law Feb. 8, 2013, 4:54 p.m. UTC 
The basic problem here is parameters are marked with REG_USERVAR_P and 
IRA creates conflicts for the parameters with the callee clobbered 
registers.

This results in the copy from the parameter registers into callee 
clobbered registers being removed.  The move instructions happen to be 
associated with the first line of code in the function.  So when a 
breakpoint is placed on the function, the breakpoint triggers a 
statement later than expected.

This patch restores IRA's behaviour WRT regs associated with PARM_DECLs 
that was inadvertently removed by Steven when he changed this code back 
in July to eliminate the inclusion of tree.h in ira-conflicts.c.

We could have used a bit in the base rtl structure to represent PARMs 
independently from user variables, but given we don't have any real 
separation of rtl from trees and the scarcity of flag bits in that 
structure, I just created a trivial function that lives in emit-rtl.c 
which has access to both tree and rtl headers.

Steven has a patch which takes the other approach (using a flag bit in 
the base rtl structure) if folks would prefer that approach.

Bootstrapped and regression tested x86_64-linux-gnu.

OK for trunk?
PR debug/53948
	* emit-rtl.c (reg_is_parm_p): New function.
	* regs.h (reg_is_parm_p): New prototype.
	* ira-conflicts.c (ira_build_conflicts): Allow parameters in
	callee-clobbered registers.

	* gcc.dg/debug/dwarf2/pr53948.c: New test.

Comments

Jakub Jelinek Feb. 8, 2013, 5:55 p.m. UTC | #1 
On Fri, Feb 08, 2013 at 09:54:19AM -0700, Jeff Law wrote:
> 	PR debug/53948
> 	* emit-rtl.c (reg_is_parm_p): New function.
> 	* regs.h (reg_is_parm_p): New prototype.
> 	* ira-conflicts.c (ira_build_conflicts): Allow parameters in
> 	callee-clobbered registers.

This looks good to me.

> 	* gcc.dg/debug/dwarf2/pr53948.c: New test.

But the testcase is problematic.

> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/debug/dwarf2/pr53948.c
> @@ -0,0 +1,10 @@
> +/* Test that we emit a .line directive for the line
> +   with local variable initializations.  */
> +/* { dg-options "-O0 -g" } */
> +/* { dg-final { scan-assembler ".loc 1 8 0" } } */

It will fail on any target which doesn't support .loc
directives.
I'd say better would be to scan the -fdump-rtl-final
for pr53948.c:8 regex.

	Jakub
Jakub Jelinek Feb. 8, 2013, 6:12 p.m. UTC | #2 
On Fri, Feb 08, 2013 at 06:55:08PM +0100, Jakub Jelinek wrote:
> But the testcase is problematic.
> 
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/debug/dwarf2/pr53948.c
> > @@ -0,0 +1,10 @@
> > +/* Test that we emit a .line directive for the line
> > +   with local variable initializations.  */
> > +/* { dg-options "-O0 -g" } */
> > +/* { dg-final { scan-assembler ".loc 1 8 0" } } */
> 
> It will fail on any target which doesn't support .loc
> directives.
> I'd say better would be to scan the -fdump-rtl-final
> for pr53948.c:8 regex.

Or perhaps
/* { dg-options "-O0 -g -dA" } */
/* { dg-final { scan-assembler ".loc 1 8 0|# line 8" } } */

	Jakub
Jeff Law Feb. 8, 2013, 6:23 p.m. UTC | #3 
On 02/08/2013 10:55 AM, Jakub Jelinek wrote:
> On Fri, Feb 08, 2013 at 09:54:19AM -0700, Jeff Law wrote:
>> 	PR debug/53948
>> 	* emit-rtl.c (reg_is_parm_p): New function.
>> 	* regs.h (reg_is_parm_p): New prototype.
>> 	* ira-conflicts.c (ira_build_conflicts): Allow parameters in
>> 	callee-clobbered registers.
>
> This looks good to me.
>
>> 	* gcc.dg/debug/dwarf2/pr53948.c: New test.
>
> But the testcase is problematic.
>
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/debug/dwarf2/pr53948.c
>> @@ -0,0 +1,10 @@
>> +/* Test that we emit a .line directive for the line
>> +   with local variable initializations.  */
>> +/* { dg-options "-O0 -g" } */
>> +/* { dg-final { scan-assembler ".loc 1 8 0" } } */
>
> It will fail on any target which doesn't support .loc
> directives.
Do we have any dwarf targets that don't support .loc?

While the bug isn't dwarf specific, I figured dwarf is used enough that 
just testing it on dwarf targets would be sufficient coverage.  Thus the 
test is in the dwarf specific part of the testsuite.

jeff
Jeff Law Feb. 8, 2013, 6:37 p.m. UTC | #4 
On 02/08/2013 10:55 AM, Jakub Jelinek wrote:
> On Fri, Feb 08, 2013 at 09:54:19AM -0700, Jeff Law wrote:
>> 	PR debug/53948
>> 	* emit-rtl.c (reg_is_parm_p): New function.
>> 	* regs.h (reg_is_parm_p): New prototype.
>> 	* ira-conflicts.c (ira_build_conflicts): Allow parameters in
>> 	callee-clobbered registers.
>
> This looks good to me.
>
>> 	* gcc.dg/debug/dwarf2/pr53948.c: New test.
>
> But the testcase is problematic.
>
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/debug/dwarf2/pr53948.c
>> @@ -0,0 +1,10 @@
>> +/* Test that we emit a .line directive for the line
>> +   with local variable initializations.  */
>> +/* { dg-options "-O0 -g" } */
>> +/* { dg-final { scan-assembler ".loc 1 8 0" } } */
>
> It will fail on any target which doesn't support .loc
> directives.
> I'd say better would be to scan the -fdump-rtl-final
> for pr53948.c:8 regex.
How about this:


/* Test that we have line information for the line
    with local variable initializations.  */
/* { dg-options "-O0 -g -fdump-rtl-final" } */
/* { dg-final { scan-rtl-dump "pr53948.c:7" "final" } } */

int f (register int a, register int b) {
   register int x = b, y = a;
   return x + y; }

Jeff
Eric Botcazou Feb. 9, 2013, 1:49 p.m. UTC | #5 
> Do we have any dwarf targets that don't support .loc?

Solaris targets with the Solaris assembler, I think.
Jeff Law Feb. 9, 2013, 8:26 p.m. UTC | #6 
On 02/09/2013 06:49 AM, Eric Botcazou wrote:
>> Do we have any dwarf targets that don't support .loc?
>
> Solaris targets with the Solaris assembler, I think.
Thanks.

I found out that ia64 only conditionally uses .loc as well.  Regardless, 
I used Jakub's regexp, so we should be good for targets with and without 
.loc support.

jeff
diff mbox

Patch

diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
index f997e5d..2c70fb1 100644
--- a/gcc/emit-rtl.c
+++ b/gcc/emit-rtl.c
@@ -919,6 +919,18 @@  gen_reg_rtx (enum machine_mode mode)
   return val;
 }
 
+/* Return TRUE if REG is a PARM_DECL, FALSE otherwise.  */
+
+bool
+reg_is_parm_p (rtx reg)
+{
+  tree decl;
+
+  gcc_assert (REG_P (reg));
+  decl = REG_EXPR (reg);
+  return (decl && TREE_CODE (decl) == PARM_DECL);
+}
+
 /* Update NEW with the same attributes as REG, but with OFFSET added
    to the REG_OFFSET.  */
 
diff --git a/gcc/ira-conflicts.c b/gcc/ira-conflicts.c
index 711db0f..710986b 100644
--- a/gcc/ira-conflicts.c
+++ b/gcc/ira-conflicts.c
@@ -895,8 +895,12 @@  ira_build_conflicts (void)
 
 	  if ((! flag_caller_saves && ALLOCNO_CALLS_CROSSED_NUM (a) != 0)
 	      /* For debugging purposes don't put user defined variables in
-		 callee-clobbered registers.  */
-	      || (optimize == 0 && REG_USERVAR_P (allocno_reg)))
+		 callee-clobbered registers.  However, do allow parameters
+		 in callee-clobbered registers to improve debugging.  This
+		 is a bit of a fragile hack.  */
+	      || (optimize == 0
+		  && REG_USERVAR_P (allocno_reg)
+		  && ! reg_is_parm_p (allocno_reg)))
 	    {
 	      IOR_HARD_REG_SET (OBJECT_TOTAL_CONFLICT_HARD_REGS (obj),
 				call_used_reg_set);
diff --git a/gcc/regs.h b/gcc/regs.h
index 0532d08..090d6b6 100644
--- a/gcc/regs.h
+++ b/gcc/regs.h
@@ -89,6 +89,8 @@  REG_N_SETS (int regno)
 #define SET_REG_N_SETS(N,V) (regstat_n_sets_and_refs[N].sets = V)
 #define INC_REG_N_SETS(N,V) (regstat_n_sets_and_refs[N].sets += V)
 
+/* Given a REG, return TRUE if the reg is a PARM_DECL, FALSE otherwise.  */
+extern bool reg_is_parm_p (rtx);
 
 /* Functions defined in regstat.c.  */
 extern void regstat_init_n_sets_and_refs (void);
diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/pr53948.c b/gcc/testsuite/gcc.dg/debug/dwarf2/pr53948.c
new file mode 100644
index 0000000..8bc8ed4
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/debug/dwarf2/pr53948.c
@@ -0,0 +1,10 @@ 
+/* Test that we emit a .line directive for the line
+   with local variable initializations.  */
+/* { dg-options "-O0 -g" } */
+/* { dg-final { scan-assembler ".loc 1 8 0" } } */
+
+
+int f (register int a, register int b) {
+  register int x = b, y = a;
+  return x + y; }
+