Debug info questy (was Re: [llvm] r256003 - [ThinLTO/LTO] Don't link in unneeded metadata)
Teresa Johnson via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 6 13:33:56 PST 2016
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()))
+ 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
More information about the llvm-commits
mailing list