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

Wed Jan 6 14:21:47 PST 2016

Oh got it, thanks. Will do that and the other suggestions (e.g. test
and add FIXME) and then commit.
Thanks,
Teresa

On Wed, Jan 6, 2016 at 2:20 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>> On 2016-Jan-06, at 14:17, Teresa Johnson via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>
>> On Wed, Jan 6, 2016 at 2:11 PM, Adrian Prantl <aprantl at apple.com> wrote:
>>>
>>>> On Jan 6, 2016, at 1:33 PM, 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.
>>>>
>>>> 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()))
>>>
>>> This is silly nitpicking, but the assertion followed by a dyn_cast is redundant (cf. dyn_cast_or_null).
>>
>> I could use dyn_cast_or_null instead of the assert. But I wanted to
>> catch a case where we somehow already ended up in the bad situation of
>> an imported entity with a null entity. The dyn_cast_or_null would
>> handle it silently, right?
>>
>
> I think Adrian's point is that the dyn_cast<>() already crashes on
> null, so you can remove the assert() on the previous line.
>
>> Teresa
>>
>>>
>>> -- adrian
>>>
>>>> +        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
>>>
>>
>>
>>
>> --
>> Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>

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