CMSE calling conventions

[folkertdev]

Support for the cmse-nonsecure-entry and cmse-nonsecure-call calling conventions on ArmV8m (thumbv8*) targets, and a lint preventing (partially) uninitialized values from crossing the security boundary.

extern "cmse-nonsecure-entry" fn entry(callback: extern "cmse-nonsecure-call" fn(_: u64)) { 
    callback(42);
}

The implementation is tracked in:

• Tracking Issue for the extern "cmse-nonsecure-entry" ABI rust#75835
• Tracking Issue for the extern "cmse-nonsecure-call" ABI rust#81391
• cmse: lint on unions crossing the secure boundary rust#147697

Important

When responding to RFCs, try to use inline review comments (it is possible to leave an inline review comment for the entire file at the top) instead of direct comments for normal comments and keep normal comments for procedural matters like starting FCPs.

This keeps the discussion more organized.

@rustbot label +T-lang

Rendered

[thejpster]

I had one quibble with the architecture terminology, but everything else looks good to me.

[folkertdev]

This has now been reviewed by several people from the rust embedded community and from Arm, so I think this is ready for T-lang to take a look.

@rustbot label +I-lang-nominated

[joshtriplett]

This looks reasonable to me, including the lint.

@rfcbot merge lang

[rust-rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[RalfJung]

Please explain the exact algorithm used for this check. There are potential semver concerns here so it'd be good to be clear about what is being checked here.

For instance, if I have a single-field struct like U64 without the repr(transparent), is that accepted or not?

[folkertdev]

Sure, I'll clarify that.

The current implementation is to look at the BackendRepr, and accept anything passed as Primitive::Int(Integer::I64, _) | Primitive::Float(Float::F64). Because we're working with a subset of AAPCS, I don't think how values are passed can ever change?

https://github.com/rust-lang/rust/blob/b765963267a390d817d0b519b15cfa0d6311d0dc/compiler/rustc_hir_analysis/src/hir_ty_lowering/cmse.rs#L161-L181

We test for the case you mention, it emits an error (clang would emit an error in this case too):

https://github.com/rust-lang/rust/blob/b765963267a390d817d0b519b15cfa0d6311d0dc/tests/ui/cmse-nonsecure/cmse-nonsecure-call/return-via-stack.rs#L13-L27

[RalfJung]

BackendRepr is an internal implementation detail of rustc and prone to changes. It seems risky to get ourselves into a corner where changing unspecified layout computation details is a breaking change.

(We do rely on BackendRepr for the SIMD vector ABI check, but that was a case of fixing a previous oversight which caused soundness trouble, so we had to minimize breakage. I wouldn't take that as a template for a new feature where we have the chance to do it right. Also, we don't rely just on BackendRepr there, we also check the PassMode, so really what we are checking there is the de-facto underlying ABI, at least on the level of detail that is represented in LLVM IR.)

[RalfJung]

We test for the case you mention

That is not the case I mentioned. I suggested to remove repr(transparent), but I didn't suggest adding repr(C). (That will probably trigger an FFI lint, but since we are talking about hard errors here, whether or not there is a lint is largely irrelevant.)

Also, the fact that repr(C) inhibits BackendRepr::Scalar is a limitation we'd like to lift (rust-lang/rust#119183). We should not add new logic that relies on how repr(C) and BackendRepr correlate.

[folkertdev]

So would a check based on the type (peeling off layers of repr(transparent) work better?

[folkertdev]

Presumably, there can be at most 4 arguments.

module zero-sized types, yes.

---

The check is based just on the size of the arguments, each rounded up to the next multiple of 4. If that exceeds 16, the arguments do not fit.

I've updated the text.

[RalfJung]

module zero-sized types, yes.

The fact that zero-sized arguments are skipped in some ABIs is an implementation detail. I would be hesistant to enshrine it in the language.

[diondokter]

ZSTs are used a lot in embedded Rust. It would be very nice to be able to pass ZSTs as an argument without it taking up the register budget.

Often hardware peripherals or certain state conditions are modeled using ZSTs.

For example, say we want a secure memcpy, that could look like:

fn secure_memcpy(dma: DMA, src: *const u8, src_len: usize, target: *mut u8, target_len: usize) -> DMA

If the DMA ZST were counted as one of the 4 possible arguments we couldn't make this work, even when there's not technical reason for it.

[folkertdev]

You could store the arguments into a repr(C) struct...

But that is just a weird workaround. Given that the calling convention inherits from aapcs, and it does ignore zero-sized types, it feels like it should be OK to allow them here?

[RalfJung]

it does ignore zero-sized types

Is that documented in the aapcs ABI description? C does not have zero-sized types, so this would surprise me. So it would be more correct to say that rustc happens to ignore ZST on aapcs calls at the moment, but that's not a deliberate choice (as in, t-lang approved or so) or a stable promise by any means. Generally we should be careful whenever we pass things over an ABI that do not exist in C, since if we make a stable choice here and C later makes a different choice, we are in trouble.

Our general ABI rules do not allow ignoring ZST, and in fact there are ABIs where they currently are not being ignored (specifically on Windows). That might be partially because we make some types ZST that shouldn't be ZST, but last time I checked it looked like we could not get away with a blanket "ZST are ignored" rule. We'd have to start making ABI-specific promises, which is a rabbit hole that we avoided so far.

[RalfJung]

So, a single argument of type (i32, (i8, i16), i64) is allowed?

That's... very strange and not at all what I expected. In the end I don't care that strongly as long as it is clearly specified, but implementation-wise this seems like it could become quite messy.

Is this our ABI to define or is it a standard ABI we have to follow? If it is the latter, allowing repr(Rust) types (this includes tuples) might be a mistake -- it would be the first time we make any kind of ABI guarantees for them.

[folkertdev]

It is really just aapcs, so the 32-bit arm C calling convention. It already lints on passing repr(Rust) types, I'd be fine with actually disallowing them though. This is meant to be an FFI boundary, the user is meant to think about the constraints.

[RalfJung]

The thing is that for normal aapcs, if we change repr(Rust) things won't fail to build. They might "just" stop working properly if you ignored the lints. This RFC turns things into a hard error, which typically has a different par for the lang team. But this feels worth explicitly calling out as an open question to them to get their vibes on it. After all, changing the size of a type can already lead to compilation failures due to repr(transparent).

[folkertdev]

I added this at https://github.com/folkertdev/rust-rfcs/blob/cmse-calling-conventions/text/0000-cmse-calling-conventions.md#a-type-changing-eg-due-to-reprrust-changing-can-cause-a-program-to-stop-compiling

[Jules-Bertholet]

What about lifetime generics? (Also, why forbid RPIT?)

[folkertdev]

Those are not relevant for layout computation, so they are fine to accept.

Is there a better term to only talk about type/const generics?

[Jules-Bertholet]

"Monomorphized generics"?

[Jules-Bertholet]

This seems excessively complicated. Rust does not preserve padding bytes on typed copy. The compiler/ABI is free to erase them across a function call. (A padding byte isn't like e.g. MaybeUninit<u8>—that initialization state is guaranteed to be preserved.) There is no need to lint on passing values with padding, the compiler should just zero it when necessary.

[folkertdev]

There is no need to lint on passing values with padding

For now there is, and the phrasing is such that when the value no longer contains uninitialized bytes we could stop linting in those cases.

the compiler should just zero it when necessary.

By what mechanism? This would be some sort of layout flag on the abi? This feels like it might have broader use, which is neat but also I'd rather not block on it. Lints are deliberately a bit more flexible wrt when changes are considered breaking.

[Jules-Bertholet]

By the mechanism of Just Doing It. Try running this program under Miri:

use std::ptr::{addr_of, addr_of_mut};

#[repr(C)]
struct HasPadding(u8, u16);

fn foo(hp: HasPadding) {
    unsafe { addr_of!(hp).cast::<[u8; 4]>().read() };
}

fn main() {
    let mut hp = HasPadding(0, 0);
    // Initialize padding
    unsafe { addr_of_mut!(hp).cast::<[u8; 4]>().write([0, 0, 0, 0]) };
    foo(hp);
}

It will fail:

error: Undefined Behavior: constructing invalid value at [1]: encountered uninitialized memory, but expected an integer
 --> src/main.rs:7:14
  |
7 |     unsafe { addr_of!(hp).cast::<[u8; 4]>().read() };
  |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here
  |
  = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
  = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
  = note: stack backtrace:
          0: foo
              at src/main.rs:7:14: 7:51
          1: main
              at src/main.rs:14:5: 14:12

We initialized the padding, but it wasn't preserved across the call to foo.

Therefore, zeroing any padding bytes on the machine-code level is perfectly consistent with Rust's semantics for function calls, as these do not promise to preserve the value of padding bytes in arguments. (And it would be impossible to promise it, because certain standard ABIs are incompatible with it.) There is no need for types to opt in.

[folkertdev]

Sure, but there would be a performance cost I'd think?

[RalfJung]

It#s definitely non-trivial. In particular, once enum are involved, determining padding byte becomes runtime-dependent -- you have to generate LLVM IR that recursively walks the value, figures out the active variant of each enum, and zeros padding accordingly. For unions, there's nothing you can do, but I guess those are forbidden here anyway.

[Jules-Bertholet]

It seems Clang does exactly what I suggest (overwrites padding with 0). For example: https://godbolt.org/z/8xEqjM3Kj

[Jules-Bertholet]

Here is the code in Clang: https://github.com/llvm/llvm-project/blob/065a39b9f7f06fca0926394096ee1c1fac41d446/clang/lib/CodeGen/CGCall.cpp#L5755