[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.