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

[Lang Hames via llvm-commits]

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

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.

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/97d1e994/attachment.html>