Re: [PING][PATCH][2/2] Early LTO debug -- main part

[Richard Biener <richard.guenther@gmail.com>]

Late response now that I'm finished refreshing the patches.

On Mon, Nov 28, 2016 at 6:20 PM, Jason Merrill <jason@redhat.com> wrote:
> On Thu, Nov 24, 2016 at 8:50 AM, Richard Biener <rguenther@suse.de> wrote:
>>> > +      /* ???  We can't annotate types late, but for LTO we may not
>>> > +    generate a location early either (gfortran.dg/save_5.f90).
>>> > +    The proper way is to handle it like VLAs though it is told
>>> > +    that DW_AT_string_length does not support this.  */
>>>
>>> I think go ahead and handle it like VLAs, this is an obvious generalization
>>> and should go into the spec soon enough.  This can happen later.
>>
>> Ok, note that VLAs are now handled by re-emitting types late to avoid
>> some guality regressions.  With inlining VLA types also get copied
>> so we'd need to re-design how we handle them a bit.
>> I'll see what the DWARF people come up with.
>
> Does the discussion in
> https://sourceware.org/bugzilla/show_bug.cgi?id=20426 cover the issue?

This covers the VLA issue, yes.  With the DW_OP_GNU_variable_value extension
we might be able to handle those better though gdb still lacks support.

For the Fortran case quoted above it might be possible to use
DW_OP_GNU_variable_value
as well but I'll leave that for followup improvements (handling it
"like VLAs" without
this extension isn't possible Jakub told me).

I've removed the reference to VLAs in the comment, added a -flto -g variant of
save_5.f90 to the testsuite and refer to that.

>
>>> > +     /* ???  This all (and above) should probably be simply
>>> > +        a ! early_dwarf check somehow.  */
>>> > +      && ((DECL_ARTIFICIAL (decl) || in_lto_p)
>>> >            || (get_AT_file (old_die, DW_AT_decl_file) == file_index
>>> >                && (get_AT_unsigned (old_die, DW_AT_decl_line)
>>> >                    == (unsigned) s.line))))
>>>
>>> Why doesn't the existing source position check handle the LTO case? Also the
>>> extra parens aren't necessary.
>>
>> Because in LTRANS we do not see those attributes anymore but they are
>> present in the early created DIEs.
>
> Ah, OK.  But the comment seems wrong, since we go through here in
> early dwarf for local class methods.

Changed the comment to

/* ???  In LTO we do not see any of the location attributes.  */

>>> > +init_sections_and_labels (bool early_lto_debug)
>>>
>>> You're changing this function to do the same thing in four slightly different
>>> ways rather than two.  I'd rather control each piece as appropriate; we ought
>>> to make SECTION_DEBUG or SECTION_DEBUG|SECTION_EXCLUDE a local variable, and
>>> select between *_SECTION and the DWO variant at each statement rather than in
>>> different blocks.
>>
>> Note that the section names change between LTO, LTO_DWO, DWO and
>> regular section names.  It's basically modeled after what we have now
>> which switches between regular and DWO section names.  We might
>> be able to refactor this with a new array
>>
>> enum section_kind { NORMAL, DWO, LTO, LTO_DWO };
>> char **section_names[section_kind][] = { { DEBUG_INFO_SECTION, ... },
>> { DEBUG_DWO_INFO_SECTION, ... },
>> { DEBUG_LTO_INFO_SECTION, ... },
>> { DEBUG_LTO_DWO_INFO_SECTION, .. } };
>>
>> would you prefer that?
>
> That sounds better, thanks.

I tried a few variants but they all end up even more awkward ...

Given the ugliness is isolated in init_sections_and_labels I think
keeping it the
way it is is best.

Updated patches posted separately (I posted the diff to the previous
state already).

Thanks,
Richard.

>
> Jason