[llvm] c1baf94 - [ORC] Avoid invalidating iterators in EHFrameRegistrationPlugin.

David Blaikie via llvm-commits llvm-commits at lists.llvm.org
Wed May 5 15:04:20 PDT 2021


On Wed, May 5, 2021 at 11:34 AM Lang Hames <lhames at gmail.com> wrote:
>>
>> > 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.
>> Hrm, that's unfortunate - how'd it get committed without any test coverage?
>
>
> 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".

Ah, OK - though that does mean this function is untested & I'd have
the same question.

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

Any chance of a unit test - isolating the ObjectLinkingLayer and
interacting with it directly? (I had quite hoped the general ORC
layered architecture would make the JIT more amenable to unit testing
in this way)

> 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.
>
> All in all I'm happy leaving this untested for now, and re-evaluating after the ORC runtime code starts to land.
>
> -- Lang.
>
> On Wed, May 5, 2021 at 10:33 AM David Blaikie <dblaikie at gmail.com> wrote:
>>
>> On Wed, May 5, 2021 at 8:36 AM Lang Hames <lhames at gmail.com> wrote:
>> >
>> > 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.
>>
>> Hrm, that's unfortunate - how'd it get committed without any test coverage?
>>
>> > 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.
>>
>> Cool - happy to chat about test strategies any time :)
>>
>> - Dave
>>
>> >
>> > -- Lang.
>> >
>> > On Sun, May 2, 2021 at 12:31 PM David Blaikie <dblaikie at gmail.com> wrote:
>> >>
>> >> Was this covered by existing tests/found by a novel UB failure on a buildbot or the like?
>> >>
>> >> On Sun, Apr 25, 2021 at 6:04 PM Lang Hames via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>> >>>
>> >>>
>> >>> Author: Lang Hames
>> >>> Date: 2021-04-25T16:55:19-07:00
>> >>> New Revision: c1baf946e6cf611ae871e34db5cfea0f94f4b5a0
>> >>>
>> >>> URL: https://github.com/llvm/llvm-project/commit/c1baf946e6cf611ae871e34db5cfea0f94f4b5a0
>> >>> DIFF: https://github.com/llvm/llvm-project/commit/c1baf946e6cf611ae871e34db5cfea0f94f4b5a0.diff
>> >>>
>> >>> LOG: [ORC] Avoid invalidating iterators in EHFrameRegistrationPlugin.
>> >>>
>> >>> In EHFrameRegistrationPlugin::notifyTransferringResources if SrcKey had
>> >>> eh-frames associated but DstKey did not we would create a new entry for DskKey,
>> >>> invalidating the iterator for SrcKey in the process. This commit fixes that by
>> >>> removing SrcKey first in this case.
>> >>>
>> >>> Added:
>> >>>
>> >>>
>> >>> Modified:
>> >>>     llvm/lib/ExecutionEngine/Orc/ObjectLinkingLayer.cpp
>> >>>
>> >>> Removed:
>> >>>
>> >>>
>> >>>
>> >>> ################################################################################
>> >>> diff  --git a/llvm/lib/ExecutionEngine/Orc/ObjectLinkingLayer.cpp b/llvm/lib/ExecutionEngine/Orc/ObjectLinkingLayer.cpp
>> >>> index dc3cf87798cf..ccac4b8c545b 100644
>> >>> --- a/llvm/lib/ExecutionEngine/Orc/ObjectLinkingLayer.cpp
>> >>> +++ b/llvm/lib/ExecutionEngine/Orc/ObjectLinkingLayer.cpp
>> >>> @@ -654,13 +654,23 @@ Error EHFrameRegistrationPlugin::notifyRemovingResources(ResourceKey K) {
>> >>>  void EHFrameRegistrationPlugin::notifyTransferringResources(
>> >>>      ResourceKey DstKey, ResourceKey SrcKey) {
>> >>>    auto SI = EHFrameRanges.find(SrcKey);
>> >>> -  if (SI != EHFrameRanges.end()) {
>> >>> +  if (SI == EHFrameRanges.end())
>> >>> +    return;
>> >>> +
>> >>> +  auto DI = EHFrameRanges.find(DstKey);
>> >>> +  if (DI != EHFrameRanges.end()) {
>> >>>      auto &SrcRanges = SI->second;
>> >>> -    auto &DstRanges = EHFrameRanges[DstKey];
>> >>> +    auto &DstRanges = DI->second;
>> >>>      DstRanges.reserve(DstRanges.size() + SrcRanges.size());
>> >>>      for (auto &SrcRange : SrcRanges)
>> >>>        DstRanges.push_back(std::move(SrcRange));
>> >>>      EHFrameRanges.erase(SI);
>> >>> +  } else {
>> >>> +    // We need to move SrcKey's ranges over without invalidating the SI
>> >>> +    // iterator.
>> >>> +    auto Tmp = std::move(SI->second);
>> >>> +    EHFrameRanges.erase(SI);
>> >>> +    EHFrameRanges[DstKey] = std::move(Tmp);
>> >>>    }
>> >>>  }
>> >>>
>> >>>
>> >>>
>> >>>
>> >>> _______________________________________________
>> >>> llvm-commits mailing list
>> >>> llvm-commits at lists.llvm.org
>> >>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits


More information about the llvm-commits mailing list