tqchen commented on code in PR #655:
URL: https://github.com/apache/tvm-ffi/pull/655#discussion_r3524189664
##########
addons/tvm_ffi_orcjit/src/ffi/orcjit_dylib.h:
##########
@@ -107,6 +107,9 @@ class ORCJITDynamicLibraryObj : public ModuleObj {
/*! \brief Link order tracking (to support incremental linking) */
llvm::orc::JITDylibSearchOrder link_order_;
+ /*! \brief Whether context slots have been refreshed since the last object
add. */
+ bool context_symbol_refreshed_{false};
+
Review Comment:
Fixed in b7d2f25. The plain shared bool is now one atomic generation/state
word. Concurrent refreshers may repeat the owner-confirmed idempotent pointer
assignments, but only a refresher for the exact current generation can publish
that generation clean; a final acquire check prevents a lookup spanning a state
change from escaping.
##########
addons/tvm_ffi_orcjit/src/ffi/orcjit_dylib.cc:
##########
@@ -113,6 +105,7 @@ void ORCJITDynamicLibraryObj::AddObjectFile(const String&
path) {
// Add object file to this JITDylib
TVM_FFI_ORCJIT_LLVM_CALL(jit_->addObjectFile(*dylib_,
std::move(*buffer_or_err)));
+ context_symbol_refreshed_ = false;
Review Comment:
Fixed in b7d2f25. AddObjectFile atomically claims an add-in-progress bit
before LLJIT can publish the object. GetFunction retries while that bit is set;
a failed add restores the exact prior state, while a successful add advances to
a new pending generation. This prevents both a post-publication visibility
window and a stale refresh from hiding invalidation.
##########
addons/tvm_ffi_orcjit/src/ffi/orcjit_dylib.cc:
##########
@@ -181,6 +180,18 @@ Optional<Function>
ORCJITDynamicLibraryObj::GetFunction(const String& name) {
});
}
+ if (!context_symbol_refreshed_) {
+ if (void** ctx_addr =
reinterpret_cast<void**>(GetSymbol(symbol::tvm_ffi_library_ctx))) {
+ *ctx_addr = this;
+ }
+ Module::VisitContextSymbols([this](const String& name, void* symbol) {
+ if (void** ctx_addr = reinterpret_cast<void**>(GetSymbol(name))) {
+ *ctx_addr = symbol;
+ }
+ });
+ context_symbol_refreshed_ = true;
+ }
Review Comment:
Fixed in b7d2f25 without holding a mutex across GetSymbol. Symbol lookup can
materialize code, execute initializers, and re-enter GetFunction, so the
proposed lock scope can deadlock. The exact-generation CAS and final state
recheck coordinate refresh instead. GCC and Clang suites pass at 139 passed / 3
skipped each, plus 200-round concurrent first-lookup and add/lookup stress runs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
