Patchwork PR debug/47510

login
register
mail settings
Submitter Dodji Seketeli
Date Jan. 28, 2011, 4:01 p.m.
Message ID <m31v3x10hv.fsf@redhat.com>
Download mbox | patch
Permalink /patch/80866/
State New
Headers show

Comments

Dodji Seketeli - Jan. 28, 2011, 4:01 p.m.
Hello,

consider the example below:

    class C {
    public:
      C() {}
      ~C() {}
    };
    typedef struct {
      C m;
    } t;
    typedef t s;
    s v;

The bug is that the DW_TAG_typedef of the naming typedef t has
children DIEs that describe the constructor and destructor of the
anonymous struct named by t.

This is a leftover of PR debug/46101.

In this case when callgraph asks the dwarf backend to emit debug info
for the constructor of the anonymous struct, get_context_die [called
with the anonymous struct's tree] returns the DIE of the naming
typedef. The dwarf emitter then hangs the DIE of the constructor off
the typedef.

This patch strips the naming typedef from the result of
get_context_die.


Tested on x86_64-unknown-linux-gnu against trunk.
Tom Tromey - Feb. 2, 2011, 4:38 p.m.
>>>>> "Dodji" == Dodji Seketeli <dodji@redhat.com> writes:

Dodji>     class C {
Dodji>     public:
Dodji>       C() {}
Dodji>       ~C() {}
Dodji>     };
Dodji>     typedef struct {
Dodji>       C m;
Dodji>     } t;
Dodji>     typedef t s;
Dodji>     s v;

Before this patch, we got the weird result of a DW_TAG_typedef with
children.  This was weird because DWARF doesn't mention this as a
possibility, so no DWARF readers are prepared to do anything sensible
with it.

After this patch, we get a different, and IMO worse, weird result:

 <1><70>: Abbrev Number: 8 (DW_TAG_structure_type)
    <71>   DW_AT_byte_size   : 1	
    <72>   DW_AT_decl_file   : 1	
    <73>   DW_AT_decl_line   : 7	
    <74>   DW_AT_sibling     : <0xb0>	
[...]
 <2><84>: Abbrev Number: 10 (DW_TAG_subprogram)
    <85>   DW_AT_name        : t	
    <87>   DW_AT_artificial  : 1	
    <88>   DW_AT_declaration : 1	
    <89>   DW_AT_object_pointer: <0x91>	
    <8d>   DW_AT_sibling     : <0x98>	
[...]
 <1><b0>: Abbrev Number: 12 (DW_TAG_typedef)
    <b1>   DW_AT_name        : t	
    <b3>   DW_AT_decl_file   : 1	
    <b4>   DW_AT_decl_line   : 9	
    <b5>   DW_AT_type        : <0x70>	


That is, the typedef DIE is a normal typedef -- good so far.  But, it is
a typedef of an anonymous struct that has the struct's members.  This
would be ok, except one of the members is the (synthetic) constructor
't'.

In order to handle this, I think gdb is going to have to look for
typedefs to anonymous structs, then search the struct for a constructor
(or maybe destructor is more reliable) matching the typedef name.  That
is because there is no marker indicating "this anonymous struct was
given a canonical name via a C++ typedef".

It seems to me that it would be simpler to just emit 't' as an ordinary
DW_TAG_structure_type and not emit a typedef at all.  This is perhaps
mildly different from what the sources say, but it is a very obscure
case.  Jan did think of one case where it matters: if you want to be
able to find the location of the typedef separately from the location of
the struct definition.  However, I think this is not very important, and
certainly less important than the gyrations debuginfo readers have to go
through to reconstruct 't'.

Tom
Tom Tromey - Feb. 3, 2011, 9:04 p.m.
>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> It seems to me that it would be simpler to just emit 't' as an ordinary
Tom> DW_TAG_structure_type and not emit a typedef at all.

After a lot of discussion on irc, we came up with another idea: extend
this patch to add DW_AT_linkage_name == 't' to the anonymous structure.
This makes the DWARF remain a faithful representation of the C++, but
also makes it simple for debuginfo readers to understand what is going
on.  In particular I think it will make the gdb side of this tractable.

Tom

Patch

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 2309297..438baec 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -6270,6 +6270,7 @@  static void remove_child_TAG (dw_die_ref, enum dwarf_tag);
 static void add_child_die (dw_die_ref, dw_die_ref);
 static dw_die_ref new_die (enum dwarf_tag, dw_die_ref, tree);
 static dw_die_ref lookup_type_die (tree);
+static dw_die_ref strip_naming_typedef (tree, dw_die_ref);
 static dw_die_ref lookup_type_die_strip_naming_typedef (tree);
 static void equate_type_number_to_die (tree, dw_die_ref);
 static hashval_t decl_die_table_hash (const void *);
@@ -8044,6 +8045,22 @@  lookup_type_die (tree type)
   return TYPE_SYMTAB_DIE (type);
 }
 
+/* Given a TYPE_DIE representing the type TYPE, if TYPE is an
+   anonymous type named by the typedef TYPE_DIE, return the DIE of the
+   anonymous type instead the one of the naming typedef.  */
+
+static inline dw_die_ref
+strip_naming_typedef (tree type, dw_die_ref type_die)
+{
+  if (type
+      && TREE_CODE (type) == RECORD_TYPE
+      && type_die
+      && type_die->die_tag == DW_TAG_typedef
+      && is_naming_typedef_decl (TYPE_NAME (type)))
+    type_die = get_AT_ref (type_die, DW_AT_type);
+  return type_die;
+}
+
 /* Like lookup_type_die, but if type is an anonymous type named by a
    typedef[1], return the DIE of the anonymous type instead the one of
    the naming typedef.  This is because in gen_typedef_die, we did
@@ -8058,11 +8075,7 @@  static inline dw_die_ref
 lookup_type_die_strip_naming_typedef (tree type)
 {
   dw_die_ref die = lookup_type_die (type);
-  if (TREE_CODE (type) == RECORD_TYPE
-      && die->die_tag == DW_TAG_typedef
-      && is_naming_typedef_decl (TYPE_NAME (type)))
-    die = get_AT_ref (die, DW_AT_type);
-  return die;
+  return strip_naming_typedef (type, die);
 }
 
 /* Equate a DIE to a given type specifier.  */
@@ -20754,7 +20767,10 @@  get_context_die (tree context)
     {
       /* Find die that represents this context.  */
       if (TYPE_P (context))
-	return force_type_die (TYPE_MAIN_VARIANT (context));
+	{
+	  context = TYPE_MAIN_VARIANT (context);
+	  return strip_naming_typedef (context, force_type_die (context));
+	}
       else
 	return force_decl_die (context);
     }
diff --git a/gcc/testsuite/g++.dg/debug/dwarf2/typedef6.C b/gcc/testsuite/g++.dg/debug/dwarf2/typedef6.C
new file mode 100644
index 0000000..8896446
--- /dev/null
+++ b/gcc/testsuite/g++.dg/debug/dwarf2/typedef6.C
@@ -0,0 +1,30 @@ 
+// Origin PR debug/
+// { dg-options "-g -dA" }
+
+class C {
+public:
+  C() {}
+  ~C() {}
+};
+typedef struct {
+  C m;
+} t;
+typedef t s;
+s v;
+
+/*
+  We want to check that we have a DIE describing the typedef t like this:
+
+	.uleb128 0xc	# (DIE (0xb8) DW_TAG_typedef)
+	.ascii "t\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (../../prtests/test.cc)
+	.byte	0xb	# DW_AT_decl_line
+	.long	0x78	# DW_AT_type
+
+  e.g, it should not haven any child DIE -- the bug here was that this
+  DIE had children DIEs. So we check that the last line is immediately
+  followed by a line containing the pattern "(DIE (", instead of a
+  line containing a DW_AT_sibling attribute.
+ */
+
+// { dg-final { scan-assembler-times "\[^\n\r\]*\\(DIE \[^\n\r\]* DW_TAG_typedef\\)\[\n\r\]{1,2}\[^\n\r\].*\"t\\\\0\"\[^\n\r\]*DW_AT_name\[\n\r\]{1,2}\[^\n\r\]*\[\n\r\]{1,2}\[^\n\r\]*\[\n\r\]{1,2}\[^\n\r\]*DW_AT_type\[\n\r\]{1,2}\[^\n\r\]*\\(DIE" 1 } }