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

[Teresa Johnson via llvm-commits]

Ah, ok. Will treat the scopes on the imported entities the same (add
them to the new set we're checking against, and add a suitable test
case). Might not get that in until tomorrow morn.

Thanks,
Teresa

On Wed, Jan 6, 2016 at 4:23 PM, Ahmed Bougacha <ahmed.bougacha at gmail.com> wrote:
> I'm sorry, but I spoke too soon!
> It turns out, this fixes llc, but later in the build, other binaries
> hit a similar problem, from elsewhere though:
>
>     frame #9: 0x0000000102c07984
> libLTO.dylib`llvm::DIScope::getRawFile(this=0x0000000000000000) const
> + 36 at DebugInfoMetadata.h:430
>     frame #10: 0x0000000102c07875
> libLTO.dylib`llvm::DIScope::getFile(this=0x0000000000000000) const +
> 21 at DebugInfoMetadata.h:415
>     frame #11: 0x0000000102beba25
> libLTO.dylib`llvm::DIScope::getFilename(this=0x0000000000000000) const
> + 21 at DebugInfoMetadata.h:503
>     frame #12: 0x0000000103907f06
> libLTO.dylib`llvm::DwarfCompileUnit::constructImportedEntityDIE(this=0x00000001e5847090,
> Module=0x000000014fc5a5a8) + 614 at DwarfCompileUnit.cpp:659
>     frame #13: 0x00000001039151cb
> libLTO.dylib`llvm::DwarfDebug::constructAndAddImportedEntityDIE(this=0x00000001c8fcbc00,
> TheCU=0x00000001e5847090, N=0x000000014fc5a5a8) + 91 at
> DwarfDebug.cpp:456
>
> Looks like it's the scope that's missing this time:
>
> (lldb) f 12
> frame #12: 0x0000000103907f06
> libLTO.dylib`llvm::DwarfCompileUnit::constructImportedEntityDIE(this=0x00000001e5847090,
> Module=0x000000014fc5a5a8) + 614 at DwarfCompileUnit.cpp:659
>    656  else
>    657    EntityDie = getDIE(Entity);
>    658  assert(EntityDie);
> -> 659  addSourceLine(*IMDie, Module->getLine(),
> Module->getScope()->getFilename(),
>    660                Module->getScope()->getDirectory());
>    661  addDIEEntry(*IMDie, dwarf::DW_AT_import, *EntityDie);
>    662  StringRef Name = Module->getName();
> (lldb) p ::Module->dump()
> <0x14fc5a5a8> = !DIImportedEntity(tag: DW_TAG_imported_module, scope:
> null, entity: <0x154545c78>, line: 154)
>
> Sorry about missing that!
> -Ahmed
>
>
> On Wed, Jan 6, 2016 at 4:11 PM, Teresa Johnson <tejohnson at google.com> wrote:
>> Great, thanks for checking. Committed at r257000.
>> Teresa
>>
>> On Wed, Jan 6, 2016 at 2:22 PM, Ahmed Bougacha <ahmed.bougacha at gmail.com> wrote:
>>> On Wed, 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.
>>>
>>> FWIW, this patch [0] fixes the issue [1] for me.
>>>
>>> Thanks for taking care of this Teresa!
>>> -Ahmed
>>>
>>> [0] on r256768 and with a fix for the unrelated PR25919
>>> [1] llc build, OS X, -O2 -flto -g
>>>
>>>> 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);
>>>>      }
>>>>    }
>>
>>
>>
>> --
>> Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413



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