gemini-code-assist[bot] commented on code in PR #660:
URL: https://github.com/apache/tvm-ffi/pull/660#discussion_r3561641313
##########
addons/tvm_ffi_orcjit/src/ffi/orcjit_session.cc:
##########
@@ -71,7 +71,48 @@ struct LLVMInitializer {
static LLVMInitializer llvm_initializer;
-ORCJITExecutionSessionObj::ORCJITExecutionSessionObj(const std::string&
orc_rt_path,
+#ifdef TVM_FFI_ORCJIT_EMBED_ORC_RT
+// liborc_rt.a embedded in .rodata by orc_rt_embed.S.in. C linkage binds to the
+// assembler symbols; the linkage spec must be at namespace scope, not in-fn.
+extern "C" const char orc_rt_archive_start[];
+extern "C" const char orc_rt_archive_end[];
+#endif
+
+namespace {
+#ifdef TVM_FFI_ORCJIT_EMBED_ORC_RT
+// Zero-copy view of the embedded archive; its bytes live for the image
lifetime.
+std::unique_ptr<llvm::MemoryBuffer> GetEmbeddedOrcRuntimeBuffer() {
+ return llvm::MemoryBuffer::getMemBuffer(
+ llvm::StringRef(orc_rt_archive_start,
+ static_cast<size_t>(orc_rt_archive_end -
orc_rt_archive_start)),
+ "liborc_rt.a", /*RequiresNullTerminator=*/false);
+}
+#endif
+
+// Install ExecutorNativePlatform per the `orc_rt` selector (see the ctor doc).
+// Linux/ELF only: the sole caller is under `#ifndef __APPLE__`.
+void SetUpOrcPlatform(llvm::orc::LLJITBuilder& builder,
+ const Optional<Variant<String, Bytes>>& orc_rt) {
+ if (!orc_rt.has_value()) return; // None -> no platform
+ const Variant<String, Bytes>& sel = orc_rt.value();
+ if (auto opt_path = sel.as<String>()) {
+ const String& path = *opt_path;
+ if (path.empty()) { // "auto" -> embedded (or nothing if compiled out)
+#ifdef TVM_FFI_ORCJIT_EMBED_ORC_RT
+
builder.setPlatformSetUp(llvm::orc::ExecutorNativePlatform(GetEmbeddedOrcRuntimeBuffer()));
+#endif
+ } else {
+ builder.setPlatformSetUp(llvm::orc::ExecutorNativePlatform(path.operator
std::string()));
+ }
+ } else { // Bytes: ExecutorNativePlatform takes ownership of the copy.
+ const Bytes& bytes = sel.get<Bytes>();
+
builder.setPlatformSetUp(llvm::orc::ExecutorNativePlatform(llvm::MemoryBuffer::getMemBufferCopy(
+ llvm::StringRef(bytes.data(), bytes.size()), "liborc_rt.a")));
+ }
+}
Review Comment:
The `SetUpOrcPlatform` function is called on Windows because the caller uses
`#ifndef __APPLE__` (which is true on Windows). However, Windows does not
support the ORC platform in this codebase (COFFPlatform is not hooked up, and
`liborc_rt` is unused). If a user passes a custom path or bytes on Windows,
`SetUpOrcPlatform` will attempt to set up `ExecutorNativePlatform`, which
violates the documented behavior that Windows ignores the selector and can
cause runtime errors.
We should make `SetUpOrcPlatform` a no-op on Windows (and macOS) to ensure
the selector is correctly ignored as documented.
```c
void SetUpOrcPlatform(llvm::orc::LLJITBuilder& builder,
const Optional<Variant<String, Bytes>>& orc_rt) {
#if defined(__APPLE__) || defined(_WIN32)
(void)builder;
(void)orc_rt;
#else
if (!orc_rt.has_value()) return; // None -> no platform
const Variant<String, Bytes>& sel = orc_rt.value();
if (auto opt_path = sel.as<String>()) {
const String& path = *opt_path;
if (path.empty()) { // "auto" -> embedded (or nothing if compiled out)
#ifdef TVM_FFI_ORCJIT_EMBED_ORC_RT
builder.setPlatformSetUp(llvm::orc::ExecutorNativePlatform(GetEmbeddedOrcRuntimeBuffer()));
#endif
} else {
builder.setPlatformSetUp(llvm::orc::ExecutorNativePlatform(path.operator
std::string()));
}
} else { // Bytes: ExecutorNativePlatform takes ownership of the copy.
const Bytes& bytes = sel.get<Bytes>();
builder.setPlatformSetUp(llvm::orc::ExecutorNativePlatform(llvm::MemoryBuffer::getMemBufferCopy(
llvm::StringRef(bytes.data(), bytes.size()), "liborc_rt.a")));
}
#endif
}
```
##########
addons/tvm_ffi_orcjit/src/ffi/orcjit_session.cc:
##########
@@ -71,7 +71,48 @@ struct LLVMInitializer {
static LLVMInitializer llvm_initializer;
-ORCJITExecutionSessionObj::ORCJITExecutionSessionObj(const std::string&
orc_rt_path,
+#ifdef TVM_FFI_ORCJIT_EMBED_ORC_RT
+// liborc_rt.a embedded in .rodata by orc_rt_embed.S.in. C linkage binds to the
+// assembler symbols; the linkage spec must be at namespace scope, not in-fn.
+extern "C" const char orc_rt_archive_start[];
+extern "C" const char orc_rt_archive_end[];
+#endif
+
+namespace {
+#ifdef TVM_FFI_ORCJIT_EMBED_ORC_RT
+// Zero-copy view of the embedded archive; its bytes live for the image
lifetime.
+std::unique_ptr<llvm::MemoryBuffer> GetEmbeddedOrcRuntimeBuffer() {
+ return llvm::MemoryBuffer::getMemBuffer(
+ llvm::StringRef(orc_rt_archive_start,
+ static_cast<size_t>(orc_rt_archive_end -
orc_rt_archive_start)),
+ "liborc_rt.a", /*RequiresNullTerminator=*/false);
+}
Review Comment:
Subtracting two unrelated pointers (pointers that do not point to elements
of the same array object) is undefined behavior in C++. Since
`orc_rt_archive_start` and `orc_rt_archive_end` are distinct extern symbols,
subtracting them directly can be optimized unexpectedly by compilers. Casting
them to `std::uintptr_t` before subtraction avoids this undefined behavior.
```suggestion
std::unique_ptr<llvm::MemoryBuffer> GetEmbeddedOrcRuntimeBuffer() {
return llvm::MemoryBuffer::getMemBuffer(
llvm::StringRef(orc_rt_archive_start,
reinterpret_cast<std::uintptr_t>(orc_rt_archive_end) -
reinterpret_cast<std::uintptr_t>(orc_rt_archive_start)),
"liborc_rt.a", /*RequiresNullTerminator=*/false);
}
```
--
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]
