| Message ID | 20180716132909.633uvqxojzgg3wg6@delia |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,debug] Add -fadd-debug-nops | expand |
On Mon, Jul 16, 2018 at 03:29:10PM +0200, Tom de Vries wrote: > this is an idea that I'm currently playing around with: adding nops in > an optimized application with debug info can improve the debug info. > > Consider f.i. this gdb session in foo of pr54200-2.c (using -Os): > ... > (gdb) n > 26 return a; /* { dg-final { gdb-test . "(int)a" "6" } } */ > (gdb) p a > 'a' has unknown type; cast it to its declared type > (gdb) n > main () at pr54200-2.c:34 > 34 return 0; > ... > > The problem is that the scope in which a is declared ends at .LBE7, and the > statement .loc for line 26 ends up attached to the ret insn: > ... > .loc 1 24 11 > addl %edx, %eax > .LVL1: > # DEBUG a => ax > .loc 1 26 7 is_stmt 1 > .LBE7: > .loc 1 28 1 is_stmt 0 > ret > .cfi_endproc > ... > > This patch fixes the problem (for Og and Os, the 'DEBUG a => ax' is missing > for O1 and higher) by adding a nop before the ret insn: > ... > .loc 1 24 11 > addl %edx, %eax > .LVL1: > # DEBUG a => ax > .loc 1 26 7 is_stmt 1 > + nop > .LBE7: > .loc 1 28 1 is_stmt 0 > ret > .cfi_endproc > ... > > and instead we have: > ... > (gdb) n > 26 return a; /* { dg-final { gdb-test . "(int)a" "6" } } */ > (gdb) p a > $1 = 6 > (gdb) n > main () at pr54200-2.c:34 > 34 return 0; > ... > > Any comments? So is this essentially a workaround for GDB not supporting the statement frontiers? Isn't the right fix just to add that support instead and then users can choose how exactly they want to step through the function in the debugger. Jakub
On Mon, 16 Jul 2018, Tom de Vries wrote: > Hi, > > this is an idea that I'm currently playing around with: adding nops in > an optimized application with debug info can improve the debug info. > > > Consider f.i. this gdb session in foo of pr54200-2.c (using -Os): > ... > (gdb) n > 26 return a; /* { dg-final { gdb-test . "(int)a" "6" } } */ > (gdb) p a > 'a' has unknown type; cast it to its declared type > (gdb) n > main () at pr54200-2.c:34 > 34 return 0; > ... > > The problem is that the scope in which a is declared ends at .LBE7, and the > statement .loc for line 26 ends up attached to the ret insn: > ... > .loc 1 24 11 > addl %edx, %eax > .LVL1: > # DEBUG a => ax > .loc 1 26 7 is_stmt 1 > .LBE7: > .loc 1 28 1 is_stmt 0 > ret > .cfi_endproc > ... > > This patch fixes the problem (for Og and Os, the 'DEBUG a => ax' is missing > for O1 and higher) by adding a nop before the ret insn: > ... > .loc 1 24 11 > addl %edx, %eax > .LVL1: > # DEBUG a => ax > .loc 1 26 7 is_stmt 1 > + nop > .LBE7: > .loc 1 28 1 is_stmt 0 > ret > .cfi_endproc > ... > > and instead we have: > ... > (gdb) n > 26 return a; /* { dg-final { gdb-test . "(int)a" "6" } } */ > (gdb) p a > $1 = 6 > (gdb) n > main () at pr54200-2.c:34 > 34 return 0; > ... > > Any comments? Interesting idea. I wonder if that should be generalized to other places and how we can avoid compare-debug failures (var-tracking usually doesn't change code-generation). Richard. > Thanks, > - Tom > > [debug] Add -fadd-debug-nops > > --- > gcc/common.opt | 4 ++++ > gcc/testsuite/gcc.dg/guality/pr54200-2.c | 37 ++++++++++++++++++++++++++++++++ > gcc/var-tracking.c | 16 ++++++++++++++ > 3 files changed, 57 insertions(+) > > diff --git a/gcc/common.opt b/gcc/common.opt > index c29abdb5cb1..8e2876d2b8e 100644 > --- a/gcc/common.opt > +++ b/gcc/common.opt > @@ -1504,6 +1504,10 @@ Common Report Var(flag_hoist_adjacent_loads) Optimization > Enable hoisting adjacent loads to encourage generating conditional move > instructions. > > +fadd-debug-nops > +Common Report Var(flag_add_debug_nops) Optimization > +Add nops if that improves debugging of optimized code > + > fkeep-gc-roots-live > Common Undocumented Report Var(flag_keep_gc_roots_live) Optimization > ; Always keep a pointer to a live memory block > diff --git a/gcc/testsuite/gcc.dg/guality/pr54200-2.c b/gcc/testsuite/gcc.dg/guality/pr54200-2.c > new file mode 100644 > index 00000000000..2694f7401e2 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/guality/pr54200-2.c > @@ -0,0 +1,37 @@ > +/* { dg-do run } */ > +/* { dg-skip-if "" { *-*-* } { "*" } { "-Og" "-Os" "-O0" } } */ > +/* { dg-options "-g -fadd-debug-nops -DMAIN" } */ > + > +#include "prevent-optimization.h" > + > +int o ATTRIBUTE_USED; > + > +void > +bar (void) > +{ > + o = 2; > +} > + > +int __attribute__((noinline,noclone)) > +foo (int z, int x, int b) > +{ > + if (x == 1) > + { > + bar (); > + return z; > + } > + else > + { > + int a = (x + z) + b; > + /* Add cast to prevent unsupported. */ > + return a; /* { dg-final { gdb-test . "(int)a" "6" } } */ > + } > +} > + > +#ifdef MAIN > +int main () > +{ > + foo (3, 2, 1); > + return 0; > +} > +#endif > diff --git a/gcc/var-tracking.c b/gcc/var-tracking.c > index 5537fa64085..230845a38a0 100644 > --- a/gcc/var-tracking.c > +++ b/gcc/var-tracking.c > @@ -9994,6 +9994,15 @@ reemit_marker_as_note (rtx_insn *insn) > } > } > > +static void > +emit_nop_before (rtx_insn *insn) > +{ > + rtx_insn *insert_after = PREV_INSN (insn); > + while (INSN_LOCATION (insert_after) == INSN_LOCATION (insn)) > + insert_after = PREV_INSN (insert_after); > + emit_insn_after (gen_nop (), insert_after); > +} > + > /* Allocate and initialize the data structures for variable tracking > and parse the RTL to get the micro operations. */ > > @@ -10224,6 +10233,13 @@ vt_initialize (void) > continue; > } > > + /* Add debug nops before ret insn. */ > + if (optimize > + && flag_add_debug_nops > + && cfun->debug_nonbind_markers > + && returnjump_p (insn)) > + emit_nop_before (insn); > + > if (MAY_HAVE_DEBUG_BIND_INSNS) > { > if (CALL_P (insn)) > >
On 07/16/2018 03:34 PM, Jakub Jelinek wrote: > On Mon, Jul 16, 2018 at 03:29:10PM +0200, Tom de Vries wrote: >> this is an idea that I'm currently playing around with: adding nops in >> an optimized application with debug info can improve the debug info. >> >> Consider f.i. this gdb session in foo of pr54200-2.c (using -Os): >> ... >> (gdb) n >> 26 return a; /* { dg-final { gdb-test . "(int)a" "6" } } */ >> (gdb) p a >> 'a' has unknown type; cast it to its declared type >> (gdb) n >> main () at pr54200-2.c:34 >> 34 return 0; >> ... >> >> The problem is that the scope in which a is declared ends at .LBE7, and the >> statement .loc for line 26 ends up attached to the ret insn: >> ... >> .loc 1 24 11 >> addl %edx, %eax >> .LVL1: >> # DEBUG a => ax >> .loc 1 26 7 is_stmt 1 >> .LBE7: >> .loc 1 28 1 is_stmt 0 >> ret >> .cfi_endproc >> ... >> >> This patch fixes the problem (for Og and Os, the 'DEBUG a => ax' is missing >> for O1 and higher) by adding a nop before the ret insn: >> ... >> .loc 1 24 11 >> addl %edx, %eax >> .LVL1: >> # DEBUG a => ax >> .loc 1 26 7 is_stmt 1 >> + nop >> .LBE7: >> .loc 1 28 1 is_stmt 0 >> ret >> .cfi_endproc >> ... >> >> and instead we have: >> ... >> (gdb) n >> 26 return a; /* { dg-final { gdb-test . "(int)a" "6" } } */ >> (gdb) p a >> $1 = 6 >> (gdb) n >> main () at pr54200-2.c:34 >> 34 return 0; >> ... >> >> Any comments? > > So is this essentially a workaround for GDB not supporting the statement > frontiers? AFAIU now, the concept of location views addresses this problem, so yes. > Isn't the right fix just to add that support instead and then > users can choose how exactly they want to step through the function in the > debugger. Right, but in the mean time I don't mind having an option that lets me filter out noise in guality test results. Thanks, - Tom
On 07/16/2018 03:50 PM, Richard Biener wrote: > On Mon, 16 Jul 2018, Tom de Vries wrote: > >> Hi, >> >> this is an idea that I'm currently playing around with: adding nops in >> an optimized application with debug info can improve the debug info. >> >> >> Consider f.i. this gdb session in foo of pr54200-2.c (using -Os): >> ... >> (gdb) n >> 26 return a; /* { dg-final { gdb-test . "(int)a" "6" } } */ >> (gdb) p a >> 'a' has unknown type; cast it to its declared type >> (gdb) n >> main () at pr54200-2.c:34 >> 34 return 0; >> ... >> >> The problem is that the scope in which a is declared ends at .LBE7, and the >> statement .loc for line 26 ends up attached to the ret insn: >> ... >> .loc 1 24 11 >> addl %edx, %eax >> .LVL1: >> # DEBUG a => ax >> .loc 1 26 7 is_stmt 1 >> .LBE7: >> .loc 1 28 1 is_stmt 0 >> ret >> .cfi_endproc >> ... >> >> This patch fixes the problem (for Og and Os, the 'DEBUG a => ax' is missing >> for O1 and higher) by adding a nop before the ret insn: >> ... >> .loc 1 24 11 >> addl %edx, %eax >> .LVL1: >> # DEBUG a => ax >> .loc 1 26 7 is_stmt 1 >> + nop >> .LBE7: >> .loc 1 28 1 is_stmt 0 >> ret >> .cfi_endproc >> ... >> >> and instead we have: >> ... >> (gdb) n >> 26 return a; /* { dg-final { gdb-test . "(int)a" "6" } } */ >> (gdb) p a >> $1 = 6 >> (gdb) n >> main () at pr54200-2.c:34 >> 34 return 0; >> ... >> >> Any comments? > > Interesting idea. I wonder if that should be generalized > to other places I kept the option name general, to allow for that. And indeed, this is a point-fix patch. I've been playing around with a more generic patch that adds nops such that each is_stmt .loc is associated with a unique insn, but that was embedded in an fkeep-vars-live branch, so this patch is minimally addressing the first problem I managed to reproduce on trunk. > and how we can avoid compare-debug failures > (var-tracking usually doesn't change code-generation). > I could remove the cfun->debug_nonbind_markers test and move the nop-insertion to a separate pass or some such. Thanks, - Tom
On Jul 16, 2018, Tom de Vries <tdevries@suse.de> wrote: > On 07/16/2018 03:34 PM, Jakub Jelinek wrote: >> So is this essentially a workaround for GDB not supporting the statement >> frontiers? > AFAIU now, the concept of location views addresses this problem, so yes. Nice! A preview for what can be obtained with LVu support in the debugger! > Right, but in the mean time I don't mind having an option that lets me > filter out noise in guality test results. FWIW, I'm a bit concerned about working around legitimate problems, as in modifying testcases so that they pass. This hides actual problems, that we'd like to fix eventually by adjusting the compiler, not the testcases. That said, thank you for the attention you've given to the guality testsuite recently. It's appreciated.
On 07/16/2018 05:10 PM, Tom de Vries wrote: > On 07/16/2018 03:50 PM, Richard Biener wrote: >> On Mon, 16 Jul 2018, Tom de Vries wrote: >>> Any comments? >> >> Interesting idea. I wonder if that should be generalized >> to other places > > I kept the option name general, to allow for that. > > And indeed, this is a point-fix patch. I've been playing around with a > more generic patch that adds nops such that each is_stmt .loc is > associated with a unique insn, but that was embedded in an > fkeep-vars-live branch, so this patch is minimally addressing the first > problem I managed to reproduce on trunk. > >> and how we can avoid compare-debug failures >> (var-tracking usually doesn't change code-generation). >> > I'll post this patch series (the current state of my fkeep-vars-live branch) in reply to this email: 1 [debug] Add fdebug-nops 2 [debug] Add fkeep-vars-live 3 [debug] Add fdebug-nops and fkeep-vars-live to Og only Bootstrapped and reg-tested on x86_64. ChangeLog entries and function header comments missing. Comments welcome. Thanks, - Tom
diff --git a/gcc/common.opt b/gcc/common.opt index c29abdb5cb1..8e2876d2b8e 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -1504,6 +1504,10 @@ Common Report Var(flag_hoist_adjacent_loads) Optimization Enable hoisting adjacent loads to encourage generating conditional move instructions. +fadd-debug-nops +Common Report Var(flag_add_debug_nops) Optimization +Add nops if that improves debugging of optimized code + fkeep-gc-roots-live Common Undocumented Report Var(flag_keep_gc_roots_live) Optimization ; Always keep a pointer to a live memory block diff --git a/gcc/testsuite/gcc.dg/guality/pr54200-2.c b/gcc/testsuite/gcc.dg/guality/pr54200-2.c new file mode 100644 index 00000000000..2694f7401e2 --- /dev/null +++ b/gcc/testsuite/gcc.dg/guality/pr54200-2.c @@ -0,0 +1,37 @@ +/* { dg-do run } */ +/* { dg-skip-if "" { *-*-* } { "*" } { "-Og" "-Os" "-O0" } } */ +/* { dg-options "-g -fadd-debug-nops -DMAIN" } */ + +#include "prevent-optimization.h" + +int o ATTRIBUTE_USED; + +void +bar (void) +{ + o = 2; +} + +int __attribute__((noinline,noclone)) +foo (int z, int x, int b) +{ + if (x == 1) + { + bar (); + return z; + } + else + { + int a = (x + z) + b; + /* Add cast to prevent unsupported. */ + return a; /* { dg-final { gdb-test . "(int)a" "6" } } */ + } +} + +#ifdef MAIN +int main () +{ + foo (3, 2, 1); + return 0; +} +#endif diff --git a/gcc/var-tracking.c b/gcc/var-tracking.c index 5537fa64085..230845a38a0 100644 --- a/gcc/var-tracking.c +++ b/gcc/var-tracking.c @@ -9994,6 +9994,15 @@ reemit_marker_as_note (rtx_insn *insn) } } +static void +emit_nop_before (rtx_insn *insn) +{ + rtx_insn *insert_after = PREV_INSN (insn); + while (INSN_LOCATION (insert_after) == INSN_LOCATION (insn)) + insert_after = PREV_INSN (insert_after); + emit_insn_after (gen_nop (), insert_after); +} + /* Allocate and initialize the data structures for variable tracking and parse the RTL to get the micro operations. */ @@ -10224,6 +10233,13 @@ vt_initialize (void) continue; } + /* Add debug nops before ret insn. */ + if (optimize + && flag_add_debug_nops + && cfun->debug_nonbind_markers + && returnjump_p (insn)) + emit_nop_before (insn); + if (MAY_HAVE_DEBUG_BIND_INSNS) { if (CALL_P (insn))
Hi, this is an idea that I'm currently playing around with: adding nops in an optimized application with debug info can improve the debug info. Consider f.i. this gdb session in foo of pr54200-2.c (using -Os): ... (gdb) n 26 return a; /* { dg-final { gdb-test . "(int)a" "6" } } */ (gdb) p a 'a' has unknown type; cast it to its declared type (gdb) n main () at pr54200-2.c:34 34 return 0; ... The problem is that the scope in which a is declared ends at .LBE7, and the statement .loc for line 26 ends up attached to the ret insn: ... .loc 1 24 11 addl %edx, %eax .LVL1: # DEBUG a => ax .loc 1 26 7 is_stmt 1 .LBE7: .loc 1 28 1 is_stmt 0 ret .cfi_endproc ... This patch fixes the problem (for Og and Os, the 'DEBUG a => ax' is missing for O1 and higher) by adding a nop before the ret insn: ... .loc 1 24 11 addl %edx, %eax .LVL1: # DEBUG a => ax .loc 1 26 7 is_stmt 1 + nop .LBE7: .loc 1 28 1 is_stmt 0 ret .cfi_endproc ... and instead we have: ... (gdb) n 26 return a; /* { dg-final { gdb-test . "(int)a" "6" } } */ (gdb) p a $1 = 6 (gdb) n main () at pr54200-2.c:34 34 return 0; ... Any comments? Thanks, - Tom [debug] Add -fadd-debug-nops --- gcc/common.opt | 4 ++++ gcc/testsuite/gcc.dg/guality/pr54200-2.c | 37 ++++++++++++++++++++++++++++++++ gcc/var-tracking.c | 16 ++++++++++++++ 3 files changed, 57 insertions(+)