| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CA+tqQ4Kyy1re209kjBvtJU037MEcv+jQzEt=E9CMS4d2iuFR5g@mail.gmail.com>
Date: Sat, 27 Dec 2025 19:45:18 +0900
From: Jesung Yang <y.j3ms.n@...il.com>
To: Benno Lossin <lossin@...nel.org>
Cc: Miguel Ojeda <ojeda@...nel.org>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>,
Trevor Gross <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>,
Alexandre Courbot <acourbot@...dia.com>, linux-kernel@...r.kernel.org,
rust-for-linux@...r.kernel.org, nouveau@...ts.freedesktop.org
Subject: Re: [PATCH v4 1/4] rust: macros: add derive macro for `Into`
On Sat, Dec 27, 2025 at 1:57 PM Benno Lossin <lossin@...nel.org> wrote:
> On Fri Dec 26, 2025 at 10:36 AM CET, Jesung Yang wrote:
> > On Fri, Dec 26, 2025 at 2:40 AM Benno Lossin <lossin@...nel.org> wrote:
> >> On Thu Dec 25, 2025 at 9:37 AM CET, Jesung Yang via B4 Relay wrote:
> >> > +fn parse_attrs(target: DeriveTarget, attrs: &[Attribute]) -> syn::Result<(Vec<ValidTy>, Ident)> {
> >> > + let helper = target.get_helper_name();
> >> > +
> >> > + let mut repr_ty = None;
> >> > + let mut helper_tys = Vec::new();
> >> > + for attr in attrs {
> >> > + if attr.path().is_ident("repr") {
> >> > + attr.parse_nested_meta(|meta| {
> >> > + let ident = meta.path.get_ident();
> >> > + if ident.is_some_and(is_valid_primitive) {
> >> > + repr_ty = ident.cloned();
> >> > + }
> >>
> >> While this works for now, writing `repr(C, {integer})` might become
> >> meaningful in the future, see [1]. We should just accept it now as well.
> >>
> >> [1]: https://github.com/rust-lang/rust/issues/68585
> >
> > I think it's worth noting. I'll add a small comment for this.
>
> My suggestion was to just accept `repr(C, {integer})` and make it result
> in the same as `repr({integer})`.
Ah I misread that. Thanks for the clarification.
> >> > + // Delegate `repr` attribute validation to rustc.
> >> > + Ok(())
> >> > + })?;
> >> > + } else if attr.path().is_ident(helper) && helper_tys.is_empty() {
> >> > + let args = attr.parse_args_with(Punctuated::<ValidTy, Token![,]>::parse_terminated)?;
> >> > + helper_tys.extend(args);
> >> > + }
> >> > + }
> >> > +
> >> > + // Note on field-less `repr(C)` enums (quote from [1]):
> >> > + //
> >> > + // In C, enums with discriminants that do not all fit into an `int` or all fit into an
> >> > + // `unsigned int` are a portability hazard: such enums are only permitted since C23, and not
> >> > + // supported e.g. by MSVC.
> >> > + //
> >> > + // Furthermore, Rust interprets the discriminant values of `repr(C)` enums as expressions of
> >> > + // type `isize`. This makes it impossible to implement the C23 behavior of enums where the
> >> > + // enum discriminants have no predefined type and instead the enum uses a type large enough
> >> > + // to hold all discriminants.
> >> > + //
> >> > + // Therefore, `repr(C)` enums in Rust require that either all discriminants to fit into a C
> >> > + // `int` or they all fit into an `unsigned int`.
> >> > + //
> >> > + // As such, `isize` is a reasonable representation for `repr(C)` enums, as it covers the range
> >> > + // of both `int` and `unsigned int`.
> >> > + //
> >> > + // For more information, see:
> >> > + // - https://github.com/rust-lang/rust/issues/124403
> >> > + // - https://github.com/rust-lang/rust/pull/147017
> >> > + // - https://github.com/rust-lang/rust/blob/2ca7bcd03b87b52f7055a59b817443b0ac4a530d/compiler/rustc_lint_defs/src/builtin.rs#L5251-L5263 [1]
> >> > +
> >> > + // Extract the representation passed by `#[repr(...)]` if present. If nothing is
> >> > + // specified, the default is `Rust` representation, which uses `isize` for its
> >> > + // discriminant type.
> >> > + // See: https://doc.rust-lang.org/reference/items/enumerations.html#r-items.enum.discriminant.repr-rust
> >>
> >> I think we should error when no `#[repr({integer})]` attribute is
> >> specified.
> >
> > Not a blocker but just out of curiosity: are you concerned that the
> > default size might change in the future, leading to silent side
> > effects?
>
> `isize` already changes size when you switch between 64 and 32 bit
> architectures. I think the author of an enum they want to convert into
> integers should think about which size the enum should be.
>
> They already can choose `repr(isize)` if that is correct in that case.
> As a default, I would have choosen `i32` (but that conflicts with Rust's
> default, so we can't do it).
Makes sense, that seems better.
> >> > + fn impl_into(
> >> > + enum_ident: &Ident,
> >> > + variants: &[Ident],
> >> > + repr_ty: &syn::Path,
> >> > + input_ty: &ValidTy,
> >> > + ) -> TokenStream {
> >> > + let param = Ident::new("value", Span::call_site());
> >> > +
> >> > + let overflow_assertion = emit_overflow_assert(enum_ident, variants, repr_ty, input_ty);
> >> > + let cast = match input_ty {
> >> > + ValidTy::Bounded(inner) => {
> >> > + let base_ty = inner.emit_qualified_base_ty();
> >> > + let expr = parse_quote! { #param as #base_ty };
> >> > + // Since the discriminant of `#param`, an enum variant, is determined
> >> > + // at compile-time, we can rely on `Bounded::from_expr()`. It requires
> >> > + // the provided expression to be verifiable at compile-time to avoid
> >> > + // triggering a build error.
> >> > + inner.emit_from_expr(&expr)
> >> > + }
> >> > + ValidTy::Primitive(ident) if ident == "bool" => {
> >> > + ::quote::quote! { (#param as #repr_ty) == 1 }
> >> > + }
> >> > + qualified @ ValidTy::Primitive(_) => ::quote::quote! { #param as #qualified },
> >> > + };
> >> > +
> >> > + ::quote::quote! {
> >> > + #[automatically_derived]
> >> > + impl ::core::convert::From<#enum_ident> for #input_ty {
> >> > + fn from(#param: #enum_ident) -> #input_ty {
> >> > + #overflow_assertion
> >> > +
> >> > + #cast
> >> > + }
> >> > + }
> >> > + }
> >> > + }
> >> > +
> >> > + fn emit_overflow_assert(
> >> > + enum_ident: &Ident,
> >> > + variants: &[Ident],
> >> > + repr_ty: &syn::Path,
> >> > + input_ty: &ValidTy,
> >> > + ) -> TokenStream {
> >>
> >> I feel like we should track this via traits rather than using a const
> >> assert. That approach will require & generate much less code.
> >
> > Sorry, but could you elaborate? A small example of what you have in
> > mind would help a lot.
>
> Oh yeah sorry, I had something different in mind compared to what I'll
> describe now, but it achieves the same thing without introducing new
> traits:
>
> We have two options:
> 1) We use `<input_ty as TryFrom<repr_ty>>::try_from` instead of writing
> the `fits` function ourself.
> 2) We require `input_ty: From<repr_ty>`.
>
> The first option would still check every variant and should behave the
> same as your current code.
>
> Option 2 allows us to avoid the const altogether, but requires us to
> choose the smallest integer as the representation (and if we want to be
> able to use both `i8` and `u8`, we can't). I missed this before, so
> using option 1 might be the only way to allow conversions of this kind.
AFAIK, `<input_ty as TryFrom<repr_ty>>::try_from` cannot be called in
const contexts without `#![feature(const_trait_impl, const_convert)]`.
I assume we want to keep this validation at compile-time? If so, we
might need to stick with the custom `fits` check for now. Please let me
know if I misunderstood you.
Best regards,
Jesung
Powered by blists - more mailing lists