[RFA, ARM] Fix line number data for PIC register setup code

[RFA, ARM] Fix line number data for PIC register setup code

[Ulrich Weigand]

Hello,

we've been seeing problems with GDB being unable to correctly identify
the first line of a function that are caused by an issue with ARM PIC
code generation.

As a small example, the following test case:

int foo (char* s);
extern char hello[];

void test (int x)
{
  int y = x + 4;
  foo(hello);
}

compiles to a function with the following prologue (using -fPIC):

        .loc 1 6 0
        .cfi_startproc
        @ args = 0, pretend = 0, frame = 16
        @ frame_needed = 1, uses_anonymous_args = 0
        stmfd   sp!, {fp, lr}
.LCFI0:
        .cfi_def_cfa_offset 8
        .cfi_offset 14, -4
        .cfi_offset 11, -8
        add     fp, sp, #4
.LCFI1:
        .cfi_def_cfa 11, 4
        sub     sp, sp, #16
        .loc 1 8 0
        ldr     r3, .L2
.LPIC0:
        add     r3, pc, r3
        .loc 1 6 0
        str     r0, [fp, #-16]

Note how --while the majority of the prologue code is correctly
identified at line 6-- the two instructions used to set up the
PIC register are identified as belonging to line 8.  This causes
GDB to set a breakpoint at line 8, instead of line 7, as the
first "real" line of the function.

The reason for this is code in require_pic_register, which constructs
the insns corresponding to those two lines.  The insns are injected into
the function prologue via
  insert_insn_on_edge (seq, single_succ_edge (ENTRY_BLOCK_PTR));

However, these days each instruction carries "locator" information
to link back to the source line it originates from.  Since the
require_pic_register does not explicitly set the locator data,
by default this information points back to whatever line was
"current" when require_pic_register was called, which will usually
be the first source line that uses some construct that requires
the PIC register to be initialized.

It seems to me the proper fix for this is to reset the locator
information for those instructions generated in require_pic_register
to "prologue_locator", just as is done for all other prologue code
in thread_prologue_and_epilogue_insns.

The following patch implements this change, leading to this
assembler code:

	.loc 1 6 0
        .cfi_startproc
        @ args = 0, pretend = 0, frame = 16
        @ frame_needed = 1, uses_anonymous_args = 0
        stmfd   sp!, {fp, lr}
.LCFI0:
        .cfi_def_cfa_offset 8
        .cfi_offset 14, -4
        .cfi_offset 11, -8
        add     fp, sp, #4
.LCFI1:
        .cfi_def_cfa 11, 4
        sub     sp, sp, #16
        ldr     r3, .L2
.LPIC0:
        add     r3, pc, r3
        str     r0, [fp, #-16]

which makes the GDB problems go away.

Tested on armv7l-linux-gnueabi with no regressions.
OK for mainline?

Bye,
Ulrich


ChangeLog:

	* config/arm/arm.c (require_pic_register): Set INSN_LOCATOR for all
	instructions injected into the prologue to prologue_locator.

Index: gcc/config/arm/arm.c
===================================================================
--- gcc/config/arm/arm.c	(revision 167799)
+++ gcc/config/arm/arm.c	(working copy)
@@ -5116,7 +5116,7 @@
 	}
       else
 	{
-	  rtx seq;
+	  rtx seq, insn;
 
 	  if (!cfun->machine->pic_reg)
 	    cfun->machine->pic_reg = gen_reg_rtx (Pmode);
@@ -5133,6 +5133,11 @@
 
 	      seq = get_insns ();
 	      end_sequence ();
+
+	      for (insn = seq; insn; insn = NEXT_INSN (insn))
+		if (INSN_P (insn))
+		  INSN_LOCATOR (insn) = prologue_locator;
+
 	      /* We can be called during expansion of PHI nodes, where
 	         we can't yet emit instructions directly in the final
 		 insn stream.  Queue the insns on the entry edge, they will
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFA, ARM] Fix line number data for PIC register setup code

[Richard Earnshaw]

On Mon, 2010-12-20 at 19:13 +0100, Ulrich Weigand wrote:
> Hello,
> 
> we've been seeing problems with GDB being unable to correctly identify
> the first line of a function that are caused by an issue with ARM PIC
> code generation.
> 
> As a small example, the following test case:
> 
> int foo (char* s);
> extern char hello[];
> 
> void test (int x)
> {
>   int y = x + 4;
>   foo(hello);
> }
> 
> compiles to a function with the following prologue (using -fPIC):
> 
>         .loc 1 6 0
>         .cfi_startproc
>         @ args = 0, pretend = 0, frame = 16
>         @ frame_needed = 1, uses_anonymous_args = 0
>         stmfd   sp!, {fp, lr}
> .LCFI0:
>         .cfi_def_cfa_offset 8
>         .cfi_offset 14, -4
>         .cfi_offset 11, -8
>         add     fp, sp, #4
> .LCFI1:
>         .cfi_def_cfa 11, 4
>         sub     sp, sp, #16
>         .loc 1 8 0
>         ldr     r3, .L2
> .LPIC0:
>         add     r3, pc, r3
>         .loc 1 6 0
>         str     r0, [fp, #-16]
> 
> Note how --while the majority of the prologue code is correctly
> identified at line 6-- the two instructions used to set up the
> PIC register are identified as belonging to line 8.  This causes
> GDB to set a breakpoint at line 8, instead of line 7, as the
> first "real" line of the function.
> 
> The reason for this is code in require_pic_register, which constructs
> the insns corresponding to those two lines.  The insns are injected into
> the function prologue via
>   insert_insn_on_edge (seq, single_succ_edge (ENTRY_BLOCK_PTR));
> 
> However, these days each instruction carries "locator" information
> to link back to the source line it originates from.  Since the
> require_pic_register does not explicitly set the locator data,
> by default this information points back to whatever line was
> "current" when require_pic_register was called, which will usually
> be the first source line that uses some construct that requires
> the PIC register to be initialized.
> 
> It seems to me the proper fix for this is to reset the locator
> information for those instructions generated in require_pic_register
> to "prologue_locator", just as is done for all other prologue code
> in thread_prologue_and_epilogue_insns.
> 
> The following patch implements this change, leading to this
> assembler code:
> 
> 	.loc 1 6 0
>         .cfi_startproc
>         @ args = 0, pretend = 0, frame = 16
>         @ frame_needed = 1, uses_anonymous_args = 0
>         stmfd   sp!, {fp, lr}
> .LCFI0:
>         .cfi_def_cfa_offset 8
>         .cfi_offset 14, -4
>         .cfi_offset 11, -8
>         add     fp, sp, #4
> .LCFI1:
>         .cfi_def_cfa 11, 4
>         sub     sp, sp, #16
>         ldr     r3, .L2
> .LPIC0:
>         add     r3, pc, r3
>         str     r0, [fp, #-16]
> 
> which makes the GDB problems go away.
> 
> Tested on armv7l-linux-gnueabi with no regressions.
> OK for mainline?
> 
> Bye,
> Ulrich
> 
> 
> ChangeLog:
> 
> 	* config/arm/arm.c (require_pic_register): Set INSN_LOCATOR for all
> 	instructions injected into the prologue to prologue_locator.
> 
> Index: gcc/config/arm/arm.c
> ===================================================================
> --- gcc/config/arm/arm.c	(revision 167799)
> +++ gcc/config/arm/arm.c	(working copy)
> @@ -5116,7 +5116,7 @@
>  	}
>        else
>  	{
> -	  rtx seq;
> +	  rtx seq, insn;
>  
>  	  if (!cfun->machine->pic_reg)
>  	    cfun->machine->pic_reg = gen_reg_rtx (Pmode);
> @@ -5133,6 +5133,11 @@
>  
>  	      seq = get_insns ();
>  	      end_sequence ();
> +
> +	      for (insn = seq; insn; insn = NEXT_INSN (insn))
> +		if (INSN_P (insn))
> +		  INSN_LOCATOR (insn) = prologue_locator;
> +

Hmm, what if get_insns() doesn't return an INSN_P(), which is presumably
what the test in the 'if' clause is for?  Won't it then abort in
NEXT_INSN()?

I think the for loop should be controlled by (insn && INSN_P (insn)) to
guard against that case.

R.

Re: [RFA, ARM] Fix line number data for PIC register setup code

[Richard Henderson]

On 12/20/2010 10:26 AM, Richard Earnshaw wrote:
>> +	      for (insn = seq; insn; insn = NEXT_INSN (insn))
>> +		if (INSN_P (insn))
>> +		  INSN_LOCATOR (insn) = prologue_locator;
>> +
> 
> Hmm, what if get_insns() doesn't return an INSN_P(), which is presumably
> what the test in the 'if' clause is for?  Won't it then abort in
> NEXT_INSN()?

No, it won't.  INSN_P does not include notes, for example.


r~

Re: [RFA, ARM] Fix line number data for PIC register setup code

[Richard Earnshaw]

On 20 Dec 2010, at 21:06, "Richard Henderson" <rth@redhat.com> wrote:

> On 12/20/2010 10:26 AM, Richard Earnshaw wrote:
>>> +          for (insn = seq; insn; insn = NEXT_INSN (insn))
>>> +        if (INSN_P (insn))
>>> +          INSN_LOCATOR (insn) = prologue_locator;
>>> +
>> 
>> Hmm, what if get_insns() doesn't return an INSN_P(), which is presumably
>> what the test in the 'if' clause is for?  Won't it then abort in
>> NEXT_INSN()?
> 
> No, it won't.  INSN_P does not include notes, for example.
> 
> 
> r~

Ah! I think I must be confusing this with some historical other behaviour.

Anyway, the other aspects of this patch were fine, so if this bit is OK then I'm happy with it all now. 

R.

Re: [RFA, ARM] Fix line number data for PIC register setup code

[Ulrich Weigand]

Richard Earnshaw wrote:
> On 20 Dec 2010, at 21:06, "Richard Henderson" <rth@redhat.com> wrote:
> > On 12/20/2010 10:26 AM, Richard Earnshaw wrote:
> >>> +          for (insn =3D seq; insn; insn =3D NEXT_INSN (insn))
> >>> +        if (INSN_P (insn))
> >>> +          INSN_LOCATOR (insn) =3D prologue_locator;
> >>> +
> >>=20
> >> Hmm, what if get_insns() doesn't return an INSN_P(), which is presumably
> >> what the test in the 'if' clause is for?  Won't it then abort in
> >> NEXT_INSN()?
> >=20
> > No, it won't.  INSN_P does not include notes, for example.
> >=20
> >=20
> > r~
> 
> Ah! I think I must be confusing this with some historical other behaviour.
> 
> Anyway, the other aspects of this patch were fine, so if this bit is OK then
> I'm happy with it all now.

OK, thanks.  I've checked the patch in now.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com