Re: [PATCH] Early LTO debug TODO

[Jason Merrill <jason@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1990 bytes --]

On Thu, Aug 31, 2017 at 4:03 PM, Richard Biener <rguenther@suse.de> wrote:
> On Thu, 31 Aug 2017, Richard Biener wrote:
>
>>
>> As suspected during review the DECL_ABSTRACT_P handling in
>> gen_formal_parameter_die is no longer necessary so the following
>> patch removes it.
>>
>> [LTO] bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
>> The gdb testsuite shows no regression.
>>
>> Will apply after testing finished.
>
> Ok, so it doesn't work - it regresses for example
> gcc.dg/debug/dwarf2/inline1.c because we end up annotating the
> abstract DIE with a location.  This is because in gen_subprogram_die
> (with decl set as abstract-self and thus generating a concrete
> instance subroutine die) we call
>
>           else if (parm && !POINTER_BOUNDS_P (parm))
>             {
>               dw_die_ref parm_die = gen_decl_die (parm, NULL, NULL,
> subr_die);
>
> thus unconditionally pass NULL as origin where gen_formal_parameter_die
> just has
>
>   /* If we have a previously generated DIE, use it, unless this is an
>      concrete instance (origin != NULL), in which case we need a new
>      DIE with a corresponding DW_AT_abstract_origin.  */
>   bool reusing_die;
>   if (parm_die && origin == NULL)
>     reusing_die = true;
>   else
>     {
>       parm_die = new_die (DW_TAG_formal_parameter, context_die, node);
>       reusing_die = false;
>     }
>
> but obviously that logic is flawed with us passing origin as NULL...
>
> What saved us here is the check whether the existing param_die has
> the correct parent, if not we didn't re-use it.  OTOH for die_parent
> == NULL we have the extra check that would have been dead code.
>
> Any hint as to whether we should pass in anything as origin or
> whether we should keep the existing die_parent logic somehow?
> (do we ever get a NULL context_die passed?)

The problem is that we aren't checking decl_ultimate_origin soon
enough, and thereby setting origin.

Tested x86_64-pc-linux-gnu, applying to trunk:

[-- Attachment #2: parm-debug.diff --]
[-- Type: text/plain, Size: 1952 bytes --]

commit b7702a2cbca9c50e94053cc125ff93438d377126
Author: Jason Merrill <jason@redhat.com>
Date:   Sat Sep 9 11:33:14 2017 +0200

            * dwarf2out.c (gen_formal_parameter_die): Remove obsolete hunk.
    
            Check ultimate_origin before setting reusing_die.

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 651dd0c..cc93db3 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -21285,30 +21285,15 @@ gen_formal_parameter_die (tree node, tree origin, bool emit_name_p,
 			  dw_die_ref context_die)
 {
   tree node_or_origin = node ? node : origin;
-  tree ultimate_origin;
   dw_die_ref parm_die = NULL;
   
-  if (TREE_CODE_CLASS (TREE_CODE (node_or_origin)) == tcc_declaration)
+  if (DECL_P (node_or_origin))
     {
       parm_die = lookup_decl_die (node);
 
-      /* If the contexts differ, we may not be talking about the same
-	 thing.
-	 ???  When in LTO the DIE parent is the "abstract" copy and the
-	 context_die is the specification "copy".  But this whole block
-	 should eventually be no longer needed.  */
-      if (parm_die && parm_die->die_parent != context_die && !in_lto_p)
-	{
-	  if (!DECL_ABSTRACT_P (node))
-	    {
-	      /* This can happen when creating an inlined instance, in
-		 which case we need to create a new DIE that will get
-		 annotated with DW_AT_abstract_origin.  */
-	      parm_die = NULL;
-	    }
-	  else
-	    gcc_unreachable ();
-	}
+      tree ultimate_origin = decl_ultimate_origin (node_or_origin);
+      if (node || ultimate_origin)
+	origin = ultimate_origin;
 
       if (parm_die && parm_die->die_parent == NULL)
 	{
@@ -21343,10 +21328,6 @@ gen_formal_parameter_die (tree node, tree origin, bool emit_name_p,
   switch (TREE_CODE_CLASS (TREE_CODE (node_or_origin)))
     {
     case tcc_declaration:
-      ultimate_origin = decl_ultimate_origin (node_or_origin);
-      if (node || ultimate_origin)
-	origin = ultimate_origin;
-
       if (reusing_die)
 	goto add_location;