On Tue, Jul 24, 2018 at 01:30:30PM +0200, Tom de Vries wrote: > 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. >
Consider 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 after the ".loc is_stmt 1" annotation: ... .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; ... The option should have the same effect as using a debugger with location views capability. There's a design principle in GCC that code generation and debug generation are independent. This guarantees that if you're encountering a problem in an application without debug info, you can recompile it with -g and be certain that you can reproduce the same problem, and use the debug info to debug the problem. This invariant is enforced by bootstrap-debug. The fdebug-nops breaks this invariant, but we consider this acceptable since it is intended for use in combination with -Og -g in the standard edit-compile-debug cycle, where -g is assumed to be already present at the point of encountering a problem. PR debug/78685 [debug] Add fdebug-nops --- gcc/common.opt | 4 +++ gcc/testsuite/gcc.dg/guality/pr54200-2.c | 37 +++++++++++++++++++++++++ gcc/var-tracking.c | 46 ++++++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+) diff --git a/gcc/common.opt b/gcc/common.opt index 4bf8de90331..984b351cd79 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -1492,6 +1492,10 @@ Common Report Var(flag_hoist_adjacent_loads) Optimization Enable hoisting adjacent loads to encourage generating conditional move instructions. +fdebug-nops +Common Report Var(flag_debug_nops) Optimization +Insert nops if that might improve 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..e30e3c92b94 --- /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 -fdebug-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..1349454d5f2 100644 --- a/gcc/var-tracking.c +++ b/gcc/var-tracking.c @@ -10427,6 +10427,49 @@ vt_finalize (void) vui_allocated = 0; } +static void +insert_debug_nops (void) +{ + rtx_insn *debug_marker = NULL; + + for (rtx_insn *insn = get_insns (); insn; insn = NEXT_INSN (insn)) + { + bool use_p = INSN_P (insn) && GET_CODE (PATTERN (insn)) == USE; + bool clobber_p = INSN_P (insn) && GET_CODE (PATTERN (insn)) == CLOBBER; + + if (!debug_marker) + { + if (DEBUG_MARKER_INSN_P (insn)) + debug_marker = insn; + continue; + } + + if (LABEL_P (insn) || BARRIER_P (insn) || DEBUG_MARKER_INSN_P (insn)) + ; + else if (use_p || clobber_p || NOTE_P (insn) || DEBUG_INSN_P (insn) + || JUMP_TABLE_DATA_P (insn)) + continue; + else if (INSN_P (insn)) + { + unsigned int debug_marker_loc = INSN_LOCATION (debug_marker); + unsigned int insn_loc = INSN_LOCATION (insn); + + if (LOCATION_FILE (insn_loc) == LOCATION_FILE (debug_marker_loc) + && LOCATION_LINE (insn_loc) == LOCATION_LINE (debug_marker_loc)) + { + debug_marker = NULL; + continue; + } + } + else + gcc_unreachable (); + + emit_insn_after_setloc (gen_nop (), debug_marker, + INSN_LOCATION (debug_marker)); + debug_marker = DEBUG_MARKER_INSN_P (insn) ? insn : NULL; + } +} + /* The entry point to variable tracking pass. */ static inline unsigned int @@ -10434,6 +10477,9 @@ variable_tracking_main_1 (void) { bool success; + if (flag_debug_nops) + insert_debug_nops (); + /* We won't be called as a separate pass if flag_var_tracking is not set, but final may call us to turn debug markers into notes. */ if ((!flag_var_tracking && MAY_HAVE_DEBUG_INSNS)