<div dir="ltr"><div dir="ltr"><div dir="ltr"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">> Found by an out-of-tree user. Unfortunately this only shows up when we exercise the resource-transfer APIs, which none of our existing regression test infrastructure exercises, so there's no easy way to write a testcase for it yet.<br>Hrm, that's unfortunate - how'd it get committed without any test coverage?</blockquote><div dir="ltr"><br></div><div>Sorry, that statement was more sweeping than intended: We have unit tests for the resource transfer APIs, but it's a listener API and the unit tests use mock listeners. The real listeners (e.g. the eh-frame registrar in this bug) are also heavily tested via regression tests, but only for resource-removal, not for resource-transfer. We're missing coverage for the intersection of "real resource manager" and "resource transfer".</div><div><br></div><div>Making this testable requires solving two problems: (1) how to describe the series of resources to allocate, and transfers to test, and (2) how to introspect the state of the listeners after the transfer.<br></div><div><br></div><div>Right now unit tests seem like the least-worst option, because regression tests would require hand-hacking the introspection code for each real world listener. This might change once the ORC runtime lands, however: Real world resource managers almost always call APIs in the executor to allocate their resources, so we might be able to write testing interposes for those APIs in the ORC runtime and use those to introspect the resource state.</div><div><br></div><div>All in all I'm happy leaving this untested for now, and re-evaluating after the ORC runtime code starts to land.</div><div><br></div><div>-- Lang. </div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, May 5, 2021 at 10:33 AM David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">On Wed, May 5, 2021 at 8:36 AM Lang Hames <<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>> wrote:<br>
><br>
> Found by an out-of-tree user. Unfortunately this only shows up when we exercise the resource-transfer APIs, which none of our existing regression test infrastructure exercises, so there's no easy way to write a testcase for it yet.<br>
<br>
Hrm, that's unfortunate - how'd it get committed without any test coverage?<br>
<br>
> I'm revisiting unit tests for some of the more obscure JIT APIs (see e.g. the recent C API unit test) -- If I'm able to work the kinks out of those then I'll look at writing a unit test for this.<br>
<br>
Cool - happy to chat about test strategies any time :)<br>
<br>
- Dave<br>
<br>
><br>
> -- Lang.<br>
><br>
> On Sun, May 2, 2021 at 12:31 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>> Was this covered by existing tests/found by a novel UB failure on a buildbot or the like?<br>
>><br>
>> On Sun, Apr 25, 2021 at 6:04 PM Lang Hames via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
>>><br>
>>><br>
>>> Author: Lang Hames<br>
>>> Date: 2021-04-25T16:55:19-07:00<br>
>>> New Revision: c1baf946e6cf611ae871e34db5cfea0f94f4b5a0<br>
>>><br>
>>> URL: <a href="https://github.com/llvm/llvm-project/commit/c1baf946e6cf611ae871e34db5cfea0f94f4b5a0" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/c1baf946e6cf611ae871e34db5cfea0f94f4b5a0</a><br>
>>> DIFF: <a href="https://github.com/llvm/llvm-project/commit/c1baf946e6cf611ae871e34db5cfea0f94f4b5a0.diff" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/c1baf946e6cf611ae871e34db5cfea0f94f4b5a0.diff</a><br>
>>><br>
>>> LOG: [ORC] Avoid invalidating iterators in EHFrameRegistrationPlugin.<br>
>>><br>
>>> In EHFrameRegistrationPlugin::notifyTransferringResources if SrcKey had<br>
>>> eh-frames associated but DstKey did not we would create a new entry for DskKey,<br>
>>> invalidating the iterator for SrcKey in the process. This commit fixes that by<br>
>>> removing SrcKey first in this case.<br>
>>><br>
>>> Added:<br>
>>><br>
>>><br>
>>> Modified:<br>
>>> llvm/lib/ExecutionEngine/Orc/ObjectLinkingLayer.cpp<br>
>>><br>
>>> Removed:<br>
>>><br>
>>><br>
>>><br>
>>> ################################################################################<br>
>>> diff --git a/llvm/lib/ExecutionEngine/Orc/ObjectLinkingLayer.cpp b/llvm/lib/ExecutionEngine/Orc/ObjectLinkingLayer.cpp<br>
>>> index dc3cf87798cf..ccac4b8c545b 100644<br>
>>> --- a/llvm/lib/ExecutionEngine/Orc/ObjectLinkingLayer.cpp<br>
>>> +++ b/llvm/lib/ExecutionEngine/Orc/ObjectLinkingLayer.cpp<br>
>>> @@ -654,13 +654,23 @@ Error EHFrameRegistrationPlugin::notifyRemovingResources(ResourceKey K) {<br>
>>> void EHFrameRegistrationPlugin::notifyTransferringResources(<br>
>>> ResourceKey DstKey, ResourceKey SrcKey) {<br>
>>> auto SI = EHFrameRanges.find(SrcKey);<br>
>>> - if (SI != EHFrameRanges.end()) {<br>
>>> + if (SI == EHFrameRanges.end())<br>
>>> + return;<br>
>>> +<br>
>>> + auto DI = EHFrameRanges.find(DstKey);<br>
>>> + if (DI != EHFrameRanges.end()) {<br>
>>> auto &SrcRanges = SI->second;<br>
>>> - auto &DstRanges = EHFrameRanges[DstKey];<br>
>>> + auto &DstRanges = DI->second;<br>
>>> DstRanges.reserve(DstRanges.size() + SrcRanges.size());<br>
>>> for (auto &SrcRange : SrcRanges)<br>
>>> DstRanges.push_back(std::move(SrcRange));<br>
>>> EHFrameRanges.erase(SI);<br>
>>> + } else {<br>
>>> + // We need to move SrcKey's ranges over without invalidating the SI<br>
>>> + // iterator.<br>
>>> + auto Tmp = std::move(SI->second);<br>
>>> + EHFrameRanges.erase(SI);<br>
>>> + EHFrameRanges[DstKey] = std::move(Tmp);<br>
>>> }<br>
>>> }<br>
>>><br>
>>><br>
>>><br>
>>><br>
>>> _______________________________________________<br>
>>> llvm-commits mailing list<br>
>>> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
>>> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div></div>