Re: [PATCH, ARM] Fix line number data for PIC register setup code
Updated patch, re-bootstrapped on x86_64 and committed to trunk. Also applied to 4.7 and 4.8 branches, the same problem is present there. You only asked for approval on trunk though, and I'm not sure we really care about the results of the GDB testsuite on the 4.7 branch at this point, so let's exclude the 4.7 branch here. -- Eric Botcazou
Re: [PATCH, ARM] Fix line number data for PIC register setup code
On 29/10/13 09:12, Eric Botcazou wrote: Updated patch, re-bootstrapped on x86_64 and committed to trunk. Also applied to 4.7 and 4.8 branches, the same problem is present there. You only asked for approval on trunk though, Eric, Sorry about that. and I'm not sure we really care about the results of the GDB testsuite on the 4.7 branch at this point, FWIW, I originally encountered the problem on 4.7. so let's exclude the 4.7 branch here. Reverted on the 4.7 branch. Thanks, - Tom
Re: [PATCH, ARM] Fix line number data for PIC register setup code
Reverted on the 4.7 branch. Thanks. -- Eric Botcazou
Re: [PATCH, ARM] Fix line number data for PIC register setup code
On 27/10/13 17:04, Eric Botcazou wrote:
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?
Yes, AFAICT parm_birth_insn is never NULL at this point.
2013-10-13 Tom de Vries t...@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.
Updated patch, re-bootstrapped on x86_64 and committed to trunk.
Also applied to 4.7 and 4.8 branches, the same problem is present there.
Thanks,
- Tom
2013-10-28 Tom de Vries t...@codesourcery.com
* cfgexpand.c (gimple_expand_cfg): Remove test for parm_birth_insn.
Don't commit insertions after NOTE_INSN_FUNCTION_BEG.
* gcc.target/arm/require-pic-register-loc.c: New test.
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index ba4c0e6..9705036 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -4789,14 +4789,18 @@ 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)
+ single_succ_p (ENTRY_BLOCK_PTR))
{
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);
2013-10-27 Tobias Burnus bur...@net-b.de
PR other/33426
diff --git a/gcc/testsuite/gcc.target/arm/require-pic-register-loc.c b/gcc/testsuite/gcc.target/arm/require-pic-register-loc.c
new file mode 100644
index 000..bd85e86
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/require-pic-register-loc.c
@@ -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 } } */
[PING] [PATCH, ARM] Fix line number data for PIC register setup code
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
Re: [PATCH, ARM] Fix line number data for PIC register setup code
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 t...@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.
--
Eric Botcazou
Re: [PATCH, ARM] Fix line number data for PIC register setup code
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
Re: Fix line number data for PIC register setup code
On 03/10/13 17:17, Tom de Vries wrote:
we need to emit it before the FUNCTION_BEG insn-note
(rough proof-of-concept patch attached), such that no .loc will be generated
for
it.
I investigated further, and now I think it's a regression caused by the fix for
PR47028.
Attached patch works for the testcase. I'll test the patch, and if successful do
a write-up and submit to gcc-patches.
Thanks,
- Tom
2013-10-13 Tom de Vries t...@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.
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 } } */
[PATCH, ARM] Fix line number data for PIC register setup code
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 {
8if (argc == 12345)
9 {
10a (v);
11return 1;
12 }
13b ();
14
15return 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 t...@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.
Index: gcc/cfgexpand.c
===
--- gcc/cfgexpand.c (revision 421892)
+++ gcc/cfgexpand.c (working copy)
@@ -4618,14 +4618,19 @@ gimple_expand_cfg (void)
Fix line number data for PIC register setup code
Richard,
( see also related discussion
http://gcc.gnu.org/ml/gcc-patches/2010-12/msg01570.html )
Consider break.c (minimized from gdb/testsuite/gdb.base/break.c):
...
void *v;
void a (void *x) { }
void b (void) { }
int
main (int argc)
{
if (argc == 12345)
{
a (v);
return 1;
}
b ();
return 0;
}
...
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 {
...
When we compile with -fno-PIC, we break at another line:
...
(gdb) b main
Breakpoint 1 at 0x83f8: file break.c, line 8.
(gdb) continue
Continuing.
Breakpoint 1, main (argc=1) at break.c:8
8 if (argc == 12345)
...
AFAIU, the correct line number is 8, so the case with -fPIC needs to be fixed.
The assembly looks like this:
...
main:
.LFB0:
.file 1 break.c
.loc 1 88 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 88 0
ldr r2, .L4
.LPIC0:
add r2, pc, r2
.loc 1 89 0
ldr r1, [fp, #-8]
ldr r3, .L4+4
cmp r1, r3
bne .L2
.loc 1 91 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.
...
Manually removing the second '.loc 1 88 0' (in the pic register setup generated
by require_pic_register) from the assembly gives us the required behaviour.
An easy way to achieve this in the compiler is this patch:
...
Index: config/arm/arm.c
===
--- config/arm/arm.c(revision 202646)
+++ config/arm/arm.c(working copy)
@@ -5542,9 +5542,12 @@ require_pic_register (void)
seq = get_insns ();
end_sequence ();
for (insn = seq; insn; insn = NEXT_INSN (insn))
if (INSN_P (insn))
- INSN_LOCATION (insn) = prologue_location;
+ INSN_LOCATION (insn) = UNKNOWN_LOCATION;
/* We can be called during expansion of PHI nodes, where
we can't yet emit instructions directly in the final
...
However, it looks to me somewhat contradictory that when we mark this code as
UNKNOWN_LOCATION gdb recognized it as part of the prologue, and when we mark it
with prologue_location, gdb doesn't recognize it as part of the prologue.
Looking in more detail why we're emitting the .loc, it's because the pic
register setup follows the FUNCTION_BEG insn-note, which forces a .loc on the
next insn using the final.c:force_source_line variable.
According to this comment in dwarf2out_source_line:
...
... NOTE_INSN_FUNCTION_BEG, i.e. the first
insn that corresponds to something the user wrote.
...
NOTE_INSN_FUNCTION_BEG marks the beginning of user code. To me it seems that we
have to make a choice here: Either the pic register setup code generated by
require_pic_register is user code, or it is prologue code.
If it's prologue code, we need to emit it before the FUNCTION_BEG insn-note
(rough proof-of-concept patch attached), such that no .loc will be generated for
it. Gdb will recognize it as part of the prologue, and the breakpoint will have
the correct line number.
It it's user code, we need to mark it with the first user line, such that a .loc
with the first user line will be generated. Gdb then won't count it as part of
the prologue, and the breakpoint will have the correct line number.
My preference would be to mark it as prologue code, since that's the case for
other uses of arm_load_pic_register.
What is the proper way to fix this?
Thanks,
- Tom
2013-09-15 Tom de Vries t...@codesourcery.com
gcc/
* cfgexpand.c (gimple_expand_cfg): Emit insns_before_parm_birth_insn.
* function.h (struct rtl_data): Add x_insns_before_parm_birth_insn
field.
(insns_before_parm_birth_insn): Define new macro.
* config/arm/arm.c (require_pic_register): Use
insns_before_parm_birth_insn.
Index: gcc/cfgexpand.c
===
--- gcc/cfgexpand.c (revision 418548)
+++ gcc/cfgexpand.c (working copy)
@@ -4609,6 +4609,10 @@ gimple_expand_cfg (void)
split edges which
