[llvm] 3d931e8 - [ORC] Don't take ownership of the trampoline pool in LazyReexportsManager.
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 21 10:56:06 PDT 2020
Ah, thanks for the context!
Might be handy, at some point, to have a test that exercises using a
different LazyCallThroughManager to demonstrate the purpose/benefit of the
eventual fix compared to this one.
On Tue, Jul 21, 2020 at 10:39 AM Lang Hames <lhames at gmail.com> wrote:
> 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/fe339b47/attachment.html>
More information about the llvm-commits
mailing list