This is an automated email from the ASF dual-hosted git repository. tlopex pushed a commit to branch revert-766-fix/rust-stubgen-object-contracts in repository https://gitbox.apache.org/repos/asf/tvm-ffi.git
commit 494386f8d7c557f59b1cbfc96d82067fc026ac77 Author: Shushi Hong <[email protected]> AuthorDate: Wed Sep 9 16:32:46 2026 -0400 Revert "[Rust][Fix] Preserve object lifetime and thread-safety contracts (#766)" This reverts commit 40c6c744c2aabf0bc46f8fee529f5da6ec68d478. --- examples/rust_stubgen/README.md | 34 +--- python/tvm_ffi/stub/cli.py | 12 +- python/tvm_ffi/stub/generator.py | 9 +- python/tvm_ffi/stub/python_generator/generator.py | 1 - python/tvm_ffi/stub/rust_generator/codegen.py | 14 +- python/tvm_ffi/stub/rust_generator/consts.py | 18 +- python/tvm_ffi/stub/rust_generator/directives.py | 10 +- python/tvm_ffi/stub/rust_generator/generator.py | 8 +- rust/tvm-ffi/src/function.rs | 42 +---- rust/tvm-ffi/src/object.rs | 138 +++++++------- rust/tvm-ffi/tests/test_function.rs | 26 --- rust/tvm-ffi/tests/test_object.rs | 76 -------- tests/python/test_stubgen_rust.py | 214 ---------------------- 13 files changed, 89 insertions(+), 513 deletions(-) diff --git a/examples/rust_stubgen/README.md b/examples/rust_stubgen/README.md index 12670a17..35906b31 100644 --- a/examples/rust_stubgen/README.md +++ b/examples/rust_stubgen/README.md @@ -111,42 +111,12 @@ Besides `prefix` and `custom-new`, this example declares the integer field // tvm-ffi-stubgen(enum): rust_stubgen.IntPair.kind -> PairKind(i32) { Unordered=0, Ordered=1 } ``` -`field` names the Rust type of a field +Three more are available: `field` names the Rust type of a field (`// tvm-ffi-stubgen(field): rust_stubgen.IntPair.a -> MyInt`), `nullable` -wraps an object-reference field in `Option` (for example, a nullable `span`), +wraps it in `Option` (`// tvm-ffi-stubgen(nullable): rust_stubgen.IntPair.a`), and `upcast` adds a conversion to a hand-written typed view (`// tvm-ffi-stubgen(upcast): rust_stubgen.IntPair -> MyView`). -## Construction and ownership - -A complete layout does not always permit direct allocation. For a registry-owned -type, keep its readable fields but suppress both generated allocators: - -```rust -// tvm-ffi-stubgen(no-alloc): <type_key> -``` - -`no-alloc` takes precedence over `custom-new` and also applies to descendants. -`no-alloc`, `nullable`, and `opaque` are shared across all input files in the same -invocation, so include the files declaring these policies when generating -descendants. Directives containing Rust type names remain file-local. The binding -supplies the registry lookup; stubgen does not infer native constructor semantics. - -Layout metadata and pointer sizes describe the loaded libraries and the current -process. Run stubgen against the ABI you intend to build for, not a different -cross-compilation target. - -## Thread safety - -Complete and opaque objects inherit `!Send` and `!Sync` from `tvm_ffi::Object`, -including when viewed through a base or `ObjectRef`. The zero-sized marker does -not change their ABI layout. `Function` remains shareable, so its `from_packed` -and `from_typed` callbacks require `Send + Sync` captures. Scoped structural -callbacks are unaffected. `ObjectArc` requires unique ownership for mutable access. - -These are source-compatibility changes. Any unsafe thread-safety opt-in must -cover hidden native state, destruction, and all accepted dynamic subtypes. - ## Partial generation Every type an object refers to (its parent, its ancestors, the types of its diff --git a/python/tvm_ffi/stub/cli.py b/python/tvm_ffi/stub/cli.py index 83776827..af8f8c24 100644 --- a/python/tvm_ffi/stub/cli.py +++ b/python/tvm_ffi/stub/cli.py @@ -39,7 +39,7 @@ from .lib_state import ( from .utils import FuncInfo, InitConfig, Options if TYPE_CHECKING: - from collections.abc import Container, Sequence + from collections.abc import Container from .generator import Generator @@ -110,12 +110,6 @@ def __main__() -> int: for code in file.code_blocks if code.kind == "object" and isinstance(code.param, str) ) - shared_directives = [ - code - for file in files - for code in file.code_blocks - if code.kind == "directive" and code.param[0] in generator.shared_directive_kinds - ] for file in files: if opt.verbose: print(f"{C.TERM_CYAN}[File] {file.path}{C.TERM_RESET}") @@ -127,7 +121,6 @@ def __main__() -> int: global_funcs, generator=generator, declared=declared, - shared_directives=shared_directives, ) except Exception: failed += 1 @@ -329,14 +322,13 @@ def _stage_3( # noqa: PLR0912 global_funcs: dict[str, list[FuncInfo]], generator: Generator, declared: Container[str] = frozenset(), - shared_directives: Sequence[CodeBlock] = (), ) -> bool: """Process one file's blocks; return whether its content is (or would be) changed.""" defined_funcs: set[str] = set() defined_types: set[str] = set() imports = generator.new_imports() # Stage 1. Hand the one-line directives the pipeline does not consume itself to the generator. - for code in [*shared_directives, *file.code_blocks]: + for code in file.code_blocks: if code.kind != "directive": continue name, payload = code.param diff --git a/python/tvm_ffi/stub/generator.py b/python/tvm_ffi/stub/generator.py index 47182397..c22522a4 100644 --- a/python/tvm_ffi/stub/generator.py +++ b/python/tvm_ffi/stub/generator.py @@ -82,10 +82,6 @@ class Generator(Protocol): #: to the pipeline; any other undeclared name is an error. directive_kinds: frozenset[str] - #: Type policies that also apply to references/descendants in other input files. - #: Unlike local type spellings, these directives must not depend on Rust imports. - shared_directive_kinds: frozenset[str] - def default_ty_map(self) -> dict[str, str]: """Return the default FFI-origin -> target-type name map for this language.""" ... @@ -99,9 +95,8 @@ class Generator(Protocol): def add_directive(self, imports: Any, name: str, payload: str, lineno: int) -> None: """Record a one-line directive (raw payload) into ``imports``. - The collector is per file. The pipeline also seeds it with directives - in :attr:`shared_directive_kinds` from the other input files. - ``name`` is always one of :attr:`directive_kinds`; + The collector is per file, so a directive applies to the blocks of the + file it appears in. ``name`` is always one of :attr:`directive_kinds`; the payload's grammar is the generator's to define. """ ... diff --git a/python/tvm_ffi/stub/python_generator/generator.py b/python/tvm_ffi/stub/python_generator/generator.py index d2f46089..5d784818 100644 --- a/python/tvm_ffi/stub/python_generator/generator.py +++ b/python/tvm_ffi/stub/python_generator/generator.py @@ -46,7 +46,6 @@ class PythonGenerator: syntax = C.PYTHON_SYNTAX source_exts = frozenset({".py", ".pyi"}) directive_kinds: frozenset[str] = frozenset({"import-object"}) - shared_directive_kinds: frozenset[str] = frozenset() def default_ty_map(self) -> dict[str, str]: """Return the default FFI-origin -> Python-type name map.""" diff --git a/python/tvm_ffi/stub/rust_generator/codegen.py b/python/tvm_ffi/stub/rust_generator/codegen.py index 0eca3783..7385dab7 100644 --- a/python/tvm_ffi/stub/rust_generator/codegen.py +++ b/python/tvm_ffi/stub/rust_generator/codegen.py @@ -41,10 +41,6 @@ provided in the same run: ``ffi.*`` by the crate, a ``ty-map`` by a hand-written binding whose object struct is ``<Name>Obj``, anything else by an ``object/`` block in one of the processed files. Otherwise the block is an error naming the missing keys, so a partial binding never references a module that does not exist. - -Layout and allocation are separate: ``no-alloc`` suppresses both allocators -without hiding readable fields. Thread restrictions are inherited from the -crate's ``Object`` base, including for opaque views. """ from __future__ import annotations @@ -263,7 +259,11 @@ class _ObjectRenderer: payload.origin in C_RUST.RUST_ANY_BACKED_OPTIONAL_PAYLOADS or payload.origin == "Optional" ) - expected = C_RUST.RUST_OPTIONAL_FIELD_SIZE if any_backed else C_RUST.RUST_POINTER_SIZE + expected = ( + C_RUST.RUST_OPTIONAL_FIELD_SIZE + if any_backed + else C_RUST.RUST_OBJECT_OPTIONAL_FIELD_SIZE + ) if field.size not in (None, expected): return None if any_backed: @@ -534,9 +534,7 @@ class _ObjectRenderer: ] ) elif verdict.is_complete: - hierarchy = {self.type_key, *self.info.ancestors} - if not hierarchy.intersection(self.imports.directives.no_alloc): - sections += self._allocator_sections(base, has_parent) + sections += self._allocator_sections(base, has_parent) upcasts = self._upcast_lines() if upcasts: sections.append(upcasts) diff --git a/python/tvm_ffi/stub/rust_generator/consts.py b/python/tvm_ffi/stub/rust_generator/consts.py index eb85701d..b6543dbf 100644 --- a/python/tvm_ffi/stub/rust_generator/consts.py +++ b/python/tvm_ffi/stub/rust_generator/consts.py @@ -18,20 +18,9 @@ from __future__ import annotations -import ctypes - #: One-line directives the Rust backend consumes. RUST_DIRECTIVE_KINDS = frozenset( - { - "import-object", - "field", - "nullable", - "enum", - "opaque", - "upcast", - "custom-new", - "no-alloc", - } + {"import-object", "field", "nullable", "enum", "opaque", "upcast", "custom-new"} ) #: Default FFI-origin -> Rust-type map; ``::`` paths get a ``use``, bare names do not. @@ -92,12 +81,13 @@ RUST_SCALAR_WIDTHS = { "f64": 8, } -#: The registry is loaded in this process, so its object pointers have the host ABI size. -RUST_POINTER_SIZE = ctypes.sizeof(ctypes.c_void_p) +#: Size of an object reference field; ``nullable`` may only wrap those. +RUST_POINTER_SIZE = 8 #: C++ ``Optional<T>`` is a 16-byte ``TVMFFIAny`` cell, or a nullable pointer for object payloads. RUST_OPTIONAL_PATH = "tvm_ffi::Optional" RUST_OPTIONAL_FIELD_SIZE = 16 +RUST_OBJECT_OPTIONAL_FIELD_SIZE = 8 #: ``Optional`` payloads kept as the 16-byte cell (a nested ``Optional`` too); the size is checked. RUST_ANY_BACKED_OPTIONAL_PAYLOADS = frozenset( diff --git a/python/tvm_ffi/stub/rust_generator/directives.py b/python/tvm_ffi/stub/rust_generator/directives.py index e7fd262a..185b9065 100644 --- a/python/tvm_ffi/stub/rust_generator/directives.py +++ b/python/tvm_ffi/stub/rust_generator/directives.py @@ -16,7 +16,7 @@ # under the License. """The Rust backend's one-line directives: payload grammar and per-file storage. -Field directives address ``<type_key>.<field>``; type directives address ``<type_key>``:: +Three address one reflected field as ``<type_key>.<field>``, three address a type:: // tvm-ffi-stubgen(field): tirx.Add.a -> PrimExpr // tvm-ffi-stubgen(nullable): ir.Expr.span @@ -24,7 +24,6 @@ Field directives address ``<type_key>.<field>``; type directives address ``<type // tvm-ffi-stubgen(opaque): ir.SourceName // tvm-ffi-stubgen(upcast): tirx.Add -> PrimExpr // tvm-ffi-stubgen(custom-new): tirx.Add - // tvm-ffi-stubgen(no-alloc): ir.SourceName ``field`` sets the field's Rust type (a name in scope, or a ``::`` path to ``use``); on a field inherited from an ancestor it narrows the allocator @@ -33,10 +32,6 @@ integer newtype for it; ``opaque`` keeps a type opaque even when its layout is reproducible. ``upcast`` adds a typed view outside the ancestor chain; ``custom-new`` says the wrapper's ``new`` is hand-written; the generated one is named ``from_complete_fields`` instead. - -``no-alloc`` preserves readable fields but suppresses both allocators, including -in descendants. Like ``nullable`` and ``opaque``, it applies across all files in -a generation run. Directives containing Rust type names remain file-local. """ from __future__ import annotations @@ -70,7 +65,6 @@ class Directives: opaque: set[str] = dataclasses.field(default_factory=set) upcasts: dict[str, list[str]] = dataclasses.field(default_factory=dict) custom_new: set[str] = dataclasses.field(default_factory=set) - no_alloc: set[str] = dataclasses.field(default_factory=set) def add(self, name: str, payload: str, lineno: int) -> None: """Parse and store one directive; raise ``ValueError`` on a malformed payload.""" @@ -89,8 +83,6 @@ class Directives: self.upcasts.setdefault(_type_target(name, lhs, lineno), []).append(rust_type) elif name == "custom-new": self.custom_new.add(_type_target(name, payload, lineno)) - elif name == "no-alloc": - self.no_alloc.add(_type_target(name, payload, lineno)) else: raise ValueError(f"Unknown directive `{name}` at line {lineno}") diff --git a/python/tvm_ffi/stub/rust_generator/generator.py b/python/tvm_ffi/stub/rust_generator/generator.py index 20b72b59..edfc7c07 100644 --- a/python/tvm_ffi/stub/rust_generator/generator.py +++ b/python/tvm_ffi/stub/rust_generator/generator.py @@ -25,7 +25,6 @@ from __future__ import annotations from typing import TYPE_CHECKING from .. import consts as C -from ..lib_state import object_info_from_type_key from . import consts as C_RUST from .codegen import ( finalize_rust_module_tree, @@ -44,13 +43,12 @@ if TYPE_CHECKING: class RustGenerator: - """Generator for complete and opaque Rust bindings (see :mod:`.codegen`).""" + """Generator that emits opaque Rust bindings for reflected objects (see :mod:`.codegen`).""" name = "rust" syntax = C.RUST_SYNTAX source_exts = frozenset({".rs"}) directive_kinds = C_RUST.RUST_DIRECTIVE_KINDS - shared_directive_kinds = frozenset({"no-alloc", "nullable", "opaque"}) def default_ty_map(self) -> dict[str, str]: """Return the default FFI-origin -> Rust-type name map.""" @@ -68,8 +66,6 @@ class RustGenerator: imports.record(payload.split(";", 1)[0].strip()) else: imports.directives.add(name, payload, lineno) - if name == "no-alloc": - object_info_from_type_key(payload.strip()) def canonical_type_name(self, type_key: str) -> str: """Return the Rust path for a defined type key (matches :attr:`RustUse.path`).""" @@ -104,7 +100,7 @@ class RustGenerator: obj_info: ObjectInfo, declared: Container[str] = frozenset(), ) -> None: - """Emit the Rust binding for an ``object/<key>`` block.""" + """Emit the opaque Rust binding for an ``object/<key>`` block.""" generate_rust_object(code, ty_map, imports, opt, obj_info, declared) def generate_import_section_block( diff --git a/rust/tvm-ffi/src/function.rs b/rust/tvm-ffi/src/function.rs index a6bcd5b7..d87e72a9 100644 --- a/rust/tvm-ffi/src/function.rs +++ b/rust/tvm-ffi/src/function.rs @@ -38,18 +38,12 @@ pub struct FunctionObj { cell: TVMFFIFunctionCell, } -/// A shareable packed function. Rust callbacks must have thread-safe captures. +/// Error reference class #[derive(Clone, ObjectRef)] pub struct Function { data: ObjectArc<FunctionObj>, } -// SAFETY: Rust callbacks require Send + Sync, including their captured state. -// Foreign callbacks must uphold the same contract (see from_extern_c). Opt in -// only the handle, not FunctionObj or its potentially stateful derived layouts. -unsafe impl Send for Function {} -unsafe impl Sync for Function {} - //------------------------------------------------------------------------ // CallbackFunctionObjImpl //------------------------------------------------------------------------ @@ -317,18 +311,7 @@ impl Function { Ok(()) } } - /// Construct a function from a packed function. - /// - /// A Function can be called, retained, and dropped on another thread, so its - /// captured state must be `Send + Sync`. This does not require its arguments - /// or result to be `Send`: they are supplied and returned on the calling thread. - /// - /// ```compile_fail - /// use std::rc::Rc; - /// use tvm_ffi::{Any, Function}; - /// let state = Rc::new(1i64); - /// let _ = Function::from_packed(move |_| Ok(Any::from(*state))); - /// ``` + /// Construct a function from a packed function /// # Arguments /// * `func` - The packed function in signature of `Fn(&[AnyView]) -> Result<Any>` /// @@ -336,7 +319,7 @@ impl Function { /// * `Function` - The function pub fn from_packed<F>(func: F) -> Self where - F: Fn(&[AnyView]) -> Result<Any> + Send + Sync + 'static, + F: Fn(&[AnyView]) -> Result<Any> + 'static, { unsafe { let callback_arc = ObjectArc::new(CallbackFunctionObjImpl::from_callback(func)); @@ -347,19 +330,7 @@ impl Function { } } - /// Construct a function from a typed function. - /// - /// Captured state must be `Send + Sync`, as for [`Self::from_packed`]. - /// - /// ```compile_fail - /// use std::cell::Cell; - /// use tvm_ffi::Function; - /// let state = Cell::new(0i64); // Send, but not Sync. - /// let _ = Function::from_typed(move || { - /// state.set(state.get() + 1); - /// Ok(state.get()) - /// }); - /// ``` + /// Construct a function from a typed function /// # Arguments /// * `func` - The typed function with function signature of `F(T0, T1, ...) -> Result<O>` /// @@ -367,7 +338,7 @@ impl Function { /// * `Function` - The function pub fn from_typed<F, I, O>(func: F) -> Self where - F: AsPackedCallable<I, O> + Send + Sync + 'static, + F: AsPackedCallable<I, O> + 'static, { let closure = move |packed_args: &[AnyView]| -> Result<Any> { let ret_value = func.call_packed(packed_args)?; @@ -381,9 +352,6 @@ impl Function { /// `handle` must be a valid pointer (or null) that is compatible with /// `safe_call` and `deleter`. The caller must ensure the handle outlives /// the returned `Function` (or that `deleter` properly frees it). - /// `safe_call` must support concurrent calls from arbitrary threads, and - /// `deleter` must be safe to run on any thread. These requirements also apply - /// to the state behind `handle`. pub unsafe fn from_extern_c( handle: *mut std::ffi::c_void, safe_call: TVMFFISafeCallType, diff --git a/rust/tvm-ffi/src/object.rs b/rust/tvm-ffi/src/object.rs index 2619817a..d690a64b 100644 --- a/rust/tvm-ffi/src/object.rs +++ b/rust/tvm-ffi/src/object.rs @@ -25,24 +25,14 @@ pub use tvm_ffi_sys::TVMFFITypeIndex as TypeIndex; /// Object related ABI handling use tvm_ffi_sys::{TVMFFIAny, TVMFFIGetTypeInfo, TVMFFIObject, COMBINED_REF_COUNT_BOTH_ONE}; -/// The common object header, including for objects with unknown native state. -/// -/// Objects are not `Send` or `Sync` by default: atomic reference counting does -/// not make an object's fields or destructor thread-safe. Keeping this marker -/// in the base also prevents casts to a base node or [`ObjectRef`] from erasing -/// a derived object's thread restrictions. It does not change the C ABI layout. -/// -/// An explicit unsafe `Send`/`Sync` implementation for a binding must account for -/// every dynamic subtype it accepts, including hidden state and destruction. +/// Object type is by default the TVMFFIObject #[repr(C)] pub struct Object { + /// example implementation of the object header: TVMFFIObject, - _thread_confined: std::marker::PhantomData<std::rc::Rc<()>>, } -/// Arc-like wrapper for Object that allows shared ownership. -/// -/// Mutable dereferencing panics if another strong or external weak owner exists. +/// Arc-like wrapper for Object that allows shared ownership /// /// \tparam T The type of the object to be wrapped #[repr(C)] @@ -54,19 +44,6 @@ pub struct ObjectArc<T: ObjectCore> { unsafe impl<T: Send + Sync + ObjectCore> Send for ObjectArc<T> {} unsafe impl<T: Send + Sync + ObjectCore> Sync for ObjectArc<T> {} -// The allocation length must outlive T's destructor when weak owners remain. -// Keep it before the ABI-visible object, never in the live reference-count header. -fn extra_items_layout<T: ObjectCoreWithExtraItems>(count: usize) -> (std::alloc::Layout, usize) { - use std::alloc::Layout; - let items = Layout::array::<T::ExtraItem>(count).expect("extra items layout overflow"); - let (body, _) = Layout::new::<T>() - .extend(items) - .expect("object layout overflow"); - Layout::new::<usize>() - .extend(body) - .expect("allocation layout overflow") -} - /// Traits that can be used to check if a type is an object /// /// This trait is unsafe because it is used to access the object header @@ -363,7 +340,7 @@ pub mod unsafe_ { /// * `obj` - The object to increase the reference count #[inline] pub unsafe fn inc_ref(handle: *mut TVMFFIObject) { - let obj = &*handle; + let obj = &mut *handle; obj.combined_ref_count.fetch_add(1, Ordering::Relaxed); } @@ -375,16 +352,17 @@ pub mod unsafe_ { /// * `obj` - The object to decrease the reference count #[inline] pub(crate) unsafe fn dec_ref(handle: *mut TVMFFIObject) { - // Do not keep a Rust reference to the header across a deleter call. - let old_combined_count = (*handle) + let obj = &mut *handle; + let old_combined_count = obj .combined_ref_count - // Match C++ Object::DecRef: publish this owner's writes before the - // last owner acquires them and runs the destructor. - .fetch_sub(COMBINED_REF_COUNT_STRONG_ONE, Ordering::Release); + .fetch_sub(COMBINED_REF_COUNT_STRONG_ONE, Ordering::Relaxed); if old_combined_count == COMBINED_REF_COUNT_BOTH_ONE { - fence(Ordering::Acquire); - if let Some(deleter) = (*handle).deleter { - deleter(handle.cast(), kTVMFFIObjectDeleterFlagBitMaskBoth as i32); + if let Some(deleter) = obj.deleter { + fence(Ordering::Acquire); + deleter( + obj as *mut TVMFFIObject as *mut c_void, + kTVMFFIObjectDeleterFlagBitMaskBoth as i32, + ); } } else if (old_combined_count & COMBINED_REF_COUNT_MASK_U32) == COMBINED_REF_COUNT_STRONG_ONE @@ -392,16 +370,22 @@ pub mod unsafe_ { // slow path, there is still a weak reference left // need to run two phase decrement fence(Ordering::Acquire); - if let Some(deleter) = (*handle).deleter { - deleter(handle.cast(), kTVMFFIObjectDeleterFlagBitMaskStrong as i32); + if let Some(deleter) = obj.deleter { + deleter( + obj as *mut TVMFFIObject as *mut c_void, + kTVMFFIObjectDeleterFlagBitMaskStrong as i32, + ); } - let old_weak_count = (*handle) + let old_weak_count = obj .combined_ref_count .fetch_sub(COMBINED_REF_COUNT_WEAK_ONE, Ordering::Release); if old_weak_count == COMBINED_REF_COUNT_WEAK_ONE { fence(Ordering::Acquire); - if let Some(deleter) = (*handle).deleter { - deleter(handle.cast(), kTVMFFIObjectDeleterFlagBitMaskWeak as i32); + if let Some(deleter) = obj.deleter { + deleter( + obj as *mut TVMFFIObject as *mut c_void, + kTVMFFIObjectDeleterFlagBitMaskWeak as i32, + ); } } } @@ -409,13 +393,13 @@ pub mod unsafe_ { #[inline] pub(crate) unsafe fn strong_count(handle: *mut TVMFFIObject) -> usize { - let obj = &*handle; + let obj = &mut *handle; (obj.combined_ref_count.load(Ordering::Relaxed) & COMBINED_REF_COUNT_MASK_U32) as usize } #[inline] pub(crate) unsafe fn weak_count(handle: *mut TVMFFIObject) -> usize { - let obj = &*handle; + let obj = &mut *handle; (obj.combined_ref_count.load(Ordering::Relaxed) >> 32) as usize } @@ -440,16 +424,31 @@ pub mod unsafe_ { T: super::ObjectCoreWithExtraItems<ExtraItem = U>, { let obj = ptr as *mut T; - if flags & kTVMFFIObjectDeleterFlagBitMaskStrong as i32 != 0 { + if flags == kTVMFFIObjectDeleterFlagBitMaskBoth as i32 { + let extra_items_count = T::extra_items_count(&(*obj)); std::ptr::drop_in_place(obj); - } - if flags & kTVMFFIObjectDeleterFlagBitMaskWeak as i32 != 0 { - // The object's offset depends only on alignment, not item count. - let (_, offset) = super::extra_items_layout::<T>(0); - let allocation = ptr.cast::<u8>().sub(offset); - let count = allocation.cast::<usize>().read(); - let (layout, _) = super::extra_items_layout::<T>(count); - std::alloc::dealloc(allocation, layout); + let layout = std::alloc::Layout::from_size_align( + std::mem::size_of::<T>() + extra_items_count * std::mem::size_of::<U>(), + std::mem::align_of::<T>(), + ) + .unwrap(); + std::alloc::dealloc(ptr as *mut u8, layout); + } else { + assert_eq!(std::mem::size_of::<T>() % std::mem::size_of::<u64>(), 0); + if flags & kTVMFFIObjectDeleterFlagBitMaskStrong as i32 != 0 { + let extra_items_count = T::extra_items_count(&(*obj)); + std::ptr::drop_in_place(obj); + std::ptr::write(obj as *mut u64, extra_items_count as u64); + } + if flags & kTVMFFIObjectDeleterFlagBitMaskWeak as i32 != 0 { + let extra_items_count = std::ptr::read(obj as *mut u64) as usize; + let layout = std::alloc::Layout::from_size_align( + std::mem::size_of::<T>() + extra_items_count * std::mem::size_of::<U>(), + std::mem::align_of::<T>(), + ) + .unwrap(); + std::alloc::dealloc(ptr as *mut u8, layout); + } } } } @@ -462,7 +461,6 @@ impl Object { pub fn new() -> Self { Self { header: TVMFFIObject::new(), - _thread_confined: std::marker::PhantomData, } } } @@ -521,13 +519,16 @@ impl<T: ObjectCore> ObjectArc<T> { assert_eq!(std::mem::align_of::<T>() % std::mem::align_of::<U>(), 0); assert_eq!(std::mem::size_of::<T>() % std::mem::align_of::<U>(), 0); let extra_items_count = T::extra_items_count(&data); - let (layout, offset) = extra_items_layout::<T>(extra_items_count); + let layout = std::alloc::Layout::from_size_align( + std::mem::size_of::<T>() + extra_items_count * std::mem::size_of::<U>(), + std::mem::align_of::<T>(), + ) + .unwrap(); let raw_data_ptr = std::alloc::alloc(layout); if raw_data_ptr.is_null() { std::alloc::handle_alloc_error(layout); } - raw_data_ptr.cast::<usize>().write(extra_items_count); - let ptr = raw_data_ptr.add(offset).cast::<T>(); + let ptr = raw_data_ptr as *mut T; std::ptr::write(ptr, data); // now override the header directly std::ptr::write( @@ -602,7 +603,7 @@ impl<T: ObjectCore> ObjectArc<T> { /// * `*mut T` - The raw pointer #[inline] pub unsafe fn as_raw_mut(this: &mut Self) -> *mut T { - this.ptr.as_ptr() + this.ptr.as_mut() } /// Get the strong reference count of the ObjectArc @@ -614,7 +615,9 @@ impl<T: ObjectCore> ObjectArc<T> { /// * `usize` - The strong reference count #[inline] pub fn strong_count(this: &Self) -> usize { - unsafe { unsafe_::strong_count(this.ptr.as_ptr().cast::<TVMFFIObject>()) } + unsafe { + unsafe_::strong_count(this.ptr.as_ref() as *const T as *mut T as *mut TVMFFIObject) + } } /// Get the weak reference count of the ObjectArc @@ -626,7 +629,7 @@ impl<T: ObjectCore> ObjectArc<T> { /// * `usize` - The weak reference count #[inline] pub fn weak_count(this: &Self) -> usize { - unsafe { unsafe_::weak_count(this.ptr.as_ptr().cast::<TVMFFIObject>()) } + unsafe { unsafe_::weak_count(this.ptr.as_ref() as *const T as *mut T as *mut TVMFFIObject) } } } @@ -639,29 +642,18 @@ impl<T: ObjectCore> Deref for ObjectArc<T> { } } -// Exclusive Rust access requires both a single strong owner and no external -// weak owners that could acquire another strong reference during the borrow. +// implement DerefMut for ObjectArc impl<T: ObjectCore> DerefMut for ObjectArc<T> { #[inline] fn deref_mut(&mut self) -> &mut Self::Target { - unsafe { - let header = &*self.ptr.as_ptr().cast::<TVMFFIObject>(); - assert_eq!( - header - .combined_ref_count - .load(std::sync::atomic::Ordering::Acquire), - COMBINED_REF_COUNT_BOTH_ONE, - "cannot mutably borrow a shared ObjectArc" - ); - self.ptr.as_mut() - } + unsafe { self.ptr.as_mut() } } } // implement Drop for ObjectArc impl<T: ObjectCore> Drop for ObjectArc<T> { fn drop(&mut self) { - unsafe { unsafe_::dec_ref(self.ptr.as_ptr().cast::<TVMFFIObject>()) } + unsafe { unsafe_::dec_ref(self.ptr.as_mut() as *mut T as *mut TVMFFIObject) } } } @@ -669,7 +661,7 @@ impl<T: ObjectCore> Drop for ObjectArc<T> { impl<T: ObjectCore> Clone for ObjectArc<T> { #[inline] fn clone(&self) -> Self { - unsafe { unsafe_::inc_ref(self.ptr.as_ptr().cast::<TVMFFIObject>()) } + unsafe { unsafe_::inc_ref(self.ptr.as_ref() as *const T as *mut T as *mut TVMFFIObject) } Self { ptr: self.ptr, _phantom: std::marker::PhantomData, diff --git a/rust/tvm-ffi/tests/test_function.rs b/rust/tvm-ffi/tests/test_function.rs index ccf2e9cb..07ed6779 100644 --- a/rust/tvm-ffi/tests/test_function.rs +++ b/rust/tvm-ffi/tests/test_function.rs @@ -53,32 +53,6 @@ fn test_function_from_packed() { assert_eq!(i32::try_from(result).unwrap(), 6); } -#[test] -fn test_function_thread_safe_captures_and_global_cache() { - use std::sync::atomic::{AtomicUsize, Ordering}; - use std::sync::Arc; - - let calls = Arc::new(AtomicUsize::new(0)); - let state = calls.clone(); - let function = Function::from_typed(move |value: i64| { - state.fetch_add(1, Ordering::Relaxed); - // Neither the argument holder nor the returned Any needs to be Send. - cached_global_func!("testing.echo").call_tuple((value,)) - }); - std::thread::scope(|scope| { - for value in 0..4i64 { - let function = &function; - scope.spawn(move || { - let result: i64 = function.call_tuple((value,)).unwrap().try_into().unwrap(); - assert_eq!(result, value); - }); - } - }); - assert_eq!(calls.load(Ordering::Relaxed), 4); - std::thread::spawn(move || drop(function)).join().unwrap(); - assert_eq!(Arc::strong_count(&calls), 1); -} - #[test] fn test_function_from_typed() { let offset = 2; diff --git a/rust/tvm-ffi/tests/test_object.rs b/rust/tvm-ffi/tests/test_object.rs index 54b3ccd1..ebab18e0 100644 --- a/rust/tvm-ffi/tests/test_object.rs +++ b/rust/tvm-ffi/tests/test_object.rs @@ -21,34 +21,6 @@ use std::sync::Arc; use std::{collections::HashMap, hash::Hash}; use tvm_ffi::*; -// An erased owning or borrowed view must not acquire thread-safety merely -// because its concrete object's non-thread-safe fields are no longer visible. -macro_rules! assert_not_impl { - ($ty:ty: $bound:path) => { - const _: fn() = || { - trait AmbiguousIfImpl<A> { - fn check() {} - } - impl<T: ?Sized> AmbiguousIfImpl<()> for T {} - struct ImplementsBound; - impl<T: ?Sized + $bound> AmbiguousIfImpl<ImplementsBound> for T {} - let _ = <$ty as AmbiguousIfImpl<_>>::check; - }; - }; -} - -assert_not_impl!(Object: Send); -assert_not_impl!(Object: Sync); -assert_not_impl!(ObjectArc<Object>: Send); -assert_not_impl!(ObjectArc<Object>: Sync); -assert_not_impl!(tvm_ffi::object::ObjectRef: Send); -assert_not_impl!(tvm_ffi::object::ObjectRef: Sync); -assert_not_impl!(ObjectIdentity: Send); -assert_not_impl!(ObjectIdentity: Sync); -assert_not_impl!(&Object: Send); -assert_not_impl!(Any: Send); -assert_not_impl!(AnyView<'static>: Send); - // must have repr(C) for the object header stays in the same position #[repr(C)] struct TestIntObj { @@ -128,21 +100,6 @@ fn test_object_arc() { assert_eq!(delete_counter.load(Ordering::Relaxed), 1); } -#[test] -fn test_object_arc_mutable_borrow_requires_unique_ownership() { - let deleted = Arc::new(AtomicU32::new(0)); - let mut value = ObjectArc::new(TestIntObj::new(1, deleted, 0)); - let alias = value.clone(); - let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { - value.value = 2; - })); - assert!(result.is_err()); - assert_eq!(alias.value, 1); - drop(alias); - value.value = 3; - assert_eq!(value.value, 3); -} - #[test] fn test_object_arc_with_extra_items() { let delete_counter = Arc::new(AtomicU32::new(0)); @@ -166,39 +123,6 @@ fn test_object_arc_with_extra_items() { assert_eq!(delete_counter.load(Ordering::Relaxed), 1); } -#[test] -fn test_extra_items_with_a_surviving_weak_owner() { - use tvm_ffi::tvm_ffi_sys::{ - TVMFFIObjectDeleterFlagBitMask::kTVMFFIObjectDeleterFlagBitMaskWeak, - COMBINED_REF_COUNT_WEAK_ONE, - }; - for count in [0, 3] { - let deleted = Arc::new(AtomicU32::new(0)); - let value = ObjectArc::new_with_extra_items(TestIntObj::new(1, deleted.clone(), count)); - // Model a native weak reference through the existing combined-count ABI. - unsafe { - let header = ObjectArc::as_raw(&value).cast::<TVMFFIObject>(); - (*header) - .combined_ref_count - .fetch_add(COMBINED_REF_COUNT_WEAK_ONE, Ordering::Relaxed); - drop(value); - assert_eq!(deleted.load(Ordering::Relaxed), 1); - assert_eq!( - (*header) - .combined_ref_count - .fetch_sub(COMBINED_REF_COUNT_WEAK_ONE, Ordering::Release), - COMBINED_REF_COUNT_WEAK_ONE, - ); - std::sync::atomic::fence(Ordering::Acquire); - (*header).deleter.unwrap()( - header.cast_mut().cast(), - kTVMFFIObjectDeleterFlagBitMaskWeak as i32, - ); - } - assert_eq!(deleted.load(Ordering::Relaxed), 1); - } -} - #[test] fn test_object_arc_from_raw() { unsafe { diff --git a/tests/python/test_stubgen_rust.py b/tests/python/test_stubgen_rust.py index 35474ad8..05449b8c 100644 --- a/tests/python/test_stubgen_rust.py +++ b/tests/python/test_stubgen_rust.py @@ -18,12 +18,7 @@ from __future__ import annotations -import ctypes -import os import re -import shutil -import subprocess -import sys from collections.abc import Container, Iterator from pathlib import Path @@ -31,7 +26,6 @@ import pytest import tvm_ffi.stub.cli as stub_cli import tvm_ffi.testing # noqa: F401 (loads the `testing.*` fixture types) from tvm_ffi.core import TypeSchema -from tvm_ffi.libinfo import find_libtvm_ffi from tvm_ffi.stub import consts as C from tvm_ffi.stub.cli import _stage_1, _stage_3 from tvm_ffi.stub.file_utils import CodeBlock, FileInfo @@ -247,7 +241,6 @@ def test_directives_parse() -> None: directives.add("upcast", "tirx.Add -> PrimExpr", 6) directives.add("upcast", "tirx.Add -> crate::typed::TypedExpr", 7) directives.add("custom-new", " tirx.Add ", 8) - directives.add("no-alloc", " ir.SourceName ", 9) assert directives.field_types == {"tirx.Add.a": "PrimExpr"} assert directives.nullable == {"ir.Expr.span"} assert directives.enums == { @@ -257,7 +250,6 @@ def test_directives_parse() -> None: assert directives.opaque == {"ir.SourceName"} assert directives.upcasts == {"tirx.Add": ["PrimExpr", "crate::typed::TypedExpr"]} assert directives.custom_new == {"tirx.Add"} - assert directives.no_alloc == {"ir.SourceName"} @pytest.mark.parametrize( @@ -292,7 +284,6 @@ def test_generator_declares_its_directives_and_records_imports() -> None: "opaque", "upcast", "custom-new", - "no-alloc", } imports = RUST.new_imports() RUST.add_directive(imports, "import-object", "tvm_ffi.libinfo.Foo;False;_Foo", 1) @@ -890,33 +881,6 @@ def test_complete_optional_field_mirrors() -> None: assert "tvm_ffi::Optional" in _uses(imports) [email protected]("pointer_size", [4, 8]) -def test_object_reference_fields_use_native_pointer_size( - monkeypatch: pytest.MonkeyPatch, pointer_size: int -) -> None: - assert RC.RUST_POINTER_SIZE == ctypes.sizeof(ctypes.c_void_p) - monkeypatch.setattr(RC, "RUST_POINTER_SIZE", pointer_size) - info = _info( - "demo.References", - ( - _field("value", "Object", 24, pointer_size), - _field( - "optional", - TypeSchema("Optional", (TypeSchema("Object"),)), - 24 + pointer_size, - pointer_size, - ), - ), - total_size=24 + 2 * pointer_size, - ) - imports = RustImports() - RUST.add_directive(imports, "nullable", "demo.References.value", 1) - text, _ = _render(info, imports) - assert "/// Complete:" in text - assert "pub value: Option<ObjectRef>," in text - assert "pub optional: Option<ObjectRef>," in text - - @pytest.mark.parametrize( "field", [ @@ -957,22 +921,6 @@ def test_custom_new_renames_the_wrapper_allocator() -> None: assert " pub fn new(" not in text -def test_no_alloc_preserves_fields_and_applies_to_descendants() -> None: - base = _info("demo.Base", (_field("value", "Object", 24, 8),), total_size=32) - child = _info("demo.Child", parent="demo.Base", total_size=32) - _register(base) - imports = RustImports() - for name in ("no-alloc", "custom-new"): - imports.directives.add(name, "demo.Base", 1) - for info in (base, child): - text, _ = _render(info, imports) - assert "/// Complete:" in text - assert "fn new(" not in text - assert "from_complete_fields" not in text - text, _ = _render(base, imports) - assert "pub value: ObjectRef," in text - - def test_upcast_directive_adds_typed_views() -> None: """`upcast` targets follow the ancestor chain; a `::` path is imported. Opaque: no allocator.""" info = _info("demo.Leaf", parent="demo.Base") @@ -1173,168 +1121,6 @@ def test_stage_3_applies_directives_to_a_registered_type(tmp_path: Path) -> None assert "use tvm_ffi::Error;" in text -def test_cli_shares_object_policies_across_files( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch -) -> None: - # The child lives in a separate file; it must not bypass the base's allocation policy. - for filename, key, directives in ( - ("base.rs", "testing.TestCxxClassBase", ["no-alloc"]), - ("child.rs", "testing.TestCxxClassDerived", []), - ): - (tmp_path / filename).write_text( - "\n".join( - [f"{C.RUST_SYNTAX.directive(name)} {key}" for name in directives] - + [f"{C.RUST_SYNTAX.begin} object/{key}", C.RUST_SYNTAX.end, ""] - ), - encoding="utf-8", - ) - monkeypatch.setattr("sys.argv", ["tvm-ffi-stubgen", "--target", "rust", str(tmp_path)]) - assert stub_cli.__main__() == 0 - for filename in ("base.rs", "child.rs"): - text = (tmp_path / filename).read_text(encoding="utf-8") - assert "/// Complete:" in text - assert "fn new(" not in text - assert stub_cli.__main__() == 0 - - [email protected]("policy", ["nullable", "opaque"]) -def test_cli_preserves_parent_layout_policies_across_files( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch, policy: str -) -> None: - base = _info("demo.Base", (_field("value", "Object", 24, 8),), total_size=32) - child = _info("demo.Child", parent="demo.Base", total_size=32) - _register(base, child) - monkeypatch.setattr(stub_cli, "object_info_from_type_key", codegen.object_info_from_type_key) - target = "demo.Base.value" if policy == "nullable" else "demo.Base" - for filename, key in (("parent.rs", "demo.Base"), ("child.rs", "demo.Child")): - directive = f"{C.RUST_SYNTAX.directive(policy)} {target}\n" if key == "demo.Base" else "" - (tmp_path / filename).write_text( - f"{directive}{C.RUST_SYNTAX.begin} object/{key}\n{C.RUST_SYNTAX.end}\n", - encoding="utf-8", - ) - # Process the child first: generation must not depend on its parent's file order. - monkeypatch.setattr( - "sys.argv", - [ - "tvm-ffi-stubgen", - "--target", - "rust", - str(tmp_path / "child.rs"), - str(tmp_path / "parent.rs"), - ], - ) - assert stub_cli.__main__() == 0 - for filename in ("parent.rs", "child.rs"): - text = (tmp_path / filename).read_text(encoding="utf-8") - if policy == "nullable": - assert "pub fn new(value: Option<ObjectRef>) -> Self" in text - else: - assert "/// Opaque:" in text - assert "fn new(" not in text - assert stub_cli.__main__() == 0 - - [email protected](shutil.which("cargo") is None, reason="Rust toolchain not installed") -def test_generated_object_contracts_compile_and_drop(tmp_path: Path) -> None: - """Compile the generated code, including negative API checks, against the real crate.""" - imports = RustImports() - RUST.add_directive(imports, "no-alloc", "testing.TestCxxClassBase", 1) - RUST.add_directive(imports, "nullable", "testing.TestDeepCopyEdges.v_obj", 2) - RUST.add_directive(imports, "custom-new", "testing.TestDeepCopyEdges", 3) - keys = ["TestCxxClassBase", "TestCxxClassHiddenField", "TestObjectBase", "TestDeepCopyEdges"] - bodies = [_render(object_info_from_type_key(f"testing.{key}"), imports)[0] for key in keys] - docs = "\n".join( - f"//! ```compile_fail\n//! {snippet}\n//! ```" - for snippet in ( - "use generated_contracts::TestCxxClassBase; let _ = TestCxxClassBase::new(1, 2);", - "fn send<T: Send>() {} send::<generated_contracts::TestCxxClassHiddenField>();", - "fn sync<T: Sync>() {} sync::<generated_contracts::TestCxxClassHiddenField>();", - "fn send<T: Send>() {} send::<generated_contracts::TestObjectBase>();", - "use generated_contracts::TestDeepCopyEdges; let _ = TestDeepCopyEdges::new(1.into(), None);", - ) - ) - source = docs + "\n" + "\n".join(item.as_use_line() for item in imports.items) - source += "\n" + "\n".join(bodies) - source += r""" -// Keep semantic construction in the binding, using the existing nullable allocator. -impl TestDeepCopyEdges { - pub fn new(v_any: tvm_ffi::Any, v_obj: tvm_ffi::object::ObjectRef) -> Self { - Self::from_complete_fields(v_any, Some(v_obj)) - } -} - -#[test] -fn native_null_storage_is_safe_to_drop() { - use tvm_ffi::object::ObjectRefCore; - use tvm_ffi::tvm_ffi_sys::{TVMFFIAny, TVMFFIGetTypeInfo, TVMFFIFieldSetter, TVMFFITestingDummyTarget}; - use tvm_ffi::tvm_ffi_sys::TVMFFIFieldFlagBitMask::kTVMFFIFieldFlagBitSetterIsFunctionObj; - assert_eq!(unsafe { TVMFFITestingDummyTarget() }, 0); - let child = TestObjectBase::new(1, 2.0, "child".into()); - let object = tvm_ffi::object::ObjectRef::try_from(tvm_ffi::Any::from(child.clone())).unwrap(); - let holder = TestDeepCopyEdges::new(7i64.into(), object); - assert!(holder.v_obj.as_ref().unwrap().same_as(&child)); - let native_child = FieldGetter::new(TestDeepCopyEdgesObj::type_index(), "v_obj") - .unwrap().get_any(&*holder).unwrap(); - assert!(tvm_ffi::object::ObjectRef::try_from(native_child).unwrap().same_as(&child)); - // Clear the field using the existing C++ reflection setter, leaving the same - // null storage as a native move-out. No borrowed field reference is live here. - unsafe { - let info = &*TVMFFIGetTypeInfo(TestDeepCopyEdgesObj::type_index()); - let field = (0..info.num_fields as usize).map(|i| &*info.fields.add(i)) - .find(|f| f.name.as_str() == "v_obj").unwrap(); - assert!(!field.setter.is_null()); - assert_eq!(field.flags & (kTVMFFIFieldFlagBitSetterIsFunctionObj as i64), 0); - let setter: TVMFFIFieldSetter = std::mem::transmute(field.setter); - let ptr = ObjectArc::as_raw(TestDeepCopyEdges::data(&holder)); - assert_eq!(setter(ptr.cast_mut().cast::<u8>().offset(field.offset as isize).cast(), - &TVMFFIAny::new()), 0); - } - assert!(holder.v_obj.is_none()); - drop(holder); - assert_eq!(ObjectArc::strong_count(TestObjectBase::data(&child)), 1); -} -""" - (tmp_path / "src").mkdir() - (tmp_path / "src/lib.rs").write_text(source, encoding="utf-8") - crate = Path(__file__).resolve().parents[2] / "rust/tvm-ffi" - (tmp_path / "Cargo.toml").write_text( - '[package]\nname = "generated_contracts"\nversion = "0.0.0"\nedition = "2021"\n' - f'[dependencies]\ntvm-ffi = {{ path = "{crate.as_posix()}" }}\n', - encoding="utf-8", - ) - env = os.environ.copy() - # Do not inherit a workspace target directory: parallel test runs are independent. - env["CARGO_TARGET_DIR"] = str(tmp_path / "target") - loader = ( - "PATH" - if sys.platform == "win32" - else ("DYLD_LIBRARY_PATH" if sys.platform == "darwin" else "LD_LIBRARY_PATH") - ) - env[loader] = str(Path(find_libtvm_ffi()).parent) + os.pathsep + env.get(loader, "") - result = subprocess.run( - ["cargo", "test", "--quiet"], - cwd=tmp_path, - env=env, - capture_output=True, - text=True, - check=False, - timeout=180, - ) - assert result.returncode == 0, result.stdout + result.stderr - - -def test_cli_rejects_unknown_no_alloc_type(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: - src = tmp_path / "mod.rs" - original = ( - f"{C.RUST_SYNTAX.directive('no-alloc')} testing.MissingObjectContract\n" - f"{C.RUST_SYNTAX.begin} object/testing.TestCxxClassBase\n{C.RUST_SYNTAX.end}\n" - ) - src.write_text(original, encoding="utf-8") - monkeypatch.setattr("sys.argv", ["tvm-ffi-stubgen", "--target", "rust", str(tmp_path)]) - assert stub_cli.__main__() == 2 - assert src.read_text(encoding="utf-8") == original - - def test_cli_init_generates_a_module_tree(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setattr( "sys.argv",
