gemini-code-assist[bot] commented on code in PR #630: URL: https://github.com/apache/tvm-ffi/pull/630#discussion_r3432585417
########## python/tvm_ffi/stub/rust_generator/codegen.py: ########## @@ -0,0 +1,733 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Rust code generation for the ``tvm-ffi-stubgen`` tool. + +Codegen orchestration lives here; low-level rendering helpers live in +``rust_generator.utils``. +""" + +from __future__ import annotations + +import dataclasses +import math +from typing import TYPE_CHECKING + +from tvm_ffi.core import MISSING + +from .. import consts as C +from ..lib_state import object_info_from_type_key +from . import consts as C_RUST +from .utils import ( + RustImports, + UnsupportedTypeError, + _deref_impl, + _packed_args_expr, + _packed_call_lines, + render_rust_type, + schema_contains, +) + +if TYPE_CHECKING: + from pathlib import Path + + from tvm_ffi.core import TypeSchema + + from ..file_utils import CodeBlock + from ..utils import FuncInfo, InitConfig, NamedTypeSchema, ObjectInfo, Options + + +# --- native (FFI-free) construction eligibility ------------------------------ + + +def _rust_string_literal(s: str) -> str: + """Escape ``s`` as a double-quoted Rust string literal.""" + out = ['"'] + for ch in s: + if ch in ('"', "\\"): + out.append("\\" + ch) + elif ch.isprintable(): + out.append(ch) + else: + out.append(f"\\u{{{ord(ch):x}}}") + out.append('"') + return "".join(out) + + +def _default_expr(field: NamedTypeSchema) -> str | None: + """Render ``field``'s registered default as a Rust expression (``None``: can't). + + Only values whose Rust spelling is self-evident are supported: ``bool`` / + ``int`` / finite ``float`` literals (which coerce to the field's possibly + narrowed scalar type in the struct-literal position) and ``str`` (which + becomes a ``tvm_ffi::String``). Anything else -- objects, containers, + non-finite floats, factories -- has no native materialization. + """ + value = field.default + if isinstance(value, bool): + return "true" if value else "false" + if isinstance(value, int): + return repr(value) + if isinstance(value, float): + return repr(value) if math.isfinite(value) else None + if isinstance(value, str): + return f"tvm_ffi::String::from({_rust_string_literal(value)})" + return None + + +def _native_blocker(info: ObjectInfo) -> str | None: + """Why ``info`` cannot be constructed natively; ``None`` when it can. + + The native builder allocates the struct directly, binding every own + field from its setter or a stubgen-rendered default and silently + bypassing any C++ constructor logic -- that is the opted-in behavior, so + native is used whenever possible. There is no FFI fallback: a blocked + type gets no generated constructor at all (the user hand-writes one). + """ + if not info.has_init: + return "the type has no reflected constructor" + for field in info.fields: + if field.origin == "Optional": + # A direct `Optional<T>` field is the view-only layout-mirror + # `tvm_ffi::Optional` (no Rust constructor); created on the C++ side. + return f"field {field.name!r} is an ffi::Optional (view-only, C++-constructed)" + if field.default_is_factory: + return f"field {field.name!r} uses a default factory (FFI-only)" + if field.default is not MISSING and _default_expr(field) is None: + return f"the default value of field {field.name!r} has no Rust rendering" + parent = info.parent_type_key + if parent in (None, "ffi.Object") or _native_eligible(parent): + return None + return f"parent {parent!r} is not natively constructible" + + +def _info_native_eligible(info: ObjectInfo) -> bool: + """Whether ``info`` can be constructed natively (see :func:`_native_blocker`).""" + return _native_blocker(info) is None + + +def _native_eligible(type_key: str) -> bool: + """Type-key wrapper of :func:`_info_native_eligible` (parent recursion). + + A type that cannot be resolved is warned about and treated as non-native. + Deliberately uncached: a cache would go stale across registry changes. + """ + try: + info = object_info_from_type_key(type_key) + except Exception as e: # any failure means "cannot prove native-safe" + print( + f"{C.TERM_YELLOW}[Warning] cannot resolve type {type_key!r} for native " + f"construction ({type(e).__name__}: {e}); treating it as non-native" + f"{C.TERM_RESET}" + ) + return False + return _info_native_eligible(info) + + +def _is_any_compatible(schema: TypeSchema) -> bool: + """Whether ``schema``'s Rust rendering implements ``AnyCompatible``. + + True unless ``Any`` or the bare base ``Object`` appears anywhere -- those are + the only renderable leaves that are not ``AnyCompatible`` (so they cannot be + the ``V`` of a native ``Option<V>`` accessor / marshal). + """ + return not schema_contains(schema, C_RUST.RUST_NOT_ANY_COMPATIBLE_ORIGINS) + + +def _layout_fields(fields: list[NamedTypeSchema]) -> list[NamedTypeSchema]: + """Sort own fields by reflection ``offset`` (C++ memory order). + + Registration order need not match memory order, but the ``#[repr(C)]`` + struct is positional. Fields without an offset (synthetic ``ObjectInfo``s + in tests) keep registration order. + """ + if any(f.offset is None for f in fields): + return list(fields) + return sorted(fields, key=lambda f: f.offset) + + +def _warn_offset_mismatch(type_key: str | None, fields: list[NamedTypeSchema]) -> None: + """Warn when ``#[repr(C)]`` cannot reproduce the recorded field offsets. + + Recomputes each field's ``#[repr(C)]`` placement from the previous field's + end, using the reflected per-field ``alignment``. When alignment is missing + (synthetic ``ObjectInfo``s in tests) it is approximated from ``size`` + (largest power of two, capped at 8), which can false-positive on composite + FFI structs like ``DLDevice``. A mismatch only warns; the binding is still + emitted. Fields without offset/size metadata are skipped and reset the + running position. + """ + prev_end: int | None = None + for field in fields: + if field.offset is None or field.size is None: + prev_end = None + continue + if prev_end is not None: + align = field.alignment or min(8, field.size & -field.size) + placed = (prev_end + align - 1) // align * align Review Comment:  If `field.size` is `0` (or not populated/invalid) and `field.alignment` is also `None` or `0`, `align` will evaluate to `0`. This will cause a `ZeroDivisionError` on the subsequent line when performing the floor division `// align`. Ensuring `align` is at least `1` prevents this potential crash. ```suggestion align = field.alignment or max(1, min(8, field.size & -field.size)) placed = (prev_end + align - 1) // align * align ``` ########## rust/tvm-ffi/src/optional.rs: ########## @@ -0,0 +1,338 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +//! Layout-mirror for C++ [`ffi::Optional<T>`] struct fields. +//! +//! [`Optional<T, A, N>`] occupies the **same size and alignment** as a C++ +//! `ffi::Optional<T>` so a `#[repr(C)]` Rust mirror of a reflected object can +//! embed an optional field at the correct offset. It is **view-only**: it is +//! never constructed, owned, mutated, or dropped by Rust — the object always +//! lives on (and is created/destroyed by) the C++ side, and Rust only ever sees +//! it through a borrow into a live object. +//! +//! All access is delegated to C++ by reusing the reflection field +//! getter/setter (see [`resolve_field`]); reads/writes marshal through a native +//! `Option<V>` at the boundary. This is "method (b)" from the design doc. +//! +//! Layout facts (must come from the C++ reflection registry, never guessed): +//! +//! | C++ `Optional<T>` category | size / align (x86_64, libstdc++) | +//! |-----------------------------------|----------------------------------| +//! | ObjectRef (`Optional<Array>` ...) | 8 / 8 | +//! | `String` / `Bytes` | 16 / 8 | +//! | `std::optional<scalar>` fallback | implementation-defined (e.g. i64 → 16/8, i32 → 8/4, DataType → 6/2) | +//! +//! Because Rust cannot parameterize `#[repr(align(N))]` by a const generic, the +//! alignment is carried by a zero-sized marker type `A` (one of [`Align1`] .. +//! [`Align16`]); the size is the const generic `N`. + +use std::cell::UnsafeCell; +use std::ffi::c_void; +use std::marker::PhantomData; + +use crate::any::Any; +use crate::error::{Error, Result, TYPE_ERROR}; +use crate::type_traits::AnyCompatible; +use tvm_ffi_sys::{TVMFFIAny, TVMFFIFieldGetter, TVMFFIFieldSetter, TVMFFIGetTypeInfo}; + +// --- alignment marker ZSTs (carry alignment; bypass const-generic align limitation) --- +macro_rules! align_marker { + ($name:ident, $n:literal) => { + #[doc = concat!("Zero-sized alignment marker for ", stringify!($n), "-byte alignment.")] + #[repr(align($n))] + #[derive(Clone, Copy, Debug, Default)] + pub struct $name; + }; +} +align_marker!(Align1, 1); +align_marker!(Align2, 2); +align_marker!(Align4, 4); +align_marker!(Align8, 8); +align_marker!(Align16, 16); + +/// Opaque, view-only mirror of a C++ `ffi::Optional<T>` struct field. +/// +/// * `T` — logical value type (for documentation and `Send` propagation only). +/// * `A` — alignment marker (one of [`Align1`] ..= [`Align16`]). +/// * `N` — byte size, equal to `sizeof(ffi::Optional<T>)` from the reflection registry. +/// +/// The storage is private and wrapped in `UnsafeCell`, so: +/// * Rust cannot construct or directly read/write the bytes (view-only); and +/// * a C++ setter invoked through a shared `&self` is sound (interior mutability). +/// +/// Auto traits: always `!Sync` (via `UnsafeCell`); `Send` follows `T`. +/// No `Drop`/`Clone`/`Copy`: destruction and duplication belong to C++. +#[repr(C)] +#[allow(dead_code)] +pub struct Optional<T, A, const N: usize> { + _align: A, + bytes: UnsafeCell<[u8; N]>, + _marker: PhantomData<T>, +} + Review Comment:  Because `Optional` contains an `UnsafeCell`, it is automatically inferred as `!Sync` by the compiler. Consequently, any struct containing an `Optional` field (and any `ObjectArc` wrapping it) will also become `!Sync` and `!Send`. This prevents these objects from being shared or sent across threads, which is a common requirement in multi-threaded TVM/FFI applications. Manually implementing `Send` and `Sync` for `Optional` when `T` is `Send`/`Sync` resolves this issue. ```rust pub struct Optional<T, A, const N: usize> { _align: A, bytes: UnsafeCell<[u8; N]>, _marker: PhantomData<T>, } unsafe impl<T: Send, A: Send, const N: usize> Send for Optional<T, A, N> {} unsafe impl<T: Sync, A: Sync, const N: usize> Sync for Optional<T, A, N> {} ``` -- 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]
