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

[Duncan P. N. Exon Smith via llvm-commits]

+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.

In the meantime, if (3) isn't too intrusive, it would be worth doing temporarily to unbreak the bot...

> 
> 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