diff mbox

PR debug/66653: avoid late_global_decl on decl_type_context()s

Message ID 558B71A3.3080004@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez June 25, 2015, 3:12 a.m. UTC
The problem here is that we are trying to call 
dwarf2out_late_global_decl() on a static variable in a template which 
has a type of TEMPLATE_TYPE_PARM:

template <typename T> class A
{
   static __thread T a;
};

We are calling late_global_decl because we are about to remove the 
unused static from the symbol table:

	  /* See if the debugger can use anything before the DECL
	     passes away.  Perhaps it can notice a DECL that is now a
	     constant and can tag the early DIE with an appropriate
	     attribute.

	     Otherwise, this is the last chance the debug_hooks have
	     at looking at optimized away DECLs, since
	     late_global_decl will subsequently be called from the
	     contents of the now pruned symbol table.  */
	  if (!decl_function_context (node->decl))
	    (*debug_hooks->late_global_decl) (node->decl);

Since gen_type_die_with_usage() cannot handle TEMPLATE_TYPE_PARMs we ICE.

I think we need to avoid calling late_global_decl on DECL's for which 
decl_type_context() is true, similarly to what we do for the call to 
early_global_decl in rest_of_decl_compilation:

       && !decl_function_context (decl)
       && !current_function_decl
       && DECL_SOURCE_LOCATION (decl) != BUILTINS_LOCATION
       && !decl_type_context (decl))
     (*debug_hooks->early_global_decl) (decl);

Presumably the old code did not run into this problem because the 
TEMPLATE_TYPE_PARAMs had been lowered by the time dwarf2out_decl was 
called, but here we are calling late_global_decl relatively early.

The attached patch fixes the problem.

Tested with --enable-languages=all.  Ada had other issues, so I skipped it.

OK for mainline?
commit 302f9976c53aa09e431bd54f37dbfeaa2c6b2acc
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Wed Jun 24 20:04:09 2015 -0700

    	PR debug/66653
    	* cgraphunit.c (analyze_functions): Do not call
    	debug_hooks->late_global_decl when decl_type_context.

Comments

Richard Biener June 25, 2015, 9:48 a.m. UTC | #1
On Thu, Jun 25, 2015 at 5:12 AM, Aldy Hernandez <aldyh@redhat.com> wrote:
> The problem here is that we are trying to call dwarf2out_late_global_decl()
> on a static variable in a template which has a type of TEMPLATE_TYPE_PARM:
>
> template <typename T> class A
> {
>   static __thread T a;
> };
>
> We are calling late_global_decl because we are about to remove the unused
> static from the symbol table:
>
>           /* See if the debugger can use anything before the DECL
>              passes away.  Perhaps it can notice a DECL that is now a
>              constant and can tag the early DIE with an appropriate
>              attribute.
>
>              Otherwise, this is the last chance the debug_hooks have
>              at looking at optimized away DECLs, since
>              late_global_decl will subsequently be called from the
>              contents of the now pruned symbol table.  */
>           if (!decl_function_context (node->decl))
>             (*debug_hooks->late_global_decl) (node->decl);
>
> Since gen_type_die_with_usage() cannot handle TEMPLATE_TYPE_PARMs we ICE.
>
> I think we need to avoid calling late_global_decl on DECL's for which
> decl_type_context() is true, similarly to what we do for the call to
> early_global_decl in rest_of_decl_compilation:
>
>       && !decl_function_context (decl)
>       && !current_function_decl
>       && DECL_SOURCE_LOCATION (decl) != BUILTINS_LOCATION
>       && !decl_type_context (decl))
>     (*debug_hooks->early_global_decl) (decl);
>
> Presumably the old code did not run into this problem because the
> TEMPLATE_TYPE_PARAMs had been lowered by the time dwarf2out_decl was called,
> but here we are calling late_global_decl relatively early.

I think we need to sort out that instead - by the time we call _early_
global decl it
should already be "lowered".  Otherwise LTO streaming will run into
the decl_type_context it cannot handle.  Is the case running into
late_global_decl
before we called early_global_decl on it btw?

Richard.

> The attached patch fixes the problem.
>
> Tested with --enable-languages=all.  Ada had other issues, so I skipped it.
>
> OK for mainline?
Eric Botcazou June 25, 2015, 1:53 p.m. UTC | #2
> Tested with --enable-languages=all.  Ada had other issues, so I skipped it.

What other issues exactly?  It's fine at r224930 for example.
Aldy Hernandez June 25, 2015, 2:41 p.m. UTC | #3
On 06/25/2015 06:53 AM, Eric Botcazou wrote:
>> Tested with --enable-languages=all.  Ada had other issues, so I skipped it.
>
> What other issues exactly?  It's fine at r224930 for example.
>

There were some defined but not used warnings (??) that caused it to 
fail, especially since I didn't configure with --disable-werror.  I 
didn't look into it.

Aldy
Aldy Hernandez June 25, 2015, 2:57 p.m. UTC | #4
On 06/25/2015 02:48 AM, Richard Biener wrote:
> On Thu, Jun 25, 2015 at 5:12 AM, Aldy Hernandez <aldyh@redhat.com> wrote:
>> The problem here is that we are trying to call dwarf2out_late_global_decl()
>> on a static variable in a template which has a type of TEMPLATE_TYPE_PARM:
>>
>> template <typename T> class A
>> {
>>    static __thread T a;
>> };
>>
>> We are calling late_global_decl because we are about to remove the unused
>> static from the symbol table:
>>
>>            /* See if the debugger can use anything before the DECL
>>               passes away.  Perhaps it can notice a DECL that is now a
>>               constant and can tag the early DIE with an appropriate
>>               attribute.
>>
>>               Otherwise, this is the last chance the debug_hooks have
>>               at looking at optimized away DECLs, since
>>               late_global_decl will subsequently be called from the
>>               contents of the now pruned symbol table.  */
>>            if (!decl_function_context (node->decl))
>>              (*debug_hooks->late_global_decl) (node->decl);
>>
>> Since gen_type_die_with_usage() cannot handle TEMPLATE_TYPE_PARMs we ICE.
>>
>> I think we need to avoid calling late_global_decl on DECL's for which
>> decl_type_context() is true, similarly to what we do for the call to
>> early_global_decl in rest_of_decl_compilation:
>>
>>        && !decl_function_context (decl)
>>        && !current_function_decl
>>        && DECL_SOURCE_LOCATION (decl) != BUILTINS_LOCATION
>>        && !decl_type_context (decl))
>>      (*debug_hooks->early_global_decl) (decl);
>>
>> Presumably the old code did not run into this problem because the
>> TEMPLATE_TYPE_PARAMs had been lowered by the time dwarf2out_decl was called,
>> but here we are calling late_global_decl relatively early.
>
> I think we need to sort out that instead - by the time we call _early_
> global decl it
> should already be "lowered".  Otherwise LTO streaming will run into
> the decl_type_context it cannot handle.  Is the case running into
> late_global_decl
> before we called early_global_decl on it btw?

Typically in C++ we call early_global_decl via:

	cp_finish_decl
	  -> make_rtl_for_nonlocal_decl
	    -> rest_of_decl_compilation
	      -> early_global_decl.

However, in this case we have not called early_global_decl on the DECL 
because cp_finish_decl avoids the make_rtl_for_nonlocal_decl path for 
templates:

cp_finish_decl():
   ...
   if (processing_template_decl)
     {
       bool type_dependent_p;
       ...
       ...
       return;
     }

Aldy
Jason Merrill June 25, 2015, 4:56 p.m. UTC | #5
On 06/24/2015 11:12 PM, Aldy Hernandez wrote:
> The problem here is that we are trying to call
> dwarf2out_late_global_decl() on a static variable in a template which
> has a type of TEMPLATE_TYPE_PARM:
>
> template <typename T> class A
> {
>    static __thread T a;
> };
>
> We are calling late_global_decl because we are about to remove the
> unused static from the symbol table:

The problem here is that 'a' should never have been in the symbol table 
in the first place, since it's an uninstantiated template.  It's there 
because of honza's change to store the TLS model in the symbol table, so 
TLS templates end up with varpool entries that, of course, will never be 
referenced.

I guess either we need to avoid putting these templates in the symbol 
table or we need to mark these fake entries somehow.

Jason
Eric Botcazou June 25, 2015, 8:36 p.m. UTC | #6
> There were some defined but not used warnings (??) that caused it to
> fail, especially since I didn't configure with --disable-werror.

Weird.  I bootstrap the compiler in default mode every day and I haven't had a 
bootstrap failure for at least a month.
Richard Biener June 26, 2015, 9:37 a.m. UTC | #7
On Thu, Jun 25, 2015 at 6:56 PM, Jason Merrill <jason@redhat.com> wrote:
> On 06/24/2015 11:12 PM, Aldy Hernandez wrote:
>>
>> The problem here is that we are trying to call
>> dwarf2out_late_global_decl() on a static variable in a template which
>> has a type of TEMPLATE_TYPE_PARM:
>>
>> template <typename T> class A
>> {
>>    static __thread T a;
>> };
>>
>> We are calling late_global_decl because we are about to remove the
>> unused static from the symbol table:
>
>
> The problem here is that 'a' should never have been in the symbol table in
> the first place, since it's an uninstantiated template.  It's there because
> of honza's change to store the TLS model in the symbol table, so TLS
> templates end up with varpool entries that, of course, will never be
> referenced.

Can we defer TLS model setting to template instantiation?

> I guess either we need to avoid putting these templates in the symbol table
> or we need to mark these fake entries somehow.
>
> Jason
>
Jason Merrill June 26, 2015, 9:59 p.m. UTC | #8
On 06/26/2015 05:37 AM, Richard Biener wrote:
> Can we defer TLS model setting to template instantiation?

We need to represent somehow that __thread (or thread_local) was used in 
the declaration, but DECL_THREAD_LOCAL_P was changed to refer to the TLS 
model.

Jason
Richard Biener June 29, 2015, 9:07 a.m. UTC | #9
On Fri, Jun 26, 2015 at 11:59 PM, Jason Merrill <jason@redhat.com> wrote:
> On 06/26/2015 05:37 AM, Richard Biener wrote:
>>
>> Can we defer TLS model setting to template instantiation?
>
>
> We need to represent somehow that __thread (or thread_local) was used in the
> declaration, but DECL_THREAD_LOCAL_P was changed to refer to the TLS model.

Ok, so "easiest" would be to allocate a bit from decl_with_vis for this...

Richard.

> Jason
>
Jason Merrill June 29, 2015, 10:32 p.m. UTC | #10
On 06/29/2015 05:07 AM, Richard Biener wrote:
> On Fri, Jun 26, 2015 at 11:59 PM, Jason Merrill <jason@redhat.com> wrote:
>> On 06/26/2015 05:37 AM, Richard Biener wrote:
>>>
>>> Can we defer TLS model setting to template instantiation?
>>
>> We need to represent somehow that __thread (or thread_local) was used in the
>> declaration, but DECL_THREAD_LOCAL_P was changed to refer to the TLS model.
>
> Ok, so "easiest" would be to allocate a bit from decl_with_vis for this...

Or I can find a flag in the front end.  I guess I'll do that.

Jason
diff mbox

Patch

diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 066a155..d2974ad 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -1149,7 +1149,8 @@  analyze_functions (bool first_time)
 	     at looking at optimized away DECLs, since
 	     late_global_decl will subsequently be called from the
 	     contents of the now pruned symbol table.  */
-	  if (!decl_function_context (node->decl))
+	  if (!decl_function_context (node->decl)
+	      && !decl_type_context (node->decl))
 	    (*debug_hooks->late_global_decl) (node->decl);
 
 	  node->remove ();
diff --git a/gcc/testsuite/g++.dg/debug/dwarf2/pr66653.C b/gcc/testsuite/g++.dg/debug/dwarf2/pr66653.C
new file mode 100644
index 0000000..bcaaf88
--- /dev/null
+++ b/gcc/testsuite/g++.dg/debug/dwarf2/pr66653.C
@@ -0,0 +1,8 @@ 
+// PR debug/54508
+// { dg-do compile }
+// { dg-options "-g" }
+
+template <typename T> class A
+{
+  static __thread T a;
+};