| Message ID | 525B1BDD.2000306@mentor.com |
|---|---|
| State | New |
| Headers | show |
On 14/10/13 00:17, Tom de Vries wrote: > This patch makes sure we emit insertions scheduled for the first real BB before > NOTE_INSN_FUNCTION_BEG. As a consequence, it moves the PIC register setup code > to before the NOTE_INSN_FUNCTION_BEG. This removes the second .loc, and the > breakpoint of main ends up at line 8. > > Bootstrapped and regtested on x86_64 (ada inclusive), no issues found. > > Tested gdb with target arm-none-linux-gnueabi and CFLAGS_FOR_TARGET=-fPIC. The > patch removes 174 FAILs. > > Re-testing gcc with target arm-none-linux-gnueabi atm. > No issues found. OK for trunk? Thanks, - Tom
Ping. Original submission at http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00903.html . This patch fixes a regression of 174 tests in the gdb testsuite for arm-linux-gnueabi with -fPIC (or arm-linux-androideabi) caused by the fix for PR47028. The fix for PR47028 made sure that insertions on the single edge from ENTRY_BLOCK_PTR to the entry bb are committed after the parameters are available. However, the fix had as side-effect that the insertions could be committed both before and after NOTE_INSN_FUNCTION_BEG. Before the fix, the insertions were always committed before NOTE_INSN_FUNCTION_BEG. NOTE_INSN_FUNCTION_BEG has significance with respect to line number info, and insertions after the note caused wrong .loc info to be generated in the assembly in some cases, which caused the gdb test failures. This patch makes sure the insertions are committed before NOTE_INSN_FUNCTION_BEG. Thanks, - Tom
> The PIC register setup code is emitted after NOTE_INSNS_FUNCTION_BEG, > because it uses the insert_insn_on_edge mechanism, and the corresponding > insertion in cfgexpand.c:gimple_expand_cfg takes care to insert the code > after the parm_birth_insn: > ... > /* Avoid putting insns before parm_birth_insn. */ > if (e->src == ENTRY_BLOCK_PTR > && single_succ_p (ENTRY_BLOCK_PTR) > && parm_birth_insn) > { > rtx insns = e->insns.r; > e->insns.r = NULL_RTX; > emit_insn_after_noloc (insns, parm_birth_insn, e->dest); > } > ... > And in the case for this test-case, parm_birth_insn is the > NOTE_INSNS_FUNCTION_BEG. So this means that parm_birth_insn can never be null, right? > 2013-10-13 Tom de Vries <tom@codesourcery.com> > > * cfgexpand.c (gimple_expand_cfg): Don't commit insertions after > NOTE_INSN_FUNCTION_BEG. > > * gcc.target/arm/require-pic-register-loc.c: New test. OK if you also remove the test on parm_birth_insn.
Index: gcc/cfgexpand.c =================================================================== --- gcc/cfgexpand.c (revision 421892) +++ gcc/cfgexpand.c (working copy) @@ -4618,14 +4618,19 @@ gimple_expand_cfg (void) if (e->insns.r) { rebuild_jump_labels_chain (e->insns.r); - /* Avoid putting insns before parm_birth_insn. */ + /* Put insns after parm birth, but before + NOTE_INSNS_FUNCTION_BEG. */ if (e->src == ENTRY_BLOCK_PTR && single_succ_p (ENTRY_BLOCK_PTR) && parm_birth_insn) { rtx insns = e->insns.r; e->insns.r = NULL_RTX; - emit_insn_after_noloc (insns, parm_birth_insn, e->dest); + if (NOTE_P (parm_birth_insn) + && NOTE_KIND (parm_birth_insn) == NOTE_INSN_FUNCTION_BEG) + emit_insn_before_noloc (insns, parm_birth_insn, e->dest); + else + emit_insn_after_noloc (insns, parm_birth_insn, e->dest); } else commit_one_edge_insertion (e); Index: gcc/testsuite/gcc.target/arm/require-pic-register-loc.c =================================================================== --- /dev/null (new file) +++ gcc/testsuite/gcc.target/arm/require-pic-register-loc.c (revision 0) @@ -0,0 +1,29 @@ +/* { dg-do compile } */ +/* { dg-options "-g -fPIC" } */ + +void *v; +void a (void *x) { } +void b (void) { } + /* line 7. */ +int /* line 8. */ +main (int argc) /* line 9. */ +{ /* line 10. */ + if (argc == 12345) /* line 11. */ + { + a (v); + return 1; + } + b (); + + return 0; +} + +/* { dg-final { scan-assembler-not "\.loc 1 7 0" } } */ +/* { dg-final { scan-assembler-not "\.loc 1 8 0" } } */ +/* { dg-final { scan-assembler-not "\.loc 1 9 0" } } */ + +/* The loc at the start of the prologue. */ +/* { dg-final { scan-assembler-times "\.loc 1 10 0" 1 } } */ + +/* The loc at the end of the prologue, with the first user line. */ +/* { dg-final { scan-assembler-times "\.loc 1 11 0" 1 } } */
Richard, This patch fixes line number data for the PIC register setup code for ARM, resulting in 174 removed FAILs for the gdb testsuite with -fPIC. Consider break.c (minimized from gdb/testsuite/gdb.base/break.c): ... 1 void *v; 2 void a (void *x) { } 3 void b (void) { } 4 5 int 6 main (int argc) 7 { 8 if (argc == 12345) 9 { 10 a (v); 11 return 1; 12 } 13 b (); 14 15 return 0; 16 } ... We compile like this with -fPIC: ... $ arm-none-linux-gnueabi-gcc break.c -g -fPIC ... and run to a breakpoint in main: ... (gdb) b main Breakpoint 1 at 0x8410: file break.c, line 7. (gdb) c Continuing. Breakpoint 1, main (argc=1) at break.c:7 7 { ... The breakpoint should be at line number 8, the first line with user code. The assembly for the start of main looks like this: ... main: .LFB2: .loc 1 7 0 .cfi_startproc @ args = 0, pretend = 0, frame = 8 @ frame_needed = 1, uses_anonymous_args = 0 stmfd sp!, {fp, lr} .cfi_def_cfa_offset 8 .cfi_offset 11, -8 .cfi_offset 14, -4 add fp, sp, #4 .cfi_def_cfa 11, 4 sub sp, sp, #8 str r0, [fp, #-8] .loc 1 7 0 ldr r2, .L6 .LPIC0: add r2, pc, r2 .loc 1 8 0 ldr r1, [fp, #-8] ldr r3, .L6+4 cmp r1, r3 bne .L4 .loc 1 10 0 ... From the point of view of the debugger, in the presence of .loc info, the prologue is the code in between the 2 first .loc markers. See this comment in gdb/arm-tdep.c:arm_skip_prologue: ... /* GCC always emits a line note before the prologue and another one after, even if the two are at the same address or on the same line. Take advantage of this so that we do not need to know every instruction that might appear in the prologue. ... So gdb breaks at line 7, because it considers the prologue the code between the first 2 .locs, and the user code for main to start at the second .loc, which has line number 7. The code at the second .loc is the PIC register setup code, generated by require_pic_register: ... .loc 1 7 0 ldr r2, .L6 .LPIC0: add r2, pc, r2 ... The second .loc is emitted by gcc, because it's the first insn after NOTE_INSNS_FUNCTION_BEG (which sets final.c:force_source_line in final.c:final_scan_insn). The PIC register setup code is emitted after NOTE_INSNS_FUNCTION_BEG, because it uses the insert_insn_on_edge mechanism, and the corresponding insertion in cfgexpand.c:gimple_expand_cfg takes care to insert the code after the parm_birth_insn: ... /* Avoid putting insns before parm_birth_insn. */ if (e->src == ENTRY_BLOCK_PTR && single_succ_p (ENTRY_BLOCK_PTR) && parm_birth_insn) { rtx insns = e->insns.r; e->insns.r = NULL_RTX; emit_insn_after_noloc (insns, parm_birth_insn, e->dest); } ... And in the case for this test-case, parm_birth_insn is the NOTE_INSNS_FUNCTION_BEG. In summary, the problem seems to be caused by a discrepancy between: - the line number of the PIC register setup code (prologue_location), and - the location of the PIC register setup code (after NOTE_INSN_FUNCTION_BEG). The problem can be reproduced using Ulrich's example here ( http://gcc.gnu.org/ml/gcc-patches/2010-12/msg01570.html ), but though related it's not the same problem as explained there. There the wrong line number of the breakpoint was after the first user line. Here the wrong line number of the breakpoint is before the first user line. We can see from that email that this problem was not present after the fix. The problem was introduced by Jakub's fix for PR47028 (Insert entry edge insertions after parm_birth_insn instead of at the beginning of first bb), 12 days after Ulrich's fix. This patch makes sure we emit insertions scheduled for the first real BB before NOTE_INSN_FUNCTION_BEG. As a consequence, it moves the PIC register setup code to before the NOTE_INSN_FUNCTION_BEG. This removes the second .loc, and the breakpoint of main ends up at line 8. Bootstrapped and regtested on x86_64 (ada inclusive), no issues found. Tested gdb with target arm-none-linux-gnueabi and CFLAGS_FOR_TARGET=-fPIC. The patch removes 174 FAILs. Re-testing gcc with target arm-none-linux-gnueabi atm. OK for trunk? Thanks, - Tom 2013-10-13 Tom de Vries <tom@codesourcery.com> * cfgexpand.c (gimple_expand_cfg): Don't commit insertions after NOTE_INSN_FUNCTION_BEG. * gcc.target/arm/require-pic-register-loc.c: New test.