[FIX][RUST] use $crate:: in tvm_ffi_dll_export_typed_func! and ensure!#590
Conversation
There was a problem hiding this comment.
Code Review
This pull request improves macro hygiene in rust/tvm-ffi by using $crate for internal references and adds the no_mangle attribute to exported FFI functions to ensure they are preserved in the output binary. A review comment points out that #[unsafe(no_mangle)] is a recent Rust feature (1.82.0+) and suggests using the standard #[no_mangle] to avoid an unnecessary increase in the Minimum Supported Rust Version (MSRV).
| // than a bare `tvm_ffi_sys::…`) lets downstream crates use the | ||
| // macro without having to add `tvm-ffi-sys` to their own | ||
| // `[dependencies]`. | ||
| #[unsafe(no_mangle)] |
There was a problem hiding this comment.
The #[unsafe(no_mangle)] attribute is a relatively new feature introduced in Rust 1.82.0. Using it will cause compilation errors for any users or downstream crates using an older version of the Rust compiler (e.g., 1.81 or earlier). Unless this project has a strict requirement to bump its Minimum Supported Rust Version (MSRV) to 1.82, it is safer to use the standard #[no_mangle] attribute, which is compatible with all Rust versions and remains perfectly valid.
| #[unsafe(no_mangle)] | |
| #[no_mangle] |
Three small bugs in the Rust ergonomics that prevented the macros from being usable from downstream cdylibs: 1. `ensure!` expanded to `crate::bail!`, which resolves to the *caller* crate at expansion site rather than `tvm_ffi`. Switched to `$crate::bail!` so the path resolves correctly in any crate. 2. `tvm_ffi_dll_export_typed_func!` referenced `tvm_ffi_sys::TVMFFIAny` without a `$crate::` prefix, forcing every downstream crate to add `tvm-ffi-sys` to its own `[dependencies]`. Switched to `$crate::tvm_ffi_sys::TVMFFIAny`; downstream now only needs `tvm-ffi`. 3. The generated `pub unsafe extern "C" fn __tvm_ffi_<name>` had no `#[no_mangle]`, so the linker stripped the symbol from cdylibs and `Module::GetFunction` could not find it. Added `#[unsafe(no_mangle)]` (supported in 2021 + 2024 editions on rustc >= 1.82). Verified by building a downstream cdylib that only depends on `tvm-ffi` (no `tvm-ffi-sys` direct dep), loading it via `tvm_ffi.load_module(...)`, and calling exported scalar + Tensor functions from Python.
2395951 to
7232d83
Compare
|
Thanks @gemini-code-assist — good catch. Switched to plain |
|
Thanks for the update, @lucifer1004. That approach makes sense for maintaining compatibility with older toolchains while preparing for future MSRV updates. The changes look correct and address the issues described in the PR summary. |
Summary
Three small bugs in the Rust ergonomics that prevented the macros from
being usable from downstream cdylibs:
ensure!macro expanded tocrate::bail!, which resolves tothe caller crate at expansion site rather than
tvm_ffi. Switchedto
$crate::bail!so the path resolves correctly in any crate.tvm_ffi_dll_export_typed_func!referencedtvm_ffi_sys::TVMFFIAnywithout a$crate::prefix, forcing everydownstream crate to add
tvm-ffi-systo its own[dependencies].Switched to
$crate::tvm_ffi_sys::TVMFFIAny; downstream crates nowonly need to depend on
tvm-ffi.Missing
#[no_mangle]: the generatedpub unsafe extern "C" fn __tvm_ffi_<name>had no#[no_mangle],so the linker stripped the symbol from cdylibs and
Module::GetFunctioncould not find it. Added#[unsafe(no_mangle)](supported in 2021 + 2024 editions onrustc ≥ 1.82).
Reproduction (before)
Building a downstream cdylib that uses
tvm_ffi_dll_export_typed_func!:```text
error[E0433]: cannot find module or crate `tvm_ffi_sys` in this scope
= note: this error originates in the macro `tvm_ffi_dll_export_typed_func`
```
After adding `tvm-ffi-sys` as a direct dep:
```text
error[E0433]: cannot find `bail` in the crate root
= note: this error originates in the macro `tvm_ffi::ensure`
```
After working around `ensure!`, the cdylib builds — but
`nm -D libfoo.so | grep tvm_ffi` returns nothing because
`#[no_mangle]` is missing.
Test plan
`tvm-ffi-sys` direct dep).
(`add_one_inplace`), 2-tensor (`dot_f32`). All round-trip.
Part 1 of 2 — companion PR is #591 (Module::LoadFromBytes).
The second adds `Module::LoadFromBytes`.