On Fri, 12 Jan 2018, Richard Biener wrote: > > This fixes PR83157 (well, not the guality fail...) and avoids creating > references to abstract instances that actually end up refering to > the concrete instance thereby eventually picking up invalid location > attributes (and whatever else there may be). > > As said in the PR a simple testcase > > int foo (int i) > { > volatile int boring = 1; > return boring + i; > } > > int bar() > { > int tem = 1; > return tem + foo (0); > } > > shows the inline instance of foo in bar refering to the concrete > instance of 'boring' rather than the abstract instance. Analysis > of the guality fail referenced in the PR shows such a case where > gdb picks up a location attribute from the wrong place (the inline > instance doesn't have one). > > The issue here is that dwarf2out_function_decl (late annotation > of the function) creates new concrete DIEs refering back to > the abstract DIEs but also registers those concrete DIEs with > equate_decl_number_to_die. This causes late annotation of > bar to pick those as abstract instance when creating inline > instance DIEs. > > Somehow this doesn't happen with GCC 5, not sure why. > > The fix is to not put concrete instances into the decl -> DIE > mapping over the dwarf2out_function_decl call but unwind any > changes that replaced DIEs (just occured to me we could assert > that the replacement DIEs always have an abstract origin of the > DIE replaced -- if that works, possibly not for multiple instances > of recursive inlining which may have its own share of similar > issues...). > > A different fix might be to do sth like getting the ultimate > origin when adding an abstract origin (following DW_AT_abstract_origin > links on the destination), but GCC 5 didn't do that and I'm not > sure if that's the desired behavior anyway (it also might have > interesting side-effects for early LTO debug, not sure). > > Bootstrapped and tested on x86_64-unknown-linux-gnu. > > Not really asking for an OK (well, maybe yes?) but for further > ideas. > > PR83157 will still need further investigation as the guality test > still fails after the patch. > > No testcase, I haven't been able to produce a simple one that > gets wrong location attributes inherited and thus might be > turned into a guality test. I also don't have any good idea > how to scan-assembler the dwarf for the correct vs. the invalid > DW_AT_abstract_origin reference... I suppose the python dwarf > parsing testsuite thing would make this feasible somehow.
So I'm following up myself quickly here since I tracked down this regression to the early debug work (r224161), specifically to gen_variable_die no longer skipping equate_decl_number_to_die in if (decl && (DECL_ABSTRACT_P (decl) || !old_die || is_declaration_die (old_die))) equate_decl_number_to_die (decl, var_die); because the rev. NULLs old_die in a path that ends with the only use being the above one. So the small testcase is fixed by Index: gcc/dwarf2out.c =================================================================== --- gcc/dwarf2out.c (revision 255167) +++ gcc/dwarf2out.c (working copy) @@ -21231,10 +21231,8 @@ gen_variable_die (tree decl, tree origin { /* If we will be creating an inlined instance, we need a new DIE that will get annotated with - DW_AT_abstract_origin. Clear things so we can get a - new DIE. */ + DW_AT_abstract_origin. */ gcc_assert (!DECL_ABSTRACT_P (decl)); - old_die = NULL; } else { as verified on the GCC 6 branch and trunk. I'll give that more testing now and if it succeeds I consider that fix quite obvious. Thanks, Richard. > Thanks, > Richard. > > 2018-01-12 Richard Biener <rguent...@suse.de> > > PR debug/83157 > * dwarf2out.c (decl_die_table_undo_stack): New static vector. > (equate_decl_number_to_die): If there's an undo stack record > previous DIE for the decl. > (dwarf2out_function_decl): Initialize decl_die_table_undo_stack > and unwind changes before returning. > > Index: gcc/dwarf2out.c > =================================================================== > --- gcc/dwarf2out.c (revision 256535) > +++ gcc/dwarf2out.c (working copy) > @@ -3159,6 +3159,8 @@ struct decl_die_hasher : ggc_ptr_hash<di > /* A hash table of references to DIE's that describe declarations. > The key is a DECL_UID() which is a unique number identifying each decl. > */ > static GTY (()) hash_table<decl_die_hasher> *decl_die_table; > +/* A stack to unwind changes to decl_die_table. */ > +static vec<std::pair<tree, dw_die_ref> > *decl_die_table_undo_stack; > > struct GTY ((for_user)) variable_value_struct { > unsigned int decl_id; > @@ -5762,7 +5764,10 @@ equate_decl_number_to_die (tree decl, dw > { > unsigned int decl_id = DECL_UID (decl); > > - *decl_die_table->find_slot_with_hash (decl, decl_id, INSERT) = decl_die; > + dw_die_ref *slot = decl_die_table->find_slot_with_hash (decl, decl_id, > INSERT); > + if (*slot && decl_die_table_undo_stack) > + decl_die_table_undo_stack->safe_push (std::make_pair (decl, *slot)); > + *slot = decl_die; > decl_die->decl_id = decl_id; > } > > @@ -26044,6 +26049,10 @@ dwarf2out_decl (tree decl) > static void > dwarf2out_function_decl (tree decl) > { > + vec<std::pair<tree, dw_die_ref> > *undo_stack > + = new vec<std::pair<tree, dw_die_ref> > (); > + decl_die_table_undo_stack = undo_stack; > + > dwarf2out_decl (decl); > call_arg_locations = NULL; > call_arg_loc_last = NULL; > @@ -26051,6 +26060,17 @@ dwarf2out_function_decl (tree decl) > tail_call_site_count = -1; > decl_loc_table->empty (); > cached_dw_loc_list_table->empty (); > + > + /* Unwind changes to the decl -> DIE mapping so we keep the abstract > + instances associated with the decl. */ > + decl_die_table_undo_stack = NULL; > + while (! undo_stack->is_empty ()) > + { > + std::pair<tree, dw_die_ref> op = undo_stack->pop (); > + equate_decl_number_to_die (op.first, op.second); > + } > + > + delete undo_stack; > } > > /* Output a marker (i.e. a label) for the beginning of the generated code for > -- Richard Biener <rguent...@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)