c-variadic: allow c-variadic inherent and trait methods

[folkertdev]

tracking issue: #44930

Continuing the work of #146342, allow inherent and trait methods to be c-variadic. However, a trait that contains a c-variadic method is no longer dyn-compatible.

There is, presumably, some way to make c-variadic methods dyn-compatible. However currently, we don't have confidence that it'll work reliably: when methods from a dyn object are cast to a function pointer, a ReifyShim is created. If that shim is c-variadic, it would need to forward the C variable argument list.

That does appear to work, because the va_list is not represented in MIR at all in this case, so the registers from the call site are untouched by the shim and can be read by the actual implementation. That just does not seem like a solid implementation.

Also, intuitively, why would c-variadic function, primarily needed for FFI, need to be used with dyn objects at all? We can revisit this limitation if a need arises.

r? @workingjubilee

[rustbot]

workingjubilee is currently at their maximum review capacity.
They may take a while to respond.

[workingjubilee]

could we have some tests in relevant files for functions which are neither extern "C" nor Rust functions, to demonstrate how the principles here extend (or don't)?

In particular I'd like them to cover an ABI which in principle doesn't support varargs and an ABI which in principle does.

View changes since this review

[folkertdev]

Sure, I added tests for these calling conventions in various contexts:

unsafe extern "cdecl" fn cdecl_free(_: ...) {}
unsafe extern "cdecl-unwind" fn cdecl_unwind_free(_: ...) {}
unsafe extern "stdcall" fn stdcall_free(_: ...) {}
unsafe extern "stdcall-unwind" fn stdcall_unwind_free(_: ...) {}
unsafe extern "thiscall" fn thiscall_free(_: ...) {}
unsafe extern "thiscall-unwind" fn thiscall_unwind_free(_: ...) {}

@rustbot ready

[workingjubilee]

Shims are also generated when #[track_caller] is involved, right?

[folkertdev]

Correct, but #[track_caller] is only allowed on normal extern "Rust" functions, and ... is disallowed for that calling convention, so these features never interact.

[workingjubilee]

Oh right.

Good.

[workingjubilee]

Will try to get to this on Monday. You will probably see many more questions in the shape of "what about ${thing_invalid_for_other_reasons}?" so my apologies if that is the sort of thing that you find frustrating. When I focus on "problem generation" like that it rarely goes well if I then try to cull possible issues too early.

[workingjubilee]

So we parse ... as a type, but reject its nesting in anything, including even something like type VarArgs = ...;, preventing the type aliasing issues that we've run into with other AST-based checks. This means that while a type alias may be defined for a function signature with ..., it must use an entire function signature.

And we didn't encounter any shim-generation in codegen for any sort of inherent methods or trait methods that aren't dynamic?

I think I'm feeling positive about this but want to sleep on it and review the codegen shims a bit maybe.

[folkertdev]

No, I think only this is relevant

pub enum ReifyReason {
    /// The `ReifyShim` was created to produce a function pointer. This happens when:
    /// * A vtable entry is directly converted to a function call (e.g. creating a fn ptr from a
    ///   method on a `dyn` object).
    /// * A function with `#[track_caller]` is converted to a function pointer
    /// * If KCFI is enabled, creating a function pointer from a method on a dyn-compatible trait.
    /// This includes the case of converting `::call`-like methods on closure-likes to function
    /// pointers.
    FnPtr,
    /// This `ReifyShim` was created to populate a vtable. Currently, this happens when a
    /// `#[track_caller]` mismatch occurs between the implementation of a method and the method.
    /// This includes the case of `::call`-like methods in closure-likes' vtables.
    Vtable,
}

#[track_caller] is only allowed on extern "Rust", and a c-variadic method makes the trait no longer dyn-compatible. I don't see any other variants of InstanceKind that would apply here.

[workingjubilee]

    /// * If KCFI is enabled, creating a function pointer from a method on a dyn-compatible trait.
    /// This includes the case of converting `::call`-like methods on closure-likes to function
    /// pointers.

Thanks for pulling that out, that was one of my big questions. Okay, so that happens when we decay a "closure-like", so a

rust/compiler/rustc_middle/src/ty/util.rs

Line 604 in 5d1b897

pub fn is_closure_like(self, def_id: DefId) -> bool {

which is to say "any kind of closure, even if it's some kind of coroutine or whatever".

I am somewhat worried about that, because I think this might actually be something that "normal" closure decay is at risk of incurring. But closures shouldn't have varargs, of course, since they're extern "Rust", and I suspect if I discussed how varargs fit into KCFI with @maurer and @rcvalle at length, I am guessing they would say something like, "The entire point of varargs is to bypass typechecking, so no, we don't have to worry about that because CFI basically doesn't apply here and it's kind of incompatible without using one of the escape hatches in CFI".

Or maybe I'm wrong? I hope I'm wrong in a direction that still aligns with not caring about these possible interactions, at least.

Either way, since it's an interaction between two unstable things, I am willing to defer having a decisive answer to this until later.

This should enable more code only, so:

@bors r+ rollup

[bors]

📌 Commit 321a76b has been approved by workingjubilee

It is now in the queue for this repository.

[workingjubilee]

Also I found out after a brief chat that apparently KCFI does get used in the case of varargs! It still shouldn't matter for the other reasons, but, you know,

[bors]

⌛ Testing commit 321a76b with merge 20aa0ef...

[bors]

💔 Test failed - checks-actions

[folkertdev]

It looks like the i686-msvc-1 action timed out (after almost 4 hours)? Its logs just sort of stop mid-test, but there is no backtrace or any indication that a test actually failed.

[fmease]

@bors retry