| Message ID | m3mxkxuzbe.fsf@redhat.com |
|---|---|
| State | New |
| Headers | show |
Tom> After a lot of discussion on irc, we came up with another idea: extend Tom> this patch to add DW_AT_linkage_name == 't' to the anonymous Tom> structure. This makes the DWARF remain a faithful representation of Tom> the C++, but also makes it simple for debuginfo readers to understand Tom> what is going on. In particular I think it will make the gdb side of Tom> this tractable. Dodji> I have updated the patch to make add the DW_AT_linkage_name to the Dodji> anonymous type. Dodji> Tested on x86_64-unknown-linux-gnus against trunk and 4.6. I think this Dodji> is material for 4.7 that we can backport to 4.6 after its release. I would like to ask that it be considered for 4.6. IIRC, if this patch does not go in 4.6, then we have to write some special and ugly GDB code to work around the debuginfo generated by 4.6. I would much prefer it if there was no need to write this code. Tom
Tom Tromey <tromey@redhat.com> writes: > I would like to ask that it be considered for 4.6. > > IIRC, if this patch does not go in 4.6, then we have to write some > special and ugly GDB code to work around the debuginfo generated by 4.6. > I would much prefer it if there was no need to write this code. I see. I was perhaps being too conservative in my judgement here. FWIW assuming the patch gets reviewed I'd second your request in asking RM to consider this for 4.6.
This patch is OK. I also think it's a bug that the constructors of the anonymous struct have 't' in their names; they should also be anonymous with DW_AT_linkage_name. Jason
Jason Merrill <jason@redhat.com> writes: > This patch is OK. Thank you. Would the RMs (in CC) object to this patch going into 4.6? > I also think it's a bug that the constructors of the anonymous struct > have 't' in their names; they should also be anonymous with > DW_AT_linkage_name. I think this makes sense. Tom, Jan, would this be good enough from a debug info consumer point of view? If yes I'll propose a separate patch for this a bit later. -- Dodji
On 3/16/2011 1:04 PM, Dodji Seketeli wrote:
> Would the RMs (in CC) object to this patch going into 4.6?
What would be the justification for that? The bar is pretty high on
putting a patch onto a release branch.
I don't see any evidence that this is a regression, and a bug that
affects debugging is never *that* serious compared to (for example)
silent wrong-code generation. In this case, we're dealing with
anonymous structs, which aren't very common. This just seems like a
run-of-the-mill bug to me.
Thank you,
Yesterday after discussing this on IRC, Jakub expressed his personal opinion by saying the patch could go in 4.6. I mistakenly took it as a formal approval from the RMs and I committed it. I should have waited for an approval by email. So I have just reverted the patch from 4.6 now. Sorry for that. Back to the discussion now :-) Mark Mitchell <mark@codesourcery.com> writes: > On 3/16/2011 1:04 PM, Dodji Seketeli wrote: > >> Would the RMs (in CC) object to this patch going into 4.6? > What would be the justification for that? It's a regression from 4.5, caused by the fix for PR c++/44188. One of the observed side effect is that a DW_TAG_typedef DIE can now have children DIEs. That is not desirable in itself and makes GDB crash. > I don't see any evidence that this is a regression This is because the bug wasn't flagged as a regression. It is now. > A bug that affects debugging is never *that* serious compared to (for > example) silent wrong-code generation. I agree that fixing silent wrong-code generation bugs is always paramount. But I believe that a bug that suddenly leads GDB to a crash is not something we would want to let happen at this point either.
On Thu, 17 Mar 2011, Dodji Seketeli wrote: > Yesterday after discussing this on IRC, Jakub expressed his personal > opinion by saying the patch could go in 4.6. I mistakenly took it as a > formal approval from the RMs and I committed it. I should have waited > for an approval by email. So I have just reverted the patch from 4.6 > now. Sorry for that. > > Back to the discussion now :-) > > Mark Mitchell <mark@codesourcery.com> writes: > > > On 3/16/2011 1:04 PM, Dodji Seketeli wrote: > > > >> Would the RMs (in CC) object to this patch going into 4.6? > > > What would be the justification for that? > > It's a regression from 4.5, caused by the fix for PR c++/44188. One of > the observed side effect is that a DW_TAG_typedef DIE can now have > children DIEs. That is not desirable in itself and makes GDB crash. > > > I don't see any evidence that this is a regression > > This is because the bug wasn't flagged as a regression. It is now. > > > A bug that affects debugging is never *that* serious compared to (for > > example) silent wrong-code generation. > > I agree that fixing silent wrong-code generation bugs is always > paramount. But I believe that a bug that suddenly leads GDB to a crash > is not something we would want to let happen at this point either. I agree that crashing GDB isn't desirable, we should avoid that for 4.6.0. Richard.
On 3/17/2011 4:08 AM, Dodji Seketeli wrote: > Yesterday after discussing this on IRC, Jakub expressed his personal > opinion by saying the patch could go in 4.6. I mistakenly took it as a > formal approval from the RMs and I committed it. I should have waited > for an approval by email. You don't have to apologize -- an approval from any RM, in any forum (IRC, email, etc.) is sufficient authorization. > It's a regression from 4.5, caused by the fix for PR c++/44188. And, in any case, if it's a regression it's OK with me. Thank you,
Mark Mitchell <mark@codesourcery.com> writes:
> And, in any case, if it's a regression it's OK with me.
Thanks. I have committed the patch back into 4.6.
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c index dfe1086..cf935d0 100644 --- a/gcc/dwarf2out.c +++ b/gcc/dwarf2out.c @@ -6346,6 +6346,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 *); @@ -8120,6 +8121,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 @@ -8134,11 +8151,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. */ @@ -20350,6 +20363,14 @@ gen_typedef_die (tree decl, dw_die_ref context_die) anonymous struct DIE. */ if (!TREE_ASM_WRITTEN (type)) gen_tagged_type_die (type, context_die, DINFO_USAGE_DIR_USE); + + /* This is a GNU Extension. We are adding a + DW_AT_linkage_name attribute to the DIE of the + anonymous struct TYPE. The value of that attribute + is the name of the typedef decl naming the anonymous + struct. This greatly eases the work of consumers of + this debug info. */ + add_linkage_attr (lookup_type_die (type), decl); } } @@ -20830,7 +20851,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 } }