<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Jan 6, 2016 at 2:03 PM Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> On 2016-Jan-06, at 13:33, Teresa Johnson <<a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a>> wrote:<br>
><br>
> Here is the change I made which fixes the test case. I can add some<br>
> comments and a test case, but let me know if this looks ok otherwise.<br>
> As Eric noted, the DISubroutine for a() is marked as a definition,<br>
> which it isn't actually, but this was the behavior before this patch<br>
> and seems to work ok.<br>
<br>
Since this calling it a definition isn't a regression I agree this<br>
is reasonable, but can you add a FIXME somewhere relevant?<br>
<br>
Besides that (and adding a testcase) I think you should just go<br>
ahead and unbrick the bot.<br>
<br></blockquote><div><br></div><div>Agreed.</div><div><br></div><div>Thanks!</div><div><br></div><div>-eric</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> Thanks,<br>
> Teresa<br>
><br>
> diff --git a/lib/Linker/IRMover.cpp b/lib/Linker/IRMover.cpp<br>
> index 309690f..9900ecc 100644<br>
> --- a/lib/Linker/IRMover.cpp<br>
> +++ b/lib/Linker/IRMover.cpp<br>
> @@ -1211,6 +1211,12 @@ void<br>
> IRLinker::findNeededSubprograms(ValueToValueMapTy &ValueMap) {<br>
> for (unsigned I = 0, E = CompileUnits->getNumOperands(); I != E; ++I) {<br>
> auto *CU = cast<DICompileUnit>(CompileUnits->getOperand(I));<br>
> assert(CU && "Expected valid compile unit");<br>
> + SmallPtrSet<DISubprogram *, 8> ImportedEntitySPs;<br>
> + for (auto *IE : CU->getImportedEntities()) {<br>
> + assert(IE->getEntity());<br>
> + if (auto *SP = dyn_cast<DISubprogram>(IE->getEntity()))<br>
> + ImportedEntitySPs.insert(SP);<br>
> + }<br>
> for (auto *Op : CU->getSubprograms()) {<br>
> // Unless we were doing function importing and deferred metadata linking,<br>
> // any needed SPs should have been mapped as they would be reached<br>
> @@ -1218,7 +1224,7 @@ void<br>
> IRLinker::findNeededSubprograms(ValueToValueMapTy &ValueMap) {<br>
> // function bodies, or from DILocation on inlined instructions).<br>
> assert(!(ValueMap.MD()[Op] && IsMetadataLinkingPostpass) &&<br>
> "DISubprogram shouldn't be mapped yet");<br>
> - if (!ValueMap.MD()[Op])<br>
> + if (!ValueMap.MD()[Op] && !ImportedEntitySPs.count(Op))<br>
> UnneededSubprograms.insert(Op);<br>
> }<br>
> }<br>
><br>
><br>
> On Wed, Jan 6, 2016 at 10:45 AM, Teresa Johnson <<a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a>> wrote:<br>
>> On Wed, Jan 6, 2016 at 10:37 AM, Teresa Johnson <<a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a>> wrote:<br>
>>> Thanks Duncan and Eric.<br>
>>><br>
>>> I should also be able to implement #1 very quickly and get it in today<br>
>>> if we want a workaround while we figure out what is the right approach<br>
>>> here. Will take a stab at that now.<br>
>><br>
>> Oh, missed your point about needing to change the DISubprogram for a()<br>
>> from a definition to a declaration. However, presumably that worked ok<br>
>> before this patch went in, since we would have had the same issue?<br>
>><br>
>>><br>
>>> (Somehow I feel the need to apologize for the overly cute subject - my<br>
>>> intended "question" got turned into "questy", presumably by fat<br>
>>> fingers. =( =) )<br>
>>><br>
>>> Teresa<br>
>>><br>
>>> On Wed, Jan 6, 2016 at 10:27 AM, Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>> wrote:<br>
>>>><br>
>>>><br>
>>>> On Wed, Jan 6, 2016 at 10:04 AM Duncan P. N. Exon Smith<br>
>>>> <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<br>
>>>>><br>
>>>>> +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<br>
>>>>>> than I<br>
>>>>>> +pcc who also found this and contacted me off-list with a possible<br>
>>>>>> solution<br>
>>>>>><br>
>>>>>> See also <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> 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,<br>
>>>>>> 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<br>
>>>>> best.<br>
>>>>><br>
>>>><br>
>>>> It's a bit iffy:<br>
>>>><br>
>>>> 3.2.3 Imported (or Renamed) Declaration Entries Some languages support the<br>
>>>> concept of importing into or making accessible in a given unit declarations<br>
>>>> made in a different module or scope. An imported declaration may sometimes<br>
>>>> be given another name. An imported declaration is represented by one or more<br>
>>>> debugging information entries with the tag DW_TAG_imported_declaration. When<br>
>>>> an overloaded entity is imported, there is one imported declaration entry<br>
>>>> for each overloading.<br>
>>>><br>
>>>> which means that we're probably looking at 1 as we care about declarations<br>
>>>> for entities rather than defining declarations. That said, unless we change<br>
>>>> the metadata to be a declaration of a subprogram rather than a defining<br>
>>>> declaration we're going to have a bad time further down the line.<br>
>>>><br>
>>>>><br>
>>>>> In the meantime, if (3) isn't too intrusive, it would be worth doing<br>
>>>>> temporarily to unbreak the bot...<br>
>>>><br>
>>>><br>
>>>> This is absolutely ok as long as it's temporary while we figure this out.<br>
>>>><br>
>>>> -eric<br>
>>>><br>
>>>>><br>
>>>>><br>
>>>>>><br>
>>>>>> Thanks,<br>
>>>>>> Teresa<br>
>>>>>><br>
>>>>>><br>
>>>>>> On Tue, Jan 5, 2016 at 4:24 PM, Ahmed Bougacha<br>
>>>>>> <<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: <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 <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<br>
>>>>>>>> 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: <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:<br>
>>>>>>>> <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>
>>>>>>>> ==============================================================================<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<br>
>>>>>>>> metadata<br>
>>>>>>>> + /// from the source module that we don't need to link into the dest<br>
>>>>>>>> module,<br>
>>>>>>>> + /// because the functions were not imported directly or via an<br>
>>>>>>>> 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 | <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> | 408-460-2413<br>
>>>>><br>
>>>><br>
>>><br>
>>><br>
>>><br>
>>> --<br>
>>> Teresa Johnson | Software Engineer | <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> | 408-460-2413<br>
>><br>
>><br>
>><br>
>> --<br>
>> Teresa Johnson | Software Engineer | <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> | 408-460-2413<br>
><br>
><br>
><br>
> --<br>
> Teresa Johnson | Software Engineer | <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> | 408-460-2413<br>
<br>
</blockquote></div></div>