Debug info questy (was Re: [llvm] r256003 - [ThinLTO/LTO] Don't link in unneeded metadata)
Eric Christopher via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 6 14:05:53 PST 2016
On Wed, Jan 6, 2016 at 2:03 PM Duncan P. N. Exon Smith <dexonsmith at apple.com>
wrote:
>
> > 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.
>
>
Agreed.
Thanks!
-eric
> > 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160106/668c616a/attachment.html>
More information about the llvm-commits
mailing list