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

Thu Jan 14 17:10:21 PST 2016

On Thu, Jan 14, 2016 at 5:03 PM, Adrian Prantl <aprantl at apple.com> wrote:

>
> On Jan 14, 2016, at 4:50 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Thu, Jan 14, 2016 at 4:46 PM, Adrian Prantl <aprantl at apple.com> wrote:
>
>>
>> On Jan 14, 2016, at 4:06 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>>
>> On Thu, Jan 14, 2016 at 4:00 PM, Adrian Prantl <aprantl at apple.com> wrote:
>>
>>>
>>> On 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?
>>>
>>>
>>> The silly example I had in mind was something like:
>>>
>>> namespace N {
>>>   void foo();
>>> }
>>>
>>> void foo() {
>>>   using N::foo;
>>>   // stop here and evaluate "foo()"
>>>
>>
>> Right - but in this module, there is no definition of 'foo', so there's
>> no way to stop at it.
>>
>> In whatever other module has a definition of 'foo', we would expect the
>> using declaration's debug info will be present there, no?
>>
>> If I'm understanding the situation correctly...
>>
>>
>> It almost looks like we’re thinking of different situations. My
>> understanding of the problem was that there is an imported entity
>> referencing a subprogram that was optimized away, and the question was
>> whether it was safe to remove the imported entity if the subprogram doesn’t
>> exist any more. The point I was trying to make was that it is never safe to
>> remove an imported entity because it changes the name lookup at the site of
>> the import (::foo in my example).
>>
>
> But it's ::foo that's been optimized away, right? So in what situation are
> you doing name lookup in that scope?
>
> In my example N::foo is optimized away and the developer breaks in ::foo
> and evaluates foo().
>
>
> I think this is about the same as if the function was never called/code
> generated (eg: Clang skips emitting functions (& there imported entities,
> etc) if the function is not called (& it's not externally visible - so
> inline, or static, etc)).
>
>
> Yes, the if(false) branch could be removed from my example without loss of
> generality.
>
> Seems OK to have the same result if a function is optimized away, or
> otherwise removed (in the case of ThinLTO it's not so much that the
> subprogram is optimized away, but that it's not imported into a certain
> module (or not needed because something else was removed, I guess))
>
>
> It’s ok to optimize the function away but never* an import that function.
>
> *) unless, e.g., the enclosing scope is optimized away
>

Oh, I may have this the wrong way around indeed... - I was thinking we were
removing (or trying to avoid removing) the enclosing scope of the imported
directive - I see, perhaps we are trying to avoid removing the destination.
Which, yes, I agree, we should avoid removing the destination even if we
have to mutate it from definition to declaration.

& this also explains why local types wouldn't be related here - they don't
reference another function. When I was thinking that it was the /scope/
that was the problem, local types made sense to be another instance of this
problem.

OK, everyone carry on about your business & ignore me. Thanks for setting
me straight, Adrian.

- Dave

>
>
>
>>
>>   if (false)
>>>     foo(); // optimized out
>>> }
>>>
>>> -- adrian
>>>
>>> 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.
>>>
>>> - 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
>>>>
>>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160114/35902b5a/attachment.html>