diff mbox

[PATCHES,PING*5] Enhance standard DWARF for Ada

Message ID 20160225094803.GN3017@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Feb. 25, 2016, 9:48 a.m. UTC
On Wed, Dec 16, 2015 at 09:53:37AM +0100, Pierre-Marie de Rodat wrote:
> On 12/11/2015 09:25 PM, Jason Merrill wrote:
> >Hmm, can we generate the DWARF procedures during finalize_size_functions
> >to avoid the need for preserve_body?
> 
> Good idea, thank you! Here’s the updated patch (bootstrapped and regtested
> on x86_64-linux, as usual).

Unfortunately, this broke the DW_OP_GNU_implicit_pointer support, on vast
majority of binaries and libraries gcc now emits invalid DWARF (which both
gdb and dwz complain about and dwz refuses to optimize because of that).

I'm attaching two possible patches, so far untested.

The first one just fixes what I mainly care about, the committed patch
assumed that DW_TAG_dwarf_procedure is always only created for the Ada
variable sized structures or whatever it was meant for, which is not the
case, and thus if we emit DW_TAG_dwarf_procedure for some other reason,
it would be pruned as unused even when it is actually used (and result in
a DIE reference to the compilation unit header, which is always invalid).

But, looking at the points where you use DW_TAG_dwarf_procedure for the Ada
things, I can't see how it can actually work at all, though there is no
testsuite coverage, so it is hard to find out for real.
The thing is, current code sets die_perennial_p on type DIEs and their
parents, but nothing else.  In particular, type DIEs are identified by
being returned from lookup_type_die, thus earlier passed to
equate_type_number_to_die.  I don't see that this would ever be the case
of DW_TAG_dwarf_procedure though, I see the return of
function_to_dwarf_procedure being used as dw_loc_oprnd1.v.val_die_ref.die
of a DW_OP_call4 that is somewhere used in some location description that is
perhaps used somewhere in some type DIE computation.
Thus, I'm afraid for Ada variable sized structures you get the same problem,
you might IMHO DW_OP_call4 .Ldebug_info0 + 0 because the
DW_TAG_dwarf_procedure will be pruned as "unused".  So IMHO the second patch
makes more sense, and if you (for GCC 7?) want to prune really unused
DW_TAG_dwarf_procedure, you need to add code that will really walk all of
the debuginfo, rather than just type DIEs themselves, and look if location
descriptions (in .debug_info or .debug_loc) reference those
DW_TAG_dwarf_procedure and mark the DW_TAG_dwarf_procedure.

So, Pierre-Marie, can I ask you to run whatever Ada debug info testsuite
you have with the second patch?  And for GCC 7 really please consider adding
gnat.dg/guality/ and fill it with tests.

	Jakub
2016-02-25  Jakub Jelinek  <jakub@redhat.com>

	PR debug/69947
	* dwarf2out.c (string_cst_pool_decl): Set die_perennial_p on
	the DW_TAG_dwarf_procedure DIE.

	* gcc.dg/guality/pr69947.c: New test.
2016-02-25  Jakub Jelinek  <jakub@redhat.com>

	PR debug/69947
	* dwarf2out.c (prune_unused_types_walk): Don't prune
	DW_TAG_dwarf_procedure.

	* gcc.dg/guality/pr69947.c: New test.

--- gcc/dwarf2out.c.jj	2016-02-24 23:03:32.000000000 +0100
+++ gcc/dwarf2out.c	2016-02-25 10:08:46.688716691 +0100
@@ -25853,11 +25853,6 @@ prune_unused_types_walk (dw_die_ref die)
     case DW_TAG_file_type:
       /* Type nodes are useful only when other DIEs reference them --- don't
 	 mark them.  */
-      /* FALLTHROUGH */
-
-    case DW_TAG_dwarf_procedure:
-      /* Likewise for DWARF procedures.  */
-
       if (die->die_perennial_p)
 	break;
 
--- gcc/testsuite/gcc.dg/guality/pr69947.c.jj	2016-02-25 10:00:25.503608176 +0100
+++ gcc/testsuite/gcc.dg/guality/pr69947.c	2016-02-25 10:05:20.446552599 +0100
@@ -0,0 +1,22 @@
+/* PR debug/69947 */
+/* { dg-do run } */
+/* { dg-options "-g" } */
+
+#include "../nop.h"
+
+static const char *c = "foobar";
+
+__attribute__((noinline, noclone)) void
+foo (void)
+{
+  static const char a[] = "abcdefg";
+  const char *b = a;		/* { dg-final { gdb-test 14 "c\[2\]" "'o'" } } */
+  asm (NOP : : : "memory");	/* { dg-final { gdb-test 14 "b\[4\]" "'e'" } } */
+}
+
+int
+main ()
+{
+  foo ();
+  return 0;
+}

Comments

Pierre-Marie de Rodat Feb. 25, 2016, 10:35 a.m. UTC | #1
On 02/25/2016 10:48 AM, Jakub Jelinek wrote:
> Unfortunately, this broke the DW_OP_GNU_implicit_pointer support, on vast
> majority of binaries and libraries gcc now emits invalid DWARF (which both
> gdb and dwz complain about and dwz refuses to optimize because of that).

Arg, sorry about this!

> I'm attaching two possible patches, so far untested.

Thanks, I’m having a look as we speak.

> So, Pierre-Marie, can I ask you to run whatever Ada debug info testsuite
> you have with the second patch?  And for GCC 7 really please consider adding
> gnat.dg/guality/ and fill it with tests.

Testing in progress…

I have a tiny Python/pyelftools-based testsuite that checks the DIE 
patterns GCC emits for several Ada types. I really wish I could somehow 
integrate them to the GCC testsuite, but right now I don’t know how I 
could do similar things, there.

As I said at the end of a message in another thread 
(https://gcc.gnu.org/ml/gcc-patches/2016-01/msg01078.html), I always 
feel uncomfortable writing brittle dg-scan testcases, hence the current 
lack of testcases for those DWARF changes.
Jakub Jelinek Feb. 25, 2016, 10:45 a.m. UTC | #2
On Thu, Feb 25, 2016 at 11:35:07AM +0100, Pierre-Marie de Rodat wrote:
> As I said at the end of a message in another thread
> (https://gcc.gnu.org/ml/gcc-patches/2016-01/msg01078.html), I always feel
> uncomfortable writing brittle dg-scan testcases, hence the current lack of
> testcases for those DWARF changes.

I agree that catching this in scan-assembler test is hard, but guality test
would catch this.  It is true that some guality tests (mostly the ones that
test behaviour of optimized code, which differs a lot between different
architectures) have known FAILs (or known XFAILs), because the target,
compilation options and gdb version matrix is too large to catch all cases.
But, if one just looks at test_summary output before/after a change,
one can detect regressions and fix them.  Perhaps in some cases you could
just limit to -O0 guality if it is something you want to just check if
the debug info is represented properly and don't need to test
-fvar-tracking-assignments etc. handling - you can just dg-skip-if the tests
except for -O0 etc.
Similarly, it shouldn't be hard to add tcl function to check for gdb
version, and limit some tests only to a particular version of gdb or newer.

	Jakub
Eric Botcazou Feb. 25, 2016, 12:24 p.m. UTC | #3
> I agree that catching this in scan-assembler test is hard, but guality test
> would catch this.  It is true that some guality tests (mostly the ones that
> test behaviour of optimized code, which differs a lot between different
> architectures) have known FAILs (or known XFAILs), because the target,
> compilation options and gdb version matrix is too large to catch all cases.

IMO the guality testsuite is not really appropriate for debug info issues, 
it's too brittle, has a low signal-over-noise ratio and nobody really cares 
about it.  And, given that most people already don't care about the regular 
gnat.dg testsuite, I think that literally nobody will about gnat.dg/guality.

Given that only AdaCore's folks work on Ada debug info issues in practice and 
that they run the GDB testsuite, I don't see any real need for it.
Jakub Jelinek Feb. 25, 2016, 3:51 p.m. UTC | #4
On Thu, Feb 25, 2016 at 11:35:07AM +0100, Pierre-Marie de Rodat wrote:
> On 02/25/2016 10:48 AM, Jakub Jelinek wrote:
> >Unfortunately, this broke the DW_OP_GNU_implicit_pointer support, on vast
> >majority of binaries and libraries gcc now emits invalid DWARF (which both
> >gdb and dwz complain about and dwz refuses to optimize because of that).
> 
> Arg, sorry about this!
> 
> >I'm attaching two possible patches, so far untested.
> 
> Thanks, I’m having a look as we speak.
> 
> >So, Pierre-Marie, can I ask you to run whatever Ada debug info testsuite
> >you have with the second patch?  And for GCC 7 really please consider adding
> >gnat.dg/guality/ and fill it with tests.
> 
> Testing in progress…
> 
> I have a tiny Python/pyelftools-based testsuite that checks the DIE patterns
> GCC emits for several Ada types. I really wish I could somehow integrate
> them to the GCC testsuite, but right now I don’t know how I could do similar
> things, there.

Do you have some short Ada testcase where the DW_OP_call4 referring to
DW_TAG_dwarf_procedure is supposed to be emitted?  I believe you must be
getting there the .Ldebug_info0+0 invalid reference in the DW_OP_call4
operand.

	Jakub
Pierre-Marie de Rodat Feb. 25, 2016, 3:59 p.m. UTC | #5
On 02/25/2016 04:51 PM, Jakub Jelinek wrote:
> Do you have some short Ada testcase where the DW_OP_call4 referring to
> DW_TAG_dwarf_procedure is supposed to be emitted?  I believe you must be
> getting there the .Ldebug_info0+0 invalid reference in the DW_OP_call4
> operand.

Sure! Here’s one:
> $ gcc -S -g -fgnat-encodings=minimal -dA foo.adb && grep DW_OP_call4 foo.s
> foo.s:313:      .byte   0x99    # DW_OP_call4
diff mbox

Patch

--- gcc/dwarf2out.c.jj	2016-02-24 23:03:32.000000000 +0100
+++ gcc/dwarf2out.c	2016-02-25 10:08:46.688716691 +0100
@@ -26288,6 +26288,7 @@  string_cst_pool_decl (tree t)
       l->dw_loc_oprnd2.v.val_vec.elt_size = 1;
       l->dw_loc_oprnd2.v.val_vec.array = array;
       add_AT_loc (ref, DW_AT_location, l);
+      ref->die_perennial_p = 1;
       equate_decl_number_to_die (decl, ref);
     }
   return rtl;
--- gcc/testsuite/gcc.dg/guality/pr69947.c.jj	2016-02-25 10:00:25.503608176 +0100
+++ gcc/testsuite/gcc.dg/guality/pr69947.c	2016-02-25 10:05:20.446552599 +0100
@@ -0,0 +1,22 @@ 
+/* PR debug/69947 */
+/* { dg-do run } */
+/* { dg-options "-g" } */
+
+#include "../nop.h"
+
+static const char *c = "foobar";
+
+__attribute__((noinline, noclone)) void
+foo (void)
+{
+  static const char a[] = "abcdefg";
+  const char *b = a;		/* { dg-final { gdb-test 14 "c\[2\]" "'o'" } } */
+  asm (NOP : : : "memory");	/* { dg-final { gdb-test 14 "b\[4\]" "'e'" } } */
+}
+
+int
+main ()
+{
+  foo ();
+  return 0;
+}