<div dir="ltr">Ah, thanks for the context!<br><br>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.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jul 21, 2020 at 10:39 AM Lang Hames <<a href="mailto:lhames@gmail.com">lhames@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">Hey Dave,<div><br></div><div>Richard and I chatted about this on discord. The eventual fix was <a href="https://github.com/llvm/llvm-project/commit/71292379d757f7a40b1771ade7738e25d7ddece5" target="_blank">https://github.com/llvm/llvm-project/commit/71292379d757f7a40b1771ade7738e25d7ddece5</a> , which makes LazyCallThroughManager's destructor virtual.</div><div><br></div><div>Regards,</div><div>Lang.</div><div><br></div><div><br></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jul 20, 2020 at 3:31 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">ping for Lang/subscribing to this thread for discussion<br>
<br>
On Tue, Jul 14, 2020 at 5:35 PM Richard Smith via llvm-commits<br>
<<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
><br>
> On Tue, 14 Jul 2020 at 11:00, Lang Hames via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>><br>
>> Author: Lang Hames<br>
>> Date: 2020-07-14T10:56:45-07:00<br>
>> New Revision: 3d931e85f1ca8bf72d3bfcd34390cb5ec0d73aa5<br>
>><br>
>> URL: <a href="https://github.com/llvm/llvm-project/commit/3d931e85f1ca8bf72d3bfcd34390cb5ec0d73aa5" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/3d931e85f1ca8bf72d3bfcd34390cb5ec0d73aa5</a><br>
>> DIFF: <a href="https://github.com/llvm/llvm-project/commit/3d931e85f1ca8bf72d3bfcd34390cb5ec0d73aa5.diff" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/3d931e85f1ca8bf72d3bfcd34390cb5ec0d73aa5.diff</a><br>
>><br>
>> LOG: [ORC] Don't take ownership of the trampoline pool in LazyReexportsManager.<br>
>><br>
>> LazyReexportsManager instances use the trampoline pool, but they don't need to<br>
>> own it. Keeping TrampolinePool ownership separate allows re-use of the<br>
>> trampoline pool by other clients.<br>
>><br>
>> Added:<br>
>><br>
>><br>
>> Modified:<br>
>> llvm/include/llvm/ExecutionEngine/Orc/LazyReexports.h<br>
>> llvm/lib/ExecutionEngine/Orc/LazyReexports.cpp<br>
>><br>
>> Removed:<br>
>><br>
>><br>
>><br>
>> ################################################################################<br>
>> diff --git a/llvm/include/llvm/ExecutionEngine/Orc/LazyReexports.h b/llvm/include/llvm/ExecutionEngine/Orc/LazyReexports.h<br>
>> index 7972ed430048..85c1fe7b19a9 100644<br>
>> --- a/llvm/include/llvm/ExecutionEngine/Orc/LazyReexports.h<br>
>> +++ b/llvm/include/llvm/ExecutionEngine/Orc/LazyReexports.h<br>
>> @@ -55,8 +55,7 @@ class LazyCallThroughManager {<br>
>> TrampolinePool::NotifyLandingResolvedFunction;<br>
>><br>
>> LazyCallThroughManager(ExecutionSession &ES,<br>
>> - JITTargetAddress ErrorHandlerAddr,<br>
>> - std::unique_ptr<TrampolinePool> TP);<br>
>> + JITTargetAddress ErrorHandlerAddr, TrampolinePool *TP);<br>
>><br>
>> struct ReexportsEntry {<br>
>> JITDylib *SourceJD;<br>
>> @@ -67,9 +66,7 @@ class LazyCallThroughManager {<br>
>> Expected<ReexportsEntry> findReexport(JITTargetAddress TrampolineAddr);<br>
>> Error notifyResolved(JITTargetAddress TrampolineAddr,<br>
>> JITTargetAddress ResolvedAddr);<br>
>> - void setTrampolinePool(std::unique_ptr<TrampolinePool> TP) {<br>
>> - this->TP = std::move(TP);<br>
>> - }<br>
>> + void setTrampolinePool(TrampolinePool &TP) { this->TP = &TP; }<br>
>><br>
>> private:<br>
>> using ReexportsMap = std::map<JITTargetAddress, ReexportsEntry>;<br>
>> @@ -79,7 +76,7 @@ class LazyCallThroughManager {<br>
>> std::mutex LCTMMutex;<br>
>> ExecutionSession &ES;<br>
>> JITTargetAddress ErrorHandlerAddr;<br>
>> - std::unique_ptr<TrampolinePool> TP;<br>
>> + TrampolinePool *TP = nullptr;<br>
>> ReexportsMap Reexports;<br>
>> NotifiersMap Notifiers;<br>
>> };<br>
>> @@ -105,10 +102,13 @@ class LocalLazyCallThroughManager : public LazyCallThroughManager {<br>
>> if (!TP)<br>
>> return TP.takeError();<br>
>><br>
>> - setTrampolinePool(std::move(*TP));<br>
>> + this->TP = std::move(*TP);<br>
>> + setTrampolinePool(*this->TP);<br>
>> return Error::success();<br>
>> }<br>
>><br>
>> + std::unique_ptr<TrampolinePool> TP;<br>
><br>
><br>
> 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.<br>
><br>
> 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!<br>
><br>
>><br>
>> +<br>
>> public:<br>
>> /// Create a LocalLazyCallThroughManager using the given ABI. See<br>
>> /// createLocalLazyCallThroughManager.<br>
>><br>
>> diff --git a/llvm/lib/ExecutionEngine/Orc/LazyReexports.cpp b/llvm/lib/ExecutionEngine/Orc/LazyReexports.cpp<br>
>> index ff66955082d8..153f6b80784f 100644<br>
>> --- a/llvm/lib/ExecutionEngine/Orc/LazyReexports.cpp<br>
>> +++ b/llvm/lib/ExecutionEngine/Orc/LazyReexports.cpp<br>
>> @@ -17,13 +17,14 @@ namespace llvm {<br>
>> namespace orc {<br>
>><br>
>> LazyCallThroughManager::LazyCallThroughManager(<br>
>> - ExecutionSession &ES, JITTargetAddress ErrorHandlerAddr,<br>
>> - std::unique_ptr<TrampolinePool> TP)<br>
>> - : ES(ES), ErrorHandlerAddr(ErrorHandlerAddr), TP(std::move(TP)) {}<br>
>> + ExecutionSession &ES, JITTargetAddress ErrorHandlerAddr, TrampolinePool *TP)<br>
>> + : ES(ES), ErrorHandlerAddr(ErrorHandlerAddr), TP(TP) {}<br>
>><br>
>> Expected<JITTargetAddress> LazyCallThroughManager::getCallThroughTrampoline(<br>
>> JITDylib &SourceJD, SymbolStringPtr SymbolName,<br>
>> NotifyResolvedFunction NotifyResolved) {<br>
>> + assert(TP && "TrampolinePool not set");<br>
>> +<br>
>> std::lock_guard<std::mutex> Lock(LCTMMutex);<br>
>> auto Trampoline = TP->getTrampoline();<br>
>><br>
>><br>
>><br>
>><br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
>> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>
</blockquote></div>