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

Thu Jan 14 14:27:51 PST 2016

On Fri, Jan 8, 2016 at 11:25 AM, Teresa Johnson <tejohnson at google.com>
wrote:

> On Fri, Jan 8, 2016 at 10:18 AM, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> > On Wed, Jan 6, 2016 at 10:38 AM, Adrian Prantl <aprantl at apple.com>
> wrote:
> >>
> >>
> >> On 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.
> >>
> >>
> >> Right. Dropping the imported entity because it’s not in the current
> >> translation unit would change the overload resolution in the debugger.
> >
> >
> > I'm not sure I follow here - if the function is never emitted (which it
> > isn't in this translation unit) where would overload resolution come into
> > it?
> >
> > OK, /maybe/ if the subprogram had a local type that leaked out somewhere
> > (eg: through a template instantiation that lead to a global variable that
> > stayed alive despite the outer function being removed) and you were
> inside a
> > member function of that local type you could rely on the imported
> > declaration to provide name lookup. But in that case you'd still need to
> > preserve the subprogram for the nested type to be the child of.
> >
> > Adrian: What name resolution situation did you have in mind?
> >
> > Teresa: Have you looked at/hit this issue with local types? I might be
> able
> > to conjure a specific example (using the aforementioned template
> scenario)
> > that could tickle it in that direction if that'd be helpful.
>
> I haven't hit any issues, but then again I haven't done any really
> in-depth analysis of the quality of the debug info pruning in this
> patch beyond some of the regression test cases.

Yeah, if this DIImportedEntity issue was hitting you in the form of a
crash/assert, I would imagine a local nested type (eg: "void func() {
struct foo { }; ... }" - also pretty much every use of lambdas) would
produce the same behavior, but may just not have come up in your testing so
far... or there's something different about types that doesn't cause the
same problem, which might be interesting to understand. (removing those is
going to be trickier because they may need to stick around for other
reasons (the local type may've been used in a template instantiation that's
not been eliminated/removed, etc))

> I also haven't tried
> removing DIImportedEntity that correspond to non-linked in functions.
>
> If you and Adrian conclude it is worthwhile and legal to try doing
> this (removing both the DISubprogram and DIImportedEntity for any
> non-linked in functions), I can add a FIXME and probably attack that
> next week. In that case, any examples of cases where we couldn't
> remove the DIImportedEntity would be really helpful.
>

Yep <ping Adrian>. I'll wait until he chimes in again & we can discuss
there.

- Dave

>
> Thanks,
> Teresa
>
> >
> > - Dave
> >
> >>
> >>
> >>
> >>>
> >>> 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.
> >>
> >>
> >> Agreed.
> >>
> >>
> >> -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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160114/cf57f025/attachment.html>