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

Wed May 5 17:55:51 PDT 2021

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

Test coverage in ORC often lags behind API development where the "right"
testing approach is unclear: The ResourceTracker API was experimental when
it landed -- everything that was obviously unit testable or regression
testable received a test, things that weren't obvious went on the todo list.

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)

ORC is definitely more unit testable than MCJIT, but some components like
this still require non-trivial environments to test. I'd prefer to hold off
until it becomes clearer whether this is best handled by a unit test or a
regression test with extra runtime support.

FWIW this particular case is incidentally exercised by the new OrcCAPITest
unit test, but it should still ideally have its own test, and the point
remains for other untested / undertested JIT components.

-- Lang.

On Wed, May 5, 2021 at 3:04 PM David Blaikie <dblaikie at gmail.com> wrote:

> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210505/1a86bdec/attachment.html>