Debug info questy (was Re: [llvm] r256003 - [ThinLTO/LTO] Don't link in unneeded metadata)

Fri Jan 8 11:25:49 PST 2016

On Fri, Jan 8, 2016 at 10:18 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Wed, Jan 6, 2016 at 10:38 AM, Adrian Prantl <aprantl at apple.com> wrote:
>>
>>
>> On Jan 6, 2016, at 10:27 AM, Eric Christopher <echristo at gmail.com> wrote:
>>
>>
>>
>> On Wed, Jan 6, 2016 at 10:04 AM Duncan P. N. Exon Smith
>> <dexonsmith at apple.com> wrote:
>>>
>>> +aprantl
>>>
>>> > On 2016-Jan-06, at 09:46, Teresa Johnson <tejohnson at google.com> wrote:
>>> >
>>> > +dexonsmith,echristo,blaikie who are way more familiar with debug info
>>> > than I
>>> > +pcc who also found this and contacted me off-list with a possible
>>> > solution
>>> >
>>> > See also https://llvm.org/bugs/show_bug.cgi?id=26037 that I updated
>>> > with a test case and more detailed analysis.
>>> >
>>> > The issue is essentially that we no longer pull in a DISubprogram on
>>> > an LTO link with this patch because the associated function was not
>>> > linked in. However, it had been referenced by a DIImportedEntity which
>>> > is leaving behind a DIImportedEntity with no entity:
>>> >
>>> > !11 = !DIImportedEntity(tag: DW_TAG_imported_declaration, scope: !8,
>>> > line: 2)
>>> >
>>> > which in turn causes an assert during debug generation. The 3
>>> > solutions I mention there are:
>>> >
>>> > 1) Change findNeededSubprograms to look at entities and conservatively
>>> > remove any subprograms reached from there from the UnneededSubprograms
>>> > list
>>> > 2) Remove any DIImportedEntity that no longer have an entity
>>> > 3) Change the debug generation code to handle DIImportedEntity with a
>>> > null entity
>>> >
>>> > pcc has a tentative solution that implements 3) above.
>>> >
>>> > Debug info experts - what is the right thing to do here? Is it legal
>>> > to have a DIImportedEntity with no entity? If so, pcc's solution
>>> > (solution 3 above) is the right one. Otherwise I should implement 1)
>>> > or 2).
>>>
>>> My intuition says that (2) is the correct solution, but Adrian would know
>>> best.
>>>
>>
>> It's a bit iffy:
>>
>> 3.2.3 Imported (or Renamed) Declaration Entries Some languages support the
>> concept of importing into or making accessible in a given unit declarations
>> made in a different module or scope. An imported declaration may sometimes
>> be given another name. An imported declaration is represented by one or more
>> debugging information entries with the tag DW_TAG_imported_declaration. When
>> an overloaded entity is imported, there is one imported declaration entry
>> for each overloading.
>>
>> which means that we're probably looking at 1 as we care about declarations
>> for entities rather than defining declarations. That said, unless we change
>> the metadata to be a declaration of a subprogram rather than a defining
>> declaration we're going to have a bad time further down the line.
>>
>>
>> Right. Dropping the imported entity because it’s not in the current
>> translation unit would change the overload resolution in the debugger.
>
>
> I'm not sure I follow here - if the function is never emitted (which it
> isn't in this translation unit) where would overload resolution come into
> it?
>
> OK, /maybe/ if the subprogram had a local type that leaked out somewhere
> (eg: through a template instantiation that lead to a global variable that
> stayed alive despite the outer function being removed) and you were inside a
> member function of that local type you could rely on the imported
> declaration to provide name lookup. But in that case you'd still need to
> preserve the subprogram for the nested type to be the child of.
>
> Adrian: What name resolution situation did you have in mind?
>
> Teresa: Have you looked at/hit this issue with local types? I might be able
> to conjure a specific example (using the aforementioned template scenario)
> that could tickle it in that direction if that'd be helpful.

I haven't hit any issues, but then again I haven't done any really
in-depth analysis of the quality of the debug info pruning in this
patch beyond some of the regression test cases. I also haven't tried
removing DIImportedEntity that correspond to non-linked in functions.

If you and Adrian conclude it is worthwhile and legal to try doing
this (removing both the DISubprogram and DIImportedEntity for any
non-linked in functions), I can add a FIXME and probably attack that
next week. In that case, any examples of cases where we couldn't
remove the DIImportedEntity would be really helpful.

Thanks,
Teresa

>
> - Dave
>
>>
>>
>>
>>>
>>> In the meantime, if (3) isn't too intrusive, it would be worth doing
>>> temporarily to unbreak the bot...
>>
>>
>> This is absolutely ok as long as it's temporary while we figure this out.
>>
>>
>> Agreed.
>>
>>
>> -eric
>>
>>>
>>>
>>> >
>>> > Thanks,
>>> > Teresa
>>> >
>>> >
>>> > On Tue, Jan 5, 2016 at 4:24 PM, Ahmed Bougacha
>>> > <ahmed.bougacha at gmail.com> wrote:
>>> >> On Fri, Dec 18, 2015 at 9:51 AM, Teresa Johnson via llvm-commits
>>> >> <llvm-commits at lists.llvm.org> wrote:
>>> >>> Author: tejohnson
>>> >>> Date: Fri Dec 18 11:51:37 2015
>>> >>> New Revision: 256003
>>> >>>
>>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=256003&view=rev
>>> >>> Log:
>>> >>> [ThinLTO/LTO] Don't link in unneeded metadata
>>> >>>
>>> >>> Summary:
>>> >>> Third patch split out from http://reviews.llvm.org/D14752.
>>> >>>
>>> >>> Only map in needed DISubroutine metadata (imported or otherwise
>>> >>> linked
>>> >>> in functions and other DISubroutine referenced by inlined
>>> >>> instructions).
>>> >>> This is supported for ThinLTO, LTO and llvm-link --only-needed, with
>>> >>> associated tests for each one.
>>> >>>
>>> >>> Depends on D14838.
>>> >>>
>>> >>> Reviewers: dexonsmith, joker.eph
>>> >>>
>>> >>> Subscribers: davidxl, llvm-commits, joker.eph
>>> >>>
>>> >>> Differential Revision: http://reviews.llvm.org/D14843
>>> >>>
>>> >>> Added:
>>> >>>    llvm/trunk/test/Linker/Inputs/only-needed-debug-metadata.ll
>>> >>>    llvm/trunk/test/Linker/only-needed-debug-metadata.ll
>>> >>> Modified:
>>> >>>    llvm/trunk/include/llvm/Transforms/Utils/ValueMapper.h
>>> >>>    llvm/trunk/lib/Linker/IRMover.cpp
>>> >>>    llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp
>>> >>>    llvm/trunk/test/Linker/thinlto_funcimport_debug.ll
>>> >>>    llvm/trunk/test/tools/gold/X86/Inputs/linkonce-weak.ll
>>> >>>    llvm/trunk/test/tools/gold/X86/linkonce-weak.ll
>>> >>>
>>> >>> Modified: llvm/trunk/lib/Linker/IRMover.cpp
>>> >>> URL:
>>> >>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Linker/IRMover.cpp?rev=256003&r1=256002&r2=256003&view=diff
>>> >>>
>>> >>> ==============================================================================
>>> >>> --- llvm/trunk/lib/Linker/IRMover.cpp (original)
>>> >>> +++ llvm/trunk/lib/Linker/IRMover.cpp Fri Dec 18 11:51:37 2015
>>> >>> @@ -487,6 +495,16 @@ class IRLinker {
>>> >>>
>>> >>>   void linkNamedMDNodes();
>>> >>>
>>> >>> +  /// Populate the UnneededSubprograms set with the DISubprogram
>>> >>> metadata
>>> >>> +  /// from the source module that we don't need to link into the
>>> >>> dest module,
>>> >>> +  /// because the functions were not imported directly or via an
>>> >>> inlined body
>>> >>> +  /// in an imported function.
>>> >>> +  void findNeededSubprograms(ValueToValueMapTy &ValueMap);
>>> >>
>>> >> Hey Teresa,
>>> >>
>>> >> This has been causing bootstrap assertion failures, I think because a
>>> >> DISubprogram referenced by DIImportedEntity should also be considered
>>> >> needed.
>>> >>
>>> >> I filed PR26037, could you please have a look?
>>> >>
>>> >> Thanks!
>>> >> -Ahmed
>>> >
>>> >
>>> >
>>> > --
>>> > Teresa Johnson | Software Engineer | tejohnson at google.com |
>>> > 408-460-2413
>>
>>
>

-- 
Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413