Fix PR83157, abstract origins refering to concrete instances

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.

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.

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
>

On Fri, Jan 12, 2018 at 09:53:28AM +0100, 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);
> }

I'm worried that we call dwarf2out_function_decl multiple times
for the same instance of the function and that undoing it means
we won't find the DIEs we've created before and we'll create them once
again.  Isn't that the case of e.g. non-LTO early vs. late dwarf?

	Jakub