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

[David Blaikie via llvm-commits]

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