[RFC,1/3,debug] Add fdebug-nops

Message ID 20180724113514.6v6ngt67n333eia7@delia
State New
Headers show
Series
  • [RFC,1/3,debug] Add fdebug-nops
Related show

Commit Message

Tom de Vries July 24, 2018, 11:35 a.m.
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(+)

Comments

Alexandre Oliva July 24, 2018, 7:06 p.m. | #1
On Jul 24, 2018, Tom de Vries <tdevries@suse.de> wrote:

> 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


I thought of a way to not break it: enable the debug info generation
machinery, including VTA and SFN, but discard those only at the very end
if -g is not enabled.  The downside is that it would likely slow -Og
down significantly, but who uses it without -g anyway?

-- 
Alexandre Oliva, freedom fighter   https://FSFLA.org/blogs/lxo
Be the change, be Free!         FSF Latin America board member
GNU Toolchain Engineer                Free Software Evangelist
Tom de Vries July 24, 2018, 9:04 p.m. | #2
On 07/24/2018 09:06 PM, Alexandre Oliva wrote:
> On Jul 24, 2018, Tom de Vries <tdevries@suse.de> wrote:

> 

>> 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

> 

> I thought of a way to not break it: enable the debug info generation

> machinery, including VTA and SFN, but discard those only at the very end

> if -g is not enabled.  The downside is that it would likely slow -Og

> down significantly, but who uses it without -g anyway?


I thought of the same.  I've submitted a patch here that uses SFN:
https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01391.html . VTA is not
needed AFAIU.

Thanks,
- Tom
Alexandre Oliva July 26, 2018, 7:50 a.m. | #3
On Jul 24, 2018, Tom de Vries <tdevries@suse.de> wrote:

>> I thought of a way to not break it: enable the debug info generation

>> machinery, including VTA and SFN, but discard those only at the very end

>> if -g is not enabled.  The downside is that it would likely slow -Og

>> down significantly, but who uses it without -g anyway?


> I thought of the same.  I've submitted a patch here that uses SFN:

> https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01391.html .


Nice!

> VTA is not needed AFAIU.


Yes, indeed.  It could avoid inserting some nops, if you were to refrain
from emitting them if there aren't any binds between neighbor SFNs, but
I like it better your way: it's even more like SFN support in the
debugger :-)

-- 
Alexandre Oliva, freedom fighter   https://FSFLA.org/blogs/lxo
Be the change, be Free!         FSF Latin America board member
GNU Toolchain Engineer                Free Software Evangelist

Patch

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)