Patchwork Fix the most annoying LTO -g ICE in dwarf2out.c

login
register
mail settings
Submitter Richard Guenther
Date Oct. 8, 2010, 2:58 p.m.
Message ID <alpine.LNX.2.00.1010081602350.8982@zhemvz.fhfr.qr>
Download mbox | patch
Permalink /patch/67228/
State New
Headers show

Comments

Richard Guenther - Oct. 8, 2010, 2:58 p.m.
The most frequent bug I run into is PR45089 where LTO manages to
not output a type that dwarf wants to reference via a AT_die_ref.
This causes us to later crash in dwarf2out.c at the various places
the target of the AT_die_ref is referenced.  An easy workaround is
to avoid generating this attribute in the first place if the
target is NULL (and it's much easier than to try fixup the consumers).

Bootstrapped and tested on x86_64-unknown-linux-gnu.  

This fixes all extra ICEs I get when building SPEC 2006 with -g in 
addition to -fwhopr -fwhole-program (this bug causes the build of 8
benchmarks to fail).

Jason, does this sound like a reasonable workaround (yes, I'll be
eventually looking for a way to avoid the situation in the first place,
but it involves type-merging, a very fragile piece of code ...)?

Thanks,
Richard.

2010-10-08  Richard Guenther  <rguenther@suse.de>

	PR lto/45089
	* dwarf2out.c (add_AT_die_ref): Work around LTO losing types.
Jason Merrill - Oct. 11, 2010, 8:42 p.m.
On 10/08/2010 10:58 AM, Richard Guenther wrote:
> Jason, does this sound like a reasonable workaround (yes, I'll be
> eventually looking for a way to avoid the situation in the first place,
> but it involves type-merging, a very fragile piece of code ...)?

It seems like a fine workaround for release branches, but when 
ENABLE_CHECKING is on I think we want it to crash as motivation for 
fixing the real bug.

Jason
Richard Guenther - Oct. 12, 2010, 9 a.m.
On Mon, 11 Oct 2010, Jason Merrill wrote:

> On 10/08/2010 10:58 AM, Richard Guenther wrote:
> > Jason, does this sound like a reasonable workaround (yes, I'll be
> > eventually looking for a way to avoid the situation in the first place,
> > but it involves type-merging, a very fragile piece of code ...)?
> 
> It seems like a fine workaround for release branches, but when ENABLE_CHECKING
> is on I think we want it to crash as motivation for fixing the real bug.

Ok, that works for me.  I'll wrap the bailout in ENABLE_CHECKING and
add an assert if checking is enabled.

Thanks,
Richard.
Richard Guenther - Oct. 18, 2010, 4:02 p.m.
On Tue, 12 Oct 2010, Richard Guenther wrote:

> On Mon, 11 Oct 2010, Jason Merrill wrote:
> 
> > On 10/08/2010 10:58 AM, Richard Guenther wrote:
> > > Jason, does this sound like a reasonable workaround (yes, I'll be
> > > eventually looking for a way to avoid the situation in the first place,
> > > but it involves type-merging, a very fragile piece of code ...)?
> > 
> > It seems like a fine workaround for release branches, but when ENABLE_CHECKING
> > is on I think we want it to crash as motivation for fixing the real bug.
> 
> Ok, that works for me.  I'll wrap the bailout in ENABLE_CHECKING and
> add an assert if checking is enabled.

On a second thought, as the problem happens here:

static inline void
add_pure_or_virtual_attribute (dw_die_ref die, tree func_decl)
{
  if (DECL_VINDEX (func_decl))
    {
      add_AT_unsigned (die, DW_AT_virtuality, DW_VIRTUALITY_virtual);

      if (host_integerp (DECL_VINDEX (func_decl), 0))
        add_AT_loc (die, DW_AT_vtable_elem_location,
                    new_loc_descr (DW_OP_constu,
                                   tree_low_cst (DECL_VINDEX (func_decl), 
0),
                                   0));

      /* GNU extension: Record what type this method came from originally.  
*/
      if (debug_info_level > DINFO_LEVEL_TERSE
          && DECL_CONTEXT (func_decl))
        add_AT_die_ref (die, DW_AT_containing_type,
                        lookup_type_die (DECL_CONTEXT (func_decl)));

DECL_CONTEXT is non-NULL but lookup_type_die fails.  We can also
not emit DW_AT_containing_type if the lookup failed (similar to the
DECL_CONTEXT check), or even use force_type_die here.  Both work
for the testcase.  A workaround on the LTO side would be to clear
DECL_CONTEXT on such functions.

Thanks,
Richard.
Jason Merrill - Oct. 18, 2010, 9:17 p.m.
On 10/18/2010 12:02 PM, Richard Guenther wrote:
> DECL_CONTEXT is non-NULL but lookup_type_die fails.  We can also
> not emit DW_AT_containing_type if the lookup failed (similar to the
> DECL_CONTEXT check), or even use force_type_die here.   Both work
> for the testcase.

I guess omitting the attribute when !ENABLE_CHECKING is also ok.

> A workaround on the LTO side would be to clear
> DECL_CONTEXT on such functions.

On which functions?  How do you detect on the LTO side that type merging 
has gone wrong?

Jason
Richard Guenther - Oct. 19, 2010, 9:13 a.m.
On Mon, 18 Oct 2010, Jason Merrill wrote:

> On 10/18/2010 12:02 PM, Richard Guenther wrote:
> > DECL_CONTEXT is non-NULL but lookup_type_die fails.  We can also
> > not emit DW_AT_containing_type if the lookup failed (similar to the
> > DECL_CONTEXT check), or even use force_type_die here.   Both work
> > for the testcase.
> 
> I guess omitting the attribute when !ENABLE_CHECKING is also ok.

Ok.

> > A workaround on the LTO side would be to clear
> > DECL_CONTEXT on such functions.
> 
> On which functions?  How do you detect on the LTO side that type merging has
> gone wrong?

On all functions which have a record or union type DECL_CONTEXT
(all functions that are in some TYPE_METHODS which we do not
stream).  Of course I can't detect whether type merging has gone wrong.

Richard.
Jason Merrill - Oct. 19, 2010, 9:47 p.m.
On 10/19/2010 05:13 AM, Richard Guenther wrote:
>>> A workaround on the LTO side would be to clear
>>> DECL_CONTEXT on such functions.
>>
>> On which functions?  How do you detect on the LTO side that type merging has
>> gone wrong?
>
> On all functions which have a record or union type DECL_CONTEXT
> (all functions that are in some TYPE_METHODS which we do not
> stream).  Of course I can't detect whether type merging has gone wrong.

That would be overkill, stipping debug info even when the types are ok.

Jason

Patch

Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	(revision 165169)
+++ gcc/dwarf2out.c	(working copy)
@@ -7335,6 +7335,11 @@  add_AT_die_ref (dw_die_ref die, enum dwa
 {
   dw_attr_node attr;
 
+  /* With LTO we can end up trying to reference something we didn't create
+     a DIE for.  Avoid crashing later on a NULL referenced DIE.  */
+  if (targ_die == NULL)
+    return;
+
   attr.dw_attr = attr_kind;
   attr.dw_attr_val.val_class = dw_val_class_die_ref;
   attr.dw_attr_val.v.val_die_ref.die = targ_die;