[llvm] 3d931e8 - [ORC] Don't take ownership of the trampoline pool in LazyReexportsManager.

[Lang Hames via llvm-commits]

Hey Dave,

Richard and I chatted about this on discord. The eventual fix was
https://github.com/llvm/llvm-project/commit/71292379d757f7a40b1771ade7738e25d7ddece5
, which makes LazyCallThroughManager's destructor virtual.

Regards,
Lang.



On Mon, Jul 20, 2020 at 3:31 PM David Blaikie <dblaikie at gmail.com> wrote:

> ping for Lang/subscribing to this thread for discussion
>
> On Tue, Jul 14, 2020 at 5:35 PM Richard Smith via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
> >
> > On Tue, 14 Jul 2020 at 11:00, Lang Hames via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
> >>
> >>
> >> Author: Lang Hames
> >> Date: 2020-07-14T10:56:45-07:00
> >> New Revision: 3d931e85f1ca8bf72d3bfcd34390cb5ec0d73aa5
> >>
> >> URL:
> https://github.com/llvm/llvm-project/commit/3d931e85f1ca8bf72d3bfcd34390cb5ec0d73aa5
> >> DIFF:
> https://github.com/llvm/llvm-project/commit/3d931e85f1ca8bf72d3bfcd34390cb5ec0d73aa5.diff
> >>
> >> LOG: [ORC] Don't take ownership of the trampoline pool in
> LazyReexportsManager.
> >>
> >> LazyReexportsManager instances use the trampoline pool, but they don't
> need to
> >> own it. Keeping TrampolinePool ownership separate allows re-use of the
> >> trampoline pool by other clients.
> >>
> >> Added:
> >>
> >>
> >> Modified:
> >>     llvm/include/llvm/ExecutionEngine/Orc/LazyReexports.h
> >>     llvm/lib/ExecutionEngine/Orc/LazyReexports.cpp
> >>
> >> Removed:
> >>
> >>
> >>
> >>
> ################################################################################
> >> diff  --git a/llvm/include/llvm/ExecutionEngine/Orc/LazyReexports.h
> b/llvm/include/llvm/ExecutionEngine/Orc/LazyReexports.h
> >> index 7972ed430048..85c1fe7b19a9 100644
> >> --- a/llvm/include/llvm/ExecutionEngine/Orc/LazyReexports.h
> >> +++ b/llvm/include/llvm/ExecutionEngine/Orc/LazyReexports.h
> >> @@ -55,8 +55,7 @@ class LazyCallThroughManager {
> >>        TrampolinePool::NotifyLandingResolvedFunction;
> >>
> >>    LazyCallThroughManager(ExecutionSession &ES,
> >> -                         JITTargetAddress ErrorHandlerAddr,
> >> -                         std::unique_ptr<TrampolinePool> TP);
> >> +                         JITTargetAddress ErrorHandlerAddr,
> TrampolinePool *TP);
> >>
> >>    struct ReexportsEntry {
> >>      JITDylib *SourceJD;
> >> @@ -67,9 +66,7 @@ class LazyCallThroughManager {
> >>    Expected<ReexportsEntry> findReexport(JITTargetAddress
> TrampolineAddr);
> >>    Error notifyResolved(JITTargetAddress TrampolineAddr,
> >>                         JITTargetAddress ResolvedAddr);
> >> -  void setTrampolinePool(std::unique_ptr<TrampolinePool> TP) {
> >> -    this->TP = std::move(TP);
> >> -  }
> >> +  void setTrampolinePool(TrampolinePool &TP) { this->TP = &TP; }
> >>
> >>  private:
> >>    using ReexportsMap = std::map<JITTargetAddress, ReexportsEntry>;
> >> @@ -79,7 +76,7 @@ class LazyCallThroughManager {
> >>    std::mutex LCTMMutex;
> >>    ExecutionSession &ES;
> >>    JITTargetAddress ErrorHandlerAddr;
> >> -  std::unique_ptr<TrampolinePool> TP;
> >> +  TrampolinePool *TP = nullptr;
> >>    ReexportsMap Reexports;
> >>    NotifiersMap Notifiers;
> >>  };
> >> @@ -105,10 +102,13 @@ class LocalLazyCallThroughManager : public
> LazyCallThroughManager {
> >>      if (!TP)
> >>        return TP.takeError();
> >>
> >> -    setTrampolinePool(std::move(*TP));
> >> +    this->TP = std::move(*TP);
> >> +    setTrampolinePool(*this->TP);
> >>      return Error::success();
> >>    }
> >>
> >> +  std::unique_ptr<TrampolinePool> TP;
> >
> >
> > This code has had undefined behavior for a while by creating
> LocalLazyCallThroughManagers and then deleting them via
> unique_ptr<LazyCallThroughManager>, but after this commit it started
> leaking (by not calling the derived class destructor and thereby not
> deleting the TrampolinePool) and potentially crashing (by passing the wrong
> size to sized operator delete, due to LocalLazyCallThroughMananger now
> being larger than LazyCallThroughManager). This started crashing for us
> because our sized delete happens to check that the passed size is correct
> in debug builds.
> >
> > I've fixed the immediate problems in llvmorg-11-init-20842-g099fd374847
> by changing the unique_ptr<LazyCallThroughManager>s to
> unique_ptr<LocalLazyCallThroughManager>s, but I'm not sure if that was your
> intent or whether the base class should instead have a virtual destructor.
> Could you take a look and double-check? Thanks!
> >
> >>
> >> +
> >>  public:
> >>    /// Create a LocalLazyCallThroughManager using the given ABI. See
> >>    /// createLocalLazyCallThroughManager.
> >>
> >> diff  --git a/llvm/lib/ExecutionEngine/Orc/LazyReexports.cpp
> b/llvm/lib/ExecutionEngine/Orc/LazyReexports.cpp
> >> index ff66955082d8..153f6b80784f 100644
> >> --- a/llvm/lib/ExecutionEngine/Orc/LazyReexports.cpp
> >> +++ b/llvm/lib/ExecutionEngine/Orc/LazyReexports.cpp
> >> @@ -17,13 +17,14 @@ namespace llvm {
> >>  namespace orc {
> >>
> >>  LazyCallThroughManager::LazyCallThroughManager(
> >> -    ExecutionSession &ES, JITTargetAddress ErrorHandlerAddr,
> >> -    std::unique_ptr<TrampolinePool> TP)
> >> -    : ES(ES), ErrorHandlerAddr(ErrorHandlerAddr), TP(std::move(TP)) {}
> >> +    ExecutionSession &ES, JITTargetAddress ErrorHandlerAddr,
> TrampolinePool *TP)
> >> +    : ES(ES), ErrorHandlerAddr(ErrorHandlerAddr), TP(TP) {}
> >>
> >>  Expected<JITTargetAddress>
> LazyCallThroughManager::getCallThroughTrampoline(
> >>      JITDylib &SourceJD, SymbolStringPtr SymbolName,
> >>      NotifyResolvedFunction NotifyResolved) {
> >> +  assert(TP && "TrampolinePool not set");
> >> +
> >>    std::lock_guard<std::mutex> Lock(LCTMMutex);
> >>    auto Trampoline = TP->getTrampoline();
> >>
> >>
> >>
> >>
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at lists.llvm.org
> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >
> > _______________________________________________
> > 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/20200721/168c5a19/attachment.html>