Re: PR debug/47510
Mark Mitchell 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. -- Dodji
Re: PR debug/47510
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 CodeSourcery m...@codesourcery.com (650) 331-3385 x713
Re: PR debug/47510
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 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.
Re: PR debug/47510
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 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. -- Dodji
Re: PR debug/47510
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, -- Mark Mitchell CodeSourcery m...@codesourcery.com (650) 331-3385 x713
Re: PR debug/47510
Jason Merrill 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
Re: PR debug/47510
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
Re: PR debug/47510
Tom Tromey 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. -- Dodji
Re: PR debug/47510
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
Re: PR debug/47510
Tom Tromey writes: > 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. I have updated the patch to make add the DW_AT_linkage_name to the anonymous type. Tested on x86_64-unknown-linux-gnus against trunk and 4.6. I think this is material for 4.7 that we can backport to 4.6 after its release. -- Dodji >From 5cc08083834604525a11e4e4b6de830734520f6e Mon Sep 17 00:00:00 2001 From: Dodji Seketeli Date: Fri, 28 Jan 2011 11:53:49 +0100 Subject: [PATCH] PR debug/47510 PR debug/47510 * gcc/dwarf2out.c (strip_naming_typedef): Factorize out of ... (lookup_type_die_strip_naming_typedef): ... here. (get_context_die): Use it. (gen_typedef_die): Add a DW_AT_{MIPS}_linkage_name attribute to the anonymous struct named by the naming typedef. * gcc/testsuite/g++.dg/debug/dwarf2/typedef6.C: New test. --- gcc/dwarf2out.c | 36 + gcc/testsuite/g++.dg/debug/dwarf2/typedef6.C | 30 + 2 files changed, 60 insertions(+), 6 deletions(-) create mode 100644 gcc/testsuite/g++.dg/debug/dwarf2/typedef6.C 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 000..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 thi