[PATCH] Improve debugging info (PR debug/53948 P1 regression)

[PATCH] Improve debugging info (PR debug/53948 P1 regression)

[Jeff Law]

[-- Attachment #1: Type: text/plain, Size: 1199 bytes --]


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?


[-- Attachment #2: P --]
[-- Type: text/plain, Size: 2525 bytes --]

	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.

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; }
+

Re: [PATCH] Improve debugging info (PR debug/53948 P1 regression)

[Jakub Jelinek]

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

Re: [PATCH] Improve debugging info (PR debug/53948 P1 regression)

[Jakub Jelinek]

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

Re: [PATCH] Improve debugging info (PR debug/53948 P1 regression)

[Jeff Law]

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

Re: [PATCH] Improve debugging info (PR debug/53948 P1 regression)

[Jeff Law]

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

Re: [PATCH] Improve debugging info (PR debug/53948 P1 regression)

[Eric Botcazou]

> Do we have any dwarf targets that don't support .loc?

Solaris targets with the Solaris assembler, I think.

-- 
Eric Botcazou

Re: [PATCH] Improve debugging info (PR debug/53948 P1 regression)

[Jeff Law]

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

[PATCH] Improve debugging info (PR debug/53948 P1 regression)

[Jeff Law]

[-- Attachment #1: Type: text/plain, Size: 124 bytes --]

Based on feedback from Jakub, I updated the testcase for this PR. 
Updated patch attached.

Installed onto the trunk.

Jeff

[-- Attachment #2: P --]
[-- Type: text/plain, Size: 3563 bytes --]

commit 48b3095f4ef7da1a06ad2fa96db3967762ce4f0b
Author: Jeff Law <law@redhat.com>
Date:   Fri Feb 8 09:57:43 2013 -0700

           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.
    
           PR debug/53948
            * gcc.dg/debug/dwarf2/pr53948.c: New test.

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 9d66888..2693b7e 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,11 @@
+2013-02-08  Jeff Law  <law@redhat.com>
+
+	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.
+
 2013-02-08  Michael Meissner  <meissner@linux.vnet.ibm.com>
 
 	PR target/56043
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/ChangeLog b/gcc/testsuite/ChangeLog
index 583e7d5..83843b7 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2013-02-08  Jeff Law  <law@redhat.com>
+
+	PR debug/53948
+	* gcc.dg/debug/dwarf2/pr53948.c: New test.
+
 2013-02-08  Michael Meissner  <meissner@linux.vnet.ibm.com>
 
 	PR target/56043
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..f0600b7
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/debug/dwarf2/pr53948.c
@@ -0,0 +1,10 @@
+/* Test that we have line information for the line
+   with local variable initializations.  */
+/* { dg-options "-O0 -g -dA" } */
+/* { dg-final { scan-assembler ".loc 1 8 0|# line 8" } } */
+
+
+int f (register int a, register int b) {
+  register int x = b, y = a;
+  return x + y; }
+