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

Wed Jan 6 14:03:33 PST 2016

> On 2016-Jan-06, at 13:33, Teresa Johnson <tejohnson at google.com> wrote:
> 
> Here is the change I made which fixes the test case. I can add some
> comments and a test case, but let me know if this looks ok otherwise.
> As Eric noted, the DISubroutine for a() is marked as a definition,
> which it isn't actually, but this was the behavior before this patch
> and seems to work ok.

Since this calling it a definition isn't a regression I agree this
is reasonable, but can you add a FIXME somewhere relevant?

Besides that (and adding a testcase) I think you should just go
ahead and unbrick the bot.

> Thanks,
> Teresa
> 
> diff --git a/lib/Linker/IRMover.cpp b/lib/Linker/IRMover.cpp
> index 309690f..9900ecc 100644
> --- a/lib/Linker/IRMover.cpp
> +++ b/lib/Linker/IRMover.cpp
> @@ -1211,6 +1211,12 @@ void
> IRLinker::findNeededSubprograms(ValueToValueMapTy &ValueMap) {
>   for (unsigned I = 0, E = CompileUnits->getNumOperands(); I != E; ++I) {
>     auto *CU = cast<DICompileUnit>(CompileUnits->getOperand(I));
>     assert(CU && "Expected valid compile unit");
> +    SmallPtrSet<DISubprogram *, 8> ImportedEntitySPs;
> +    for (auto *IE : CU->getImportedEntities()) {
> +      assert(IE->getEntity());
> +      if (auto *SP = dyn_cast<DISubprogram>(IE->getEntity()))
> +        ImportedEntitySPs.insert(SP);
> +    }
>     for (auto *Op : CU->getSubprograms()) {
>       // Unless we were doing function importing and deferred metadata linking,
>       // any needed SPs should have been mapped as they would be reached
> @@ -1218,7 +1224,7 @@ void
> IRLinker::findNeededSubprograms(ValueToValueMapTy &ValueMap) {
>       // function bodies, or from DILocation on inlined instructions).
>       assert(!(ValueMap.MD()[Op] && IsMetadataLinkingPostpass) &&
>              "DISubprogram shouldn't be mapped yet");
> -      if (!ValueMap.MD()[Op])
> +      if (!ValueMap.MD()[Op] && !ImportedEntitySPs.count(Op))
>         UnneededSubprograms.insert(Op);
>     }
>   }
> 
> 
> On Wed, Jan 6, 2016 at 10:45 AM, Teresa Johnson <tejohnson at google.com> wrote:
>> On Wed, Jan 6, 2016 at 10:37 AM, Teresa Johnson <tejohnson at google.com> wrote:
>>> Thanks Duncan and Eric.
>>> 
>>> I should also be able to implement #1 very quickly and get it in today
>>> if we want a workaround while we figure out what is the right approach
>>> here. Will take a stab at that now.
>> 
>> Oh, missed your point about needing to change the DISubprogram for a()
>> from a definition to a declaration. However, presumably that worked ok
>> before this patch went in, since we would have had the same issue?
>> 
>>> 
>>> (Somehow I feel the need to apologize for the overly cute subject - my
>>> intended "question" got turned into "questy", presumably by fat
>>> fingers. =(  =)  )
>>> 
>>> Teresa
>>> 
>>> On Wed, 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.
>>>> 
>>>>> 
>>>>> 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.
>>>> 
>>>> -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
>> 
>> 
>> 
>> --
>> Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413
> 
> 
> 
> -- 
> Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413