Message ID | 201012201813.oBKIDHN4012271@d06av02.portsmouth.uk.ibm.com |
---|---|
State | New |
Headers | show |
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.
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~
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.
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