[debug-early] Handle specification of class scoped static functions

[debug-early] Handle specification of class scoped static functions

[Aldy Hernandez]

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

Hi Jason.

For class scoped static functions:

		   class C {
		   public:
		     static void moo () {}
		   };

...we are calling gen_subprogram on moo() the usual handful of times 
(during early dwarf): once as a member of C and once because moo() is a 
reachable function.  However, on the second time, we reuse the cached 
DIE and merely remove the DW_AT_declaration attribute:

	  /* Clear out the declaration attribute, but leave the
	     parameters so they can be augmented with location
	     information later.  */
	  remove_AT (subr_die, DW_AT_declaration);

This causes us to reuse the DIE from within the class, instead of adding 
a specification of this cached DIE.

With the attached tweak we fix this problem, and about a hundred gdb 
regressions.  This IMHO is a "Good Thing" :).

Would you be so kind as to look at this two-liner to make sure you're OK 
with it?

Thanks.
Aldy

[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 1179 bytes --]

commit cec08d43caffbf086720ac3994d068010dc103c9
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Fri Mar 20 09:55:31 2015 -0700

    Handle specification of class scoped static functions.

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 8884afd..1325dfe 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -18770,7 +18770,20 @@ gen_subprogram_die (tree decl, dw_die_ref context_die)
       struct dwarf_file_data * file_index = lookup_filename (s.file);
       if (((is_cu_die (old_die->die_parent)
 	    || context_die == NULL
-	    || dumped_early)
+	    || (dumped_early
+		/* For class scoped static functions, the dumped early
+		   version was the declaration, whereas the next time
+		   around with a different context should be the
+		   specification.  In this case, avoid reusing the
+		   DIE, but generate a specification below. E.g.:
+
+		   class C {
+		   public:
+		     static void moo () {}
+		   };
+		*/
+		&& (!is_cu_die (context_die)
+		    || !class_or_namespace_scope_p (old_die->die_parent))))
 	   && (DECL_ARTIFICIAL (decl)
 	       || (get_AT_file (old_die, DW_AT_decl_file) == file_index
 		   && (get_AT_unsigned (old_die, DW_AT_decl_line)

Re: [debug-early] Handle specification of class scoped static functions

[Jason Merrill]

I think we want to drop the debug_early check there entirely; the added 
conditions seem to be gutting it.  If is_cu_die (old_die->die_parent) is 
false, then class_or_namespace_scope_p (old_die->die_parent) ought to be 
true.

Jason

Re: [debug-early] Handle specification of class scoped static functions

[Aldy Hernandez]

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

On 03/20/2015 02:21 PM, Jason Merrill wrote:
> I think we want to drop the debug_early check there entirely; the added
> conditions seem to be gutting it.  If is_cu_die (old_die->die_parent) is
> false, then class_or_namespace_scope_p (old_die->die_parent) ought to be
> true.
>
> Jason
>

Good catch.  I am so glad you are keeping track of all this spaghetti, 
but in my defense, it was pasta already.

With the attached I also got rid of one superfluous check for `old_die', 
as well as your suggestion.  We get rid of one more gdb regression.  Yay.

I'll wait for your high-five (or OK) before committing to branch.

Thanks.
Aldy

[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 1781 bytes --]

commit b3d910713d27dc29801f3ddbe8671a4a6e0de4c1
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Fri Mar 20 09:55:31 2015 -0700

    Handle specification of class scoped static functions.
    
    Remove superfluous check for old_die.

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 8884afd..7a52dc8 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -18735,7 +18735,7 @@ gen_subprogram_die (tree decl, dw_die_ref context_die)
      much as possible.  */
   else if (old_die)
     {
-      dumped_early = old_die && old_die->dumped_early;
+      dumped_early = old_die->dumped_early;
 
       /* A declaration that has been previously dumped needs no
 	 additional information.  */
@@ -18768,13 +18768,23 @@ gen_subprogram_die (tree decl, dw_die_ref context_die)
 	 apply; we just use the old DIE.  */
       expanded_location s = expand_location (DECL_SOURCE_LOCATION (decl));
       struct dwarf_file_data * file_index = lookup_filename (s.file);
-      if (((is_cu_die (old_die->die_parent)
-	    || context_die == NULL
-	    || dumped_early)
+      if ((is_cu_die (old_die->die_parent)
+	   || context_die == NULL
+	   /* For class scoped static functions, the dumped early
+	      version was the declaration, whereas the next time
+	      around with a different context should be the
+	      specification.  In this case, avoid reusing the DIE, but
+	      generate a specification below. E.g.:
+
+	      class C {
+	      public:
+	        static void moo () {}
+	      };  */
+	   || !is_cu_die (context_die))
 	   && (DECL_ARTIFICIAL (decl)
 	       || (get_AT_file (old_die, DW_AT_decl_file) == file_index
 		   && (get_AT_unsigned (old_die, DW_AT_decl_line)
-		       == (unsigned) s.line)))))
+		       == (unsigned) s.line))))
 	{
 	  subr_die = old_die;
 

Re: [debug-early] Handle specification of class scoped static functions

[Jason Merrill]

On 03/20/2015 08:11 PM, Aldy Hernandez wrote:
> +	   /* For class scoped static functions, the dumped early
> +	      version was the declaration, whereas the next time
> +	      around with a different context should be the
> +	      specification.  In this case, avoid reusing the DIE, but
> +	      generate a specification below. E.g.:
> +
> +	      class C {
> +	      public:
> +	        static void moo () {}
> +	      };  */
> +	   || !is_cu_die (context_die))

Why do we still need this added (relative to trunk)?  Are we getting 
here multiple times with class context_die?

Also, the comment seems redundant with the comment immediately above.

Jason

Re: [debug-early] Handle specification of class scoped static functions

[Aldy Hernandez]

On 04/03/2015 07:48 AM, Jason Merrill wrote:
> On 03/20/2015 08:11 PM, Aldy Hernandez wrote:
>> +       /* For class scoped static functions, the dumped early
>> +          version was the declaration, whereas the next time
>> +          around with a different context should be the
>> +          specification.  In this case, avoid reusing the DIE, but
>> +          generate a specification below. E.g.:
>> +
>> +          class C {
>> +          public:
>> +            static void moo () {}
>> +          };  */
>> +       || !is_cu_die (context_die))
>
> Why do we still need this added (relative to trunk)?  Are we getting
> here multiple times with class context_die?

Apparently we no longer need it for the C++ case above, so the comment 
certainly needs updating, but we need it for fortran:

module some_m
contains
    logical function funky (FLAG)
      funky = .true.
   end function
end module

The first time through gen_subprogram_die() we generate the DIE with a 
context of DW_TAG_module (early dwarf).  The second time, in late dwarf, 
we get here with a DW_TAG_module context again, so the above code will 
allow us to reuse the DIE, instead of creating a DW_AT_specification.

...or perhaps we could change the condition to:

       if ((is_cu_die (old_die->die_parent)
+	   || old_die->die_parent->die_tag == DW_TAG_module
	   || context_die == NULL

??
Aldy

Re: [debug-early] Handle specification of class scoped static functions

[Jason Merrill]

On 04/13/2015 02:01 PM, Aldy Hernandez wrote:
> ...or perhaps we could change the condition to:
>
>        if ((is_cu_die (old_die->die_parent)
> +       || old_die->die_parent->die_tag == DW_TAG_module
>         || context_die == NULL

Does checking context_die == old_die->die_parent work?

Jason

Re: [debug-early] Handle specification of class scoped static functions

[Jason Merrill]

On 04/13/2015 05:04 PM, Jason Merrill wrote:
> On 04/13/2015 02:01 PM, Aldy Hernandez wrote:
>> ...or perhaps we could change the condition to:
>>
>>        if ((is_cu_die (old_die->die_parent)
>> +       || old_die->die_parent->die_tag == DW_TAG_module
>>         || context_die == NULL
>
> Does checking context_die == old_die->die_parent work?

Actually, no.  The comment here is saying that we don't want to reuse 
the DIE in this case, because we want the DIE with the source location 
info to be at CU scope.  We only want to reuse if the function is at CU 
scope anyway or it's a nested function.

Jason