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

Thu Jan 14 16:46:00 PST 2016

> On Jan 14, 2016, at 4:06 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Thu, Jan 14, 2016 at 4:00 PM, Adrian Prantl <aprantl at apple.com <mailto:aprantl at apple.com>> wrote:
> 
>> On Jan 8, 2016, at 10:18 AM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>> 
>> 
>> 
>> On Wed, Jan 6, 2016 at 10:38 AM, Adrian Prantl <aprantl at apple.com <mailto:aprantl at apple.com>> wrote:
>> 
>>> On Jan 6, 2016, at 10:27 AM, Eric Christopher <echristo at gmail.com <mailto:echristo at gmail.com>> wrote:
>>> 
>>> 
>>> 
>>> On Wed, Jan 6, 2016 at 10:04 AM Duncan P. N. Exon Smith <dexonsmith at apple.com <mailto:dexonsmith at apple.com>> wrote:
>>> +aprantl
>>> 
>>> > On 2016-Jan-06, at 09:46, Teresa Johnson <tejohnson at google.com <mailto: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 <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?
>> 
> 
> The silly example I had in mind was something like:
> 
> namespace N {
>   void foo();
> }
> 
> void foo() {
>   using N::foo;
>   // stop here and evaluate "foo()"
> 
> Right - but in this module, there is no definition of 'foo', so there's no way to stop at it.
> 
> In whatever other module has a definition of 'foo', we would expect the using declaration's debug info will be present there, no?
> 
> If I'm understanding the situation correctly... 

It almost looks like we’re thinking of different situations. My understanding of the problem was that there is an imported entity referencing a subprogram that was optimized away, and the question was whether it was safe to remove the imported entity if the subprogram doesn’t exist any more. The point I was trying to make was that it is never safe to remove an imported entity because it changes the name lookup at the site of the import (::foo in my example).

>   if (false)
>     foo(); // optimized out
> }
> 
> -- adrian
> 
>> 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.
>> 
>> - 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 <mailto: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 <mailto: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 <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 <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 <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 <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 <mailto:tejohnson at google.com> | 408-460-2413 <tel:408-460-2413>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160114/286740c3/attachment.html>