Re: [PATCH] Fix PR83157, abstract origins refering to concrete instances

[Richard Biener <rguenther at suse dot de>]

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  <rguenther@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 <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)