[RFC,debug] Add -fadd-debug-nops

Message ID 20180716132909.633uvqxojzgg3wg6@delia
State New
Headers show
Series
  • [RFC,debug] Add -fadd-debug-nops
Related show

Commit Message

Tom de Vries July 16, 2018, 1:29 p.m.
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(+)

Comments

Jakub Jelinek July 16, 2018, 1:34 p.m. | #1
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
Richard Biener July 16, 2018, 1:50 p.m. | #2
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))

> 

> 


-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
Tom de Vries July 16, 2018, 2:32 p.m. | #3
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
Tom de Vries July 16, 2018, 3:10 p.m. | #4
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
Alexandre Oliva July 17, 2018, 1:01 a.m. | #5
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.

-- 
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, 11:30 a.m. | #6
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

Patch

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