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

[Richard Smith via llvm-commits]

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