Re: [PING*2][PATCH] DWARF: add abstract origin links on lexical blocks DIEs

[Pierre-Marie de Rodat]

On 05/09/2016 01:01 PM, Richard Biener wrote:

Ok and sorry for the delay.


No problem. :-) Thanks, this is committed!

--
Pierre-Marie de Rodat

Re: [PING*2][PATCH] DWARF: add abstract origin links on lexical blocks DIEs

[Richard Biener]

On Wed, May 4, 2016 at 4:23 PM, Pierre-Marie de Rodat
 wrote:
> Ping for the patch submitted at
> . It applies just
> fine on the current trunk and still bootstrapps and regtests successfuly on
> x86_64-linux.
>
> Thank you in advance,

Ok and sorry for the delay.

Thanks,
Richard.

> --
> Pierre-Marie de Rodat

[PING*2][PATCH] DWARF: add abstract origin links on lexical blocks DIEs

[Pierre-Marie de Rodat]

Ping for the patch submitted at 
. It applies 
just fine on the current trunk and still bootstrapps and regtests 
successfuly on x86_64-linux.


Thank you in advance,

--
Pierre-Marie de Rodat

[PING][PATCH] DWARF: add abstract origin links on lexical blocks DIEs

[Pierre-Marie de Rodat]

Ping for the patch submitted at 
. It applies 
just fine on the current trunk and still bootstrapps and regtests 
successfuly on x86_64-linux.


Thank you in advance,

--
Pierre-Marie de Rodat

Re: [PATCH] DWARF: add abstract origin links on lexical blocks DIEs

[Pierre-Marie de Rodat]

On 01/18/2016 10:47 AM, Pierre-Marie de Rodat wrote:

Thank you for your inputs! I’m going to try that, then. I hope this test
will not be too fragile…


Here it is! Re-bootstrapped and regtested successfuly on x86_64-linux. 
I’ve checked that the testcase fails on the mainline.


--
Pierre-Marie de Rodat
>From 451d62ff871734727b0f0f570f89b6cfbed922f2 Mon Sep 17 00:00:00 2001
From: Pierre-Marie de Rodat <dero...@adacore.com>
Date: Tue, 12 Jan 2016 14:50:33 +0100
Subject: [PATCH] DWARF: add abstract origin links on lexical blocks DIEs

Track from which abstract lexical block concrete ones come from in DWARF
so that debuggers can inherit the former from the latter. This enables
debuggers to properly handle the following case:

  * function Child2 is nested in a lexical block, itself nested in
function Child1;
  * function Child1 is inlined into some call site;
  * function Child2 is never inlined.

Here, Child2 is described in DWARF only in the abstract instance of
Child1. So when debuggers decode Child1's concrete instances, they need
to fetch the definition for Child2 in the corresponding abstract
instance: the DW_AT_abstract_origin link on the lexical block that
embeds Child1 enables them to do that.

Bootstrapped and regtested on x86_64-linux.

gcc/ChangeLog:

	* dwarf2out.c (add_abstract_origin_attribute): Adjust
	documentation comment.  For BLOCK nodes, add a
	DW_AT_abstract_origin attribute that points to the DIE generated
	for the origin BLOCK.
	(gen_lexical_block_die): Call add_abstract_origin_attribute for
	blocks from inlined functions.

gcc/testsuite/Changelog:

	* gcc.dg/debug/dwarf2/nested_fun.c: New testcase.
---
 gcc/dwarf2out.c| 13 --
 gcc/testsuite/gcc.dg/debug/dwarf2/nested_fun.c | 65 ++
 2 files changed, 75 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/debug/dwarf2/nested_fun.c

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index f742900..d1503ec 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -18470,15 +18470,16 @@ add_prototyped_attribute (dw_die_ref die, tree func_type)
 }
 
 /* Add an 'abstract_origin' attribute below a given DIE.  The DIE is found
-   by looking in either the type declaration or object declaration
-   equate table.  */
+   by looking in the type declaration, the object declaration equate table or
+   the block mapping.  */
 
 static inline dw_die_ref
 add_abstract_origin_attribute (dw_die_ref die, tree origin)
 {
   dw_die_ref origin_die = NULL;
 
-  if (TREE_CODE (origin) != FUNCTION_DECL)
+  if (TREE_CODE (origin) != FUNCTION_DECL
+  && TREE_CODE (origin) != BLOCK)
 {
   /* We may have gotten separated from the block for the inlined
 	 function, if we're in an exception handler or some such; make
@@ -18500,6 +18501,8 @@ add_abstract_origin_attribute (dw_die_ref die, tree origin)
 origin_die = lookup_decl_die (origin);
   else if (TYPE_P (origin))
 origin_die = lookup_type_die (origin);
+  else if (TREE_CODE (origin) == BLOCK)
+origin_die = BLOCK_DIE (origin);
 
   /* XXX: Functions that are never lowered don't always have correct block
  trees (in the case of java, they simply have no block tree, in some other
@@ -21307,6 +21310,10 @@ gen_lexical_block_die (tree stmt, dw_die_ref context_die)
 	  BLOCK_DIE (stmt) = stmt_die;
 	  old_die = NULL;
 	}
+
+  tree origin = block_ultimate_origin (stmt);
+  if (origin != NULL_TREE && origin != stmt)
+	add_abstract_origin_attribute (stmt_die, origin);
 }
 
   if (old_die)
diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/nested_fun.c b/gcc/testsuite/gcc.dg/debug/dwarf2/nested_fun.c
new file mode 100644
index 000..c783ac0
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/debug/dwarf2/nested_fun.c
@@ -0,0 +1,65 @@
+/* As part of inlining, a BLOCK (described as DW_TAG_lexical_block DIE's) may
+   be present both as an abstract instance and a concrete one in the DWARF
+   output.  This testcase attempts to make sure that the concrete ones refer to
+   the abstract ones thanks to the DW_AT_abstract_origin attribute.
+
+   Such a back-link enables debuggers to make entities present in the abstract
+   instance only available in concrete ones.  */
+
+/* { dg-options "-O2 -g -std=gnu99 -gdwarf -dA" } */
+/* { dg-final { scan-assembler-times "\\(DIE \\(0x.*\\) DW_TAG_lexical_block\\)\[^)\]*DW_AT_abstract_origin" 1 } } */
+
+extern void *create (const char *);
+extern void destroy (void *);
+extern void do_nothing (char);
+
+struct string
+{
+  const char *data;
+  int lb;
+  int ub;
+};
+
+int
+main (void)
+{
+  void *o1 = create ("foo");
+
+  void
+  parent (void)
+  {
+{
+  void *o2 = create ("bar");
+
+  int
+  child (struct string s)
+  {
+	int i = s.lb;
+
+	if (s.lb <= s.ub)
+	  while (1)
+	{
+	  char c = s.data[i - s.lb];
+	  do_nothing (c);
+	  if (c == 'o')
+		return 1;
+	  if (i == s.ub

Re: [PATCH] DWARF: add abstract origin links on lexical blocks DIEs

[Richard Biener]

On Sun, Jan 17, 2016 at 9:09 PM, Eric Botcazou  wrote:
>> Sounds like a good excuse to add a guality for Ada (which has unique
>> needs for dwarf).
>
> Well, the guality testsuite is a pain to maintain so I'd rather not.
> The GDB testsuite is clearly the right place for this kind of testcases.

But that tests GDB and not GCCs generation of DWARF ... which means
take the other option of writing a scan-assembler testcase looking for the
previously missing DWARF.

It would be nice if we'd support dropping in gdb/testsuite into
testsuite/gdb/ or so and include that in testing (plus in the test_summary
report).

Richard.

> --
> Eric Botcazou

Re: [PATCH] DWARF: add abstract origin links on lexical blocks DIEs

[Eric Botcazou]

> But that tests GDB and not GCCs generation of DWARF ...

But GDB only consumes the DWARF generated by GCC, it cannot synthetize it. ;-)

> which means take the other option of writing a scan-assembler testcase
> looking for the previously missing DWARF.

Fine with me (either Ada or C as far as I'm concerned).

-- 
Eric Botcazou

Re: [PATCH] DWARF: add abstract origin links on lexical blocks DIEs

[Pierre-Marie de Rodat]

On 01/18/2016 10:45 AM, Eric Botcazou wrote:

which means take the other option of writing a scan-assembler testcase
looking for the previously missing DWARF.


Fine with me (either Ada or C as far as I'm concerned).


Thank you for your inputs! I’m going to try that, then. I hope this test 
will not be too fragile…


--
Pierre-Marie de Rodat

Re: [PATCH] DWARF: add abstract origin links on lexical blocks DIEs

[Eric Botcazou]

> Sounds like a good excuse to add a guality for Ada (which has unique
> needs for dwarf).

Well, the guality testsuite is a pain to maintain so I'd rather not.
The GDB testsuite is clearly the right place for this kind of testcases.

-- 
Eric Botcazou

Re: [PATCH] DWARF: add abstract origin links on lexical blocks DIEs

[Pierre-Marie de Rodat]

On 01/13/2016 01:17 PM, Richard Biener wrote:

I wonder if you can construct a guality testcase that passes with and
fails without
the patch?


I’ve tried to first look at how guality testcases are written (thanks 
for your answers on IRC, by the way :-)) and then how I could write a 
testcase for my fix. It seems there are two ways: match patterns in the 
assembly file or evaluate an expression in GDB.


I already have the testcase I used during development: it’s written in 
Ada, to build with -O2. The way it checks the fix is to see if GDB 
manages to put a breakpoint on the Child2 symbol before executing the 
program (it cannot before my fix and it can afterwards). Oh, and it 
requires a fairly recent GDB version (7.10 looks good).


I managed to get a similar GNU C99 reproducer (it’s attached): the 
debugging information has the pattern that exhibits the bugfix. Namely: 
while the “parent” function is inlined, the “child” function (which is 
in a block inside “parent”) is not. So GDB relies on the 
DW_TAG_abstract_origin in the inlined block to refer to the abstract 
block that contains the DIE that materializes “child“.


However, it looks like there is no way in GDB to refer to C nested 
functions when they are not in the current scope:

$ gcc -g -O2 -std=gnu99 nested_fun.c nested_fun_helpers.c
$ gdb -n -q ./a.out
(gdb) ptype child
No symbol "child" in current context.
(gdb) ptype nested_fun.parent.child
No symbol "nested_fun" in current context.


On the other hand, this works with the Ada testcase:

(gdb) ptype nested_fun.parent.child
type = (false, true)


So I’m not sure what to do next: should I do a fragile testcase based on 
scanning the assembly file? (it could break with an optimizer change) 
create a guality testsuite for Ada?



Anyway, the patch looks ok to me but please give others a chance to chime in.


Sure. Thank you for reviewing!

--
Pierre-Marie de Rodat
/* { dg-do run } */
/* { dg-options "-O2 -g -std=gnu99" } */

extern void *create (const char *);
extern void destroy (void *);
extern void do_nothing (char);

struct string
{
  const char *data;
  int lb;
  int ub;
};

int
main (void)
{
  void *o1 = create ("foo");

  void
  parent (void)
  {
{
  void *o2 = create ("bar");

  int
  child (struct string s)
  {
	int i = s.lb;

	if (s.lb <= s.ub)
	  while (1)
	{
	  char c = s.data[i - s.lb];
	  do_nothing (c);
	  if (c == 'o')
		return 1;
	  if (i == s.ub)
		break;
	  ++i;
	}
	return 0;
  }

  int r;

  r = child ((struct string) {"baz", 1, 3});
  r = child ((struct string) {"qux", 2, 4});
  r = child ((struct string) {"foobar", 1, 6});
}

do_nothing (0);
  }

  /* { dg-final { gdb-test 56 "type:main::parent::child" "int (struct string)" } } */
  parent ();
  return 0;
}
void *
create (const char *s)
{
  return 0;
}

void
destroy (void *o)
{
  return;
}

void
do_nothing (char c)
{
  return;
}

Re: [PATCH] DWARF: add abstract origin links on lexical blocks DIEs

[Richard Biener]

On Fri, Jan 15, 2016 at 3:41 PM, Pierre-Marie de Rodat
 On 01/13/2016 01:17 PM, Richard Biener wrote:
>>
>> I wonder if you can construct a guality testcase that passes with and
>> fails without
>> the patch?
>
>
> I’ve tried to first look at how guality testcases are written (thanks for
> your answers on IRC, by the way :-)) and then how I could write a testcase
> for my fix. It seems there are two ways: match patterns in the assembly file
> or evaluate an expression in GDB.
>
> I already have the testcase I used during development: it’s written in Ada,
> to build with -O2. The way it checks the fix is to see if GDB manages to put
> a breakpoint on the Child2 symbol before executing the program (it cannot
> before my fix and it can afterwards). Oh, and it requires a fairly recent
> GDB version (7.10 looks good).
>
> I managed to get a similar GNU C99 reproducer (it’s attached): the debugging
> information has the pattern that exhibits the bugfix. Namely: while the
> “parent” function is inlined, the “child” function (which is in a block
> inside “parent”) is not. So GDB relies on the DW_TAG_abstract_origin in the
> inlined block to refer to the abstract block that contains the DIE that
> materializes “child“.
>
> However, it looks like there is no way in GDB to refer to C nested functions
> when they are not in the current scope:
>>
>> $ gcc -g -O2 -std=gnu99 nested_fun.c nested_fun_helpers.c
>> $ gdb -n -q ./a.out
>> (gdb) ptype child
>> No symbol "child" in current context.
>> (gdb) ptype nested_fun.parent.child
>> No symbol "nested_fun" in current context.
>
>
> On the other hand, this works with the Ada testcase:
>>
>> (gdb) ptype nested_fun.parent.child
>> type = (false, true)
>
>
> So I’m not sure what to do next: should I do a fragile testcase based on
> scanning the assembly file? (it could break with an optimizer change) create
> a guality testsuite for Ada?

Sounds like a good excuse to add a guality for Ada (which has unique
needs for dwarf).

Richard.

>> Anyway, the patch looks ok to me but please give others a chance to chime
>> in.
>
>
> Sure. Thank you for reviewing!
>
> --
> Pierre-Marie de Rodat

Re: [PATCH] DWARF: add abstract origin links on lexical blocks DIEs

[Richard Biener]

On Tue, Jan 12, 2016 at 6:05 PM, Pierre-Marie de Rodat
 wrote:
> Hello,
>
> Although the following patch does not fix a regression, I believe it fixes a
> bug visible from a debugger, so I think it’s a valid candidate at this
> stage.
>
> This change tracks from which abstract lexical block concrete ones come from
> in DWARF so that debuggers can inherit the former from the latter. This
> enables debuggers to properly handle the following case:
>
>   * function Child2 is nested in a lexical block, itself nested in
> function Child1;
>   * function Child1 is inlined into some call site;
>   * function Child2 is never inlined.
>
> Here, Child2 is described in DWARF only in the abstract instance of Child1.
> So when debuggers decode Child1's concrete instances, they need to fetch the
> definition for Child2 in the corresponding abstract instance: the
> DW_AT_abstract_origin link on the lexical block that embeds Child1 enables
> them to do that.
>
> Bootstrapped and regtested on x86_64-linux.
> Ok to commit? Thank you in advance!

I wonder if you can construct a guality testcase that passes with and
fails without
the patch?

Anyway, the patch looks ok to me but please give others a chance to chime in.

Thanks,
Richard.

> gcc/ChangeLog:
>
> * dwarf2out.c (add_abstract_origin_attribute): Adjust
> documentation comment.  For BLOCK nodes, add a
> DW_AT_abstract_origin attribute that points to the DIE generated
> for the origin BLOCK.
> (gen_lexical_block_die): Call add_abstract_origin_attribute for
> blocks from inlined functions.
> ---
>  gcc/dwarf2out.c | 13 ++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
> index da5524e..a889dbb 100644
> --- a/gcc/dwarf2out.c
> +++ b/gcc/dwarf2out.c
> @@ -18463,15 +18463,16 @@ add_prototyped_attribute (dw_die_ref die, tree
> func_type)
>  }
>   /* Add an 'abstract_origin' attribute below a given DIE.  The DIE is found
> -   by looking in either the type declaration or object declaration
> -   equate table.  */
> +   by looking in the type declaration, the object declaration equate table
> or
> +   the block mapping.  */
>   static inline dw_die_ref
>  add_abstract_origin_attribute (dw_die_ref die, tree origin)
>  {
>dw_die_ref origin_die = NULL;
>  -  if (TREE_CODE (origin) != FUNCTION_DECL)
> +  if (TREE_CODE (origin) != FUNCTION_DECL
> +  && TREE_CODE (origin) != BLOCK)
>  {
>/* We may have gotten separated from the block for the inlined
>  function, if we're in an exception handler or some such; make
> @@ -18493,6 +18494,8 @@ add_abstract_origin_attribute (dw_die_ref die, tree
> origin)
>  origin_die = lookup_decl_die (origin);
>else if (TYPE_P (origin))
>  origin_die = lookup_type_die (origin);
> +  else if (TREE_CODE (origin) == BLOCK)
> +origin_die = BLOCK_DIE (origin);
> /* XXX: Functions that are never lowered don't always have correct block
>   trees (in the case of java, they simply have no block tree, in some
> other
> @@ -21294,6 +21297,10 @@ gen_lexical_block_die (tree stmt, dw_die_ref
> context_die)
>   BLOCK_DIE (stmt) = stmt_die;
>   old_die = NULL;
> }
> +
> +  tree origin = block_ultimate_origin (stmt);
> +  if (origin != NULL_TREE && origin != stmt)
> +   add_abstract_origin_attribute (stmt_die, origin);
>  }
> if (old_die)
> --
> 2.3.3.199.g52cae64
>

[PATCH] DWARF: add abstract origin links on lexical blocks DIEs

[Pierre-Marie de Rodat]

Hello,

Although the following patch does not fix a regression, I believe it 
fixes a bug visible from a debugger, so I think it’s a valid candidate 
at this stage.


This change tracks from which abstract lexical block concrete ones come 
from in DWARF so that debuggers can inherit the former from the latter. 
This enables debuggers to properly handle the following case:


  * function Child2 is nested in a lexical block, itself nested in
function Child1;
  * function Child1 is inlined into some call site;
  * function Child2 is never inlined.

Here, Child2 is described in DWARF only in the abstract instance of 
Child1. So when debuggers decode Child1's concrete instances, they need 
to fetch the definition for Child2 in the corresponding abstract 
instance: the DW_AT_abstract_origin link on the lexical block that 
embeds Child1 enables them to do that.


Bootstrapped and regtested on x86_64-linux.
Ok to commit? Thank you in advance!

gcc/ChangeLog:

* dwarf2out.c (add_abstract_origin_attribute): Adjust
documentation comment.  For BLOCK nodes, add a
DW_AT_abstract_origin attribute that points to the DIE generated
for the origin BLOCK.
(gen_lexical_block_die): Call add_abstract_origin_attribute for
blocks from inlined functions.
---
 gcc/dwarf2out.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index da5524e..a889dbb 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -18463,15 +18463,16 @@ add_prototyped_attribute (dw_die_ref die, tree 
func_type)

 }
  /* Add an 'abstract_origin' attribute below a given DIE.  The DIE is 
found

-   by looking in either the type declaration or object declaration
-   equate table.  */
+   by looking in the type declaration, the object declaration equate 
table or

+   the block mapping.  */
  static inline dw_die_ref
 add_abstract_origin_attribute (dw_die_ref die, tree origin)
 {
   dw_die_ref origin_die = NULL;
 -  if (TREE_CODE (origin) != FUNCTION_DECL)
+  if (TREE_CODE (origin) != FUNCTION_DECL
+  && TREE_CODE (origin) != BLOCK)
 {
   /* We may have gotten separated from the block for the inlined
 function, if we're in an exception handler or some such; make
@@ -18493,6 +18494,8 @@ add_abstract_origin_attribute (dw_die_ref die, 
tree origin)

 origin_die = lookup_decl_die (origin);
   else if (TYPE_P (origin))
 origin_die = lookup_type_die (origin);
+  else if (TREE_CODE (origin) == BLOCK)
+origin_die = BLOCK_DIE (origin);
/* XXX: Functions that are never lowered don't always have correct 
block
  trees (in the case of java, they simply have no block tree, in 
some other
@@ -21294,6 +21297,10 @@ gen_lexical_block_die (tree stmt, dw_die_ref 
context_die)

  BLOCK_DIE (stmt) = stmt_die;
  old_die = NULL;
}
+
+  tree origin = block_ultimate_origin (stmt);
+  if (origin != NULL_TREE && origin != stmt)
+   add_abstract_origin_attribute (stmt_die, origin);
 }
if (old_die)
--
2.3.3.199.g52cae64