<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 6, 2016 at 10:38 AM, Adrian Prantl <span dir="ltr"><<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div><div class="h5"><blockquote type="cite"><div>On Jan 6, 2016, at 10:27 AM, Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>> wrote:</div><br><div><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Jan 6, 2016 at 10:04 AM Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">+aprantl<br><br>> On 2016-Jan-06, at 09:46, Teresa Johnson <<a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a>> wrote:<br>><br>> +dexonsmith,echristo,blaikie who are way more familiar with debug info than I<br>> +pcc who also found this and contacted me off-list with a possible solution<br>><br>> See also<span> </span><a href="https://llvm.org/bugs/show_bug.cgi?id=26037" rel="noreferrer" target="_blank">https://llvm.org/bugs/show_bug.cgi?id=26037</a><span> </span>that I updated<br>> with a test case and more detailed analysis.<br>><br>> The issue is essentially that we no longer pull in a DISubprogram on<br>> an LTO link with this patch because the associated function was not<br>> linked in. However, it had been referenced by a DIImportedEntity which<br>> is leaving behind a DIImportedEntity with no entity:<br>><br>> !11 = !DIImportedEntity(tag: DW_TAG_imported_declaration, scope: !8, line: 2)<br>><br>> which in turn causes an assert during debug generation. The 3<br>> solutions I mention there are:<br>><br>> 1) Change findNeededSubprograms to look at entities and conservatively<br>> remove any subprograms reached from there from the UnneededSubprograms<br>> list<br>> 2) Remove any DIImportedEntity that no longer have an entity<br>> 3) Change the debug generation code to handle DIImportedEntity with a<br>> null entity<br>><br>> pcc has a tentative solution that implements 3) above.<br>><br>> Debug info experts - what is the right thing to do here? Is it legal<br>> to have a DIImportedEntity with no entity? If so, pcc's solution<br>> (solution 3 above) is the right one. Otherwise I should implement 1)<br>> or 2).<br><br>My intuition says that (2) is the correct solution, but Adrian would know best.<br><br></blockquote><div><br></div><div>It's a bit iffy:</div><div><br></div><div>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.<br></div><div><br></div><div>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.</div></div></div></div></blockquote><div><br></div></div></div>Right. Dropping the imported entity because it’s not in the current translation unit would change the overload resolution in the debugger. </div></div></blockquote><div><br>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?<br><br>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.<br><br>Adrian: What name resolution situation did you have in mind?<br><br>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.<br><br>- Dave<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><span class=""><br><blockquote type="cite"><div><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">In the meantime, if (3) isn't too intrusive, it would be worth doing temporarily to unbreak the bot...<br></blockquote><div><br></div><div>This is absolutely ok as long as it's temporary while we figure this out.</div></div></div></div></blockquote><div><br></div></span>Agreed.<div><div class="h5"><br><blockquote type="cite"><div><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="gmail_quote"><div><br></div><div>-eric</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br>><br>> Thanks,<br>> Teresa<br>><br>><br>> On Tue, Jan 5, 2016 at 4:24 PM, Ahmed Bougacha <<a href="mailto:ahmed.bougacha@gmail.com" target="_blank">ahmed.bougacha@gmail.com</a>> wrote:<br>>> On Fri, Dec 18, 2015 at 9:51 AM, Teresa Johnson via llvm-commits<br>>> <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>>>> Author: tejohnson<br>>>> Date: Fri Dec 18 11:51:37 2015<br>>>> New Revision: 256003<br>>>><br>>>> URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project?rev=256003&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=256003&view=rev</a><br>>>> Log:<br>>>> [ThinLTO/LTO] Don't link in unneeded metadata<br>>>><br>>>> Summary:<br>>>> Third patch split out from<span> </span><a href="http://reviews.llvm.org/D14752" rel="noreferrer" target="_blank">http://reviews.llvm.org/D14752</a>.<br>>>><br>>>> Only map in needed DISubroutine metadata (imported or otherwise linked<br>>>> in functions and other DISubroutine referenced by inlined instructions).<br>>>> This is supported for ThinLTO, LTO and llvm-link --only-needed, with<br>>>> associated tests for each one.<br>>>><br>>>> Depends on D14838.<br>>>><br>>>> Reviewers: dexonsmith, joker.eph<br>>>><br>>>> Subscribers: davidxl, llvm-commits, joker.eph<br>>>><br>>>> Differential Revision:<span> </span><a href="http://reviews.llvm.org/D14843" rel="noreferrer" target="_blank">http://reviews.llvm.org/D14843</a><br>>>><br>>>> Added:<br>>>>    llvm/trunk/test/Linker/Inputs/only-needed-debug-metadata.ll<br>>>>    llvm/trunk/test/Linker/only-needed-debug-metadata.ll<br>>>> Modified:<br>>>>    llvm/trunk/include/llvm/Transforms/Utils/ValueMapper.h<br>>>>    llvm/trunk/lib/Linker/IRMover.cpp<br>>>>    llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp<br>>>>    llvm/trunk/test/Linker/thinlto_funcimport_debug.ll<br>>>>    llvm/trunk/test/tools/gold/X86/Inputs/linkonce-weak.ll<br>>>>    llvm/trunk/test/tools/gold/X86/linkonce-weak.ll<br>>>><br>>>> Modified: llvm/trunk/lib/Linker/IRMover.cpp<br>>>> URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Linker/IRMover.cpp?rev=256003&r1=256002&r2=256003&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Linker/IRMover.cpp?rev=256003&r1=256002&r2=256003&view=diff</a><br>>>> ==============================================================================<br>>>> --- llvm/trunk/lib/Linker/IRMover.cpp (original)<br>>>> +++ llvm/trunk/lib/Linker/IRMover.cpp Fri Dec 18 11:51:37 2015<br>>>> @@ -487,6 +495,16 @@ class IRLinker {<br>>>><br>>>>   void linkNamedMDNodes();<br>>>><br>>>> +  /// Populate the UnneededSubprograms set with the DISubprogram metadata<br>>>> +  /// from the source module that we don't need to link into the dest module,<br>>>> +  /// because the functions were not imported directly or via an inlined body<br>>>> +  /// in an imported function.<br>>>> +  void findNeededSubprograms(ValueToValueMapTy &ValueMap);<br>>><br>>> Hey Teresa,<br>>><br>>> This has been causing bootstrap assertion failures, I think because a<br>>> DISubprogram referenced by DIImportedEntity should also be considered<br>>> needed.<br>>><br>>> I filed PR26037, could you please have a look?<br>>><br>>> Thanks!<br>>> -Ahmed<br>><br>><br>><br>> --<br>> Teresa Johnson | Software Engineer |<span> </span><a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a><span> </span>| <a href="tel:408-460-2413" value="+14084602413" target="_blank">408-460-2413</a></blockquote></div></div></div></blockquote></div></div></div><br></div></blockquote></div><br></div></div>