Server stub INCOMING_SIZE larger than necessary?

[jgallagher]

Hi folks - I've been fiddling around with hubris on an stm32f4 discovery board, and was trying to update one of my tasks to use idolatry following the changes made to similar tasks in hubris, and ran into a couple questions: should the generated server's INCOMING_SIZE be the max of the message sizes instead of the sum, and what is the proper way to handle alignment when declaring a buffer to be given to idol_runtime::dispatch?

Specifically, I was updating a task that's almost identical to stm32h7-rcc based on this commit in hubris; this bit in particular:

    // Ensure our buffer is aligned properly for a u32 by declaring it as one.
-   let mut buffer = [0u32; 1];
+   let mut buffer = [0u8; idl::INCOMING_SIZE];

The INCOMING_SIZE in the generated server stub is

pub const INCOMING_SIZE: usize = 0
    + ENABLE_CLOCK_RAW_MSG_SIZE
    + DISABLE_CLOCK_RAW_MSG_SIZE
    + ENTER_RESET_RAW_MSG_SIZE
    + LEAVE_RESET_RAW_MSG_SIZE
    ;

which makes buffer four times larger than it was before (which I believe is unnecessary based on https://github.com/oxidecomputer/idolatry/blob/ec1047c/runtime/src/lib.rs#L154-L161). The comment is also out of date in terms of alignment, but I'm not sure what the right fix is; in my task I tried keeping buffer declared as [0u32; 1] and adding an assertion that its size matched idl::INCOMING_SIZE, which is how I bumped into this question.

[jgallagher]

I fiddled with this a bit more and confirmed the currently-generated server stubs can end up producing unaligned reads, but maybe that's fine? I was experimenting with a u32, and ldr.w doesn't require the source to be aligned, at least.

In terms of the size, I think that point is still relevant. I have a small patch locally that changes the incoming size in the example above to calculate the max via a const fn:

const fn max_incoming_size() -> usize {
    let mut max = 0;
    if max < ENABLE_CLOCK_RAW_MSG_SIZE {
        max = ENABLE_CLOCK_RAW_MSG_SIZE;
    }
    if max < DISABLE_CLOCK_RAW_MSG_SIZE {
        max = DISABLE_CLOCK_RAW_MSG_SIZE;
    }
    if max < ENTER_RESET_RAW_MSG_SIZE {
        max = ENTER_RESET_RAW_MSG_SIZE;
    }
    if max < LEAVE_RESET_RAW_MSG_SIZE {
        max = LEAVE_RESET_RAW_MSG_SIZE;
    }
    max
}
pub const INCOMING_SIZE: usize = max_incoming_size();

Not the nicest max implementation, but for isn't allowed in const fn (yet?). Would be happy to PR this if there's interest, assuming I haven't misunderstood something and the sum of sizes is actually necessary.

[cbiffle]

Ha, nice catch. Yes, this was a braino on my part when writing the server code generation. I'd happily take your patch, that's roughly what I would have written.

confirmed the currently-generated server stubs can end up producing unaligned reads

Hmmm it really shouldn't be... or, that is, it should be generating careful unaligned reads, because of the use of packed. What is it you're seeing?

[jgallagher]

I think "careful unaligned reads" might be what I'm seeing. I created a dummy task with this idol:

Interface(
    name: "Dummy",
    ops: {
        "dummy1": (
            args: {
                "x": "u8",
                "y": "u32",
            },
            reply: Result(
                ok: "()",
                err: CLike("DummyError"),
            ),
            idempotent: true,
        ),
    },
)

and then poked at the internal implementation in the server stub like this:

        let buf = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10];
        for i in 0..4 {
            // force awkward alignment
            let buf = &buf[i..i+server_stub::DUMMY1_MSG_SIZE];

            // convert to a repr(packed) &Dummy1Args
            let args = server_stub::read_dummy1_msg(buf).unwrap();

            // read x and y; y will be read unaligned 3/4 times
            let x = args.x;
            let y = args.y;

            sys_log!("x = {:#x}, y = {:#x}", x, y);
        }

which compiles to this (comments are my notes, which I believe are correct based on gdb register contents):

; r6 = original buf
; r5 = i
=> 0x8020286 <dummy_srv::main+142>:     ldrb    r0, [r6, r5]   ; load args.x
   0x8020288 <dummy_srv::main+144>:     strb.w  r0, [r7, #-49] ; store args.x
   0x802028c <dummy_srv::main+148>:     adds    r0, r6, r5     ; compute address of args.y
   0x802028e <dummy_srv::main+150>:     str     r4, [sp, #32]  ; unrelated?
   0x8020290 <dummy_srv::main+152>:     ldr.w   r0, [r0, #1]   ; load args.y; this is an unaligned read if r0 % 4 != 3

One thing that concerned me a bit was https://rust-lang.github.io/rfcs/1240-repr-packed-unsafe-ref.html. If anyone were to take a reference to the fields of an args struct returned by one of the stub reader functions, that could introduce undefined behavior (since the compiler assumes &T matches the alignment of T). But that isn't happening; the server stub doesn't expose the args structs directly to callers. Even if someone were to go poking about like this and added a ref, the compiler emits a suitably-ominous warning:

warning: reference to packed field is unaligned
  --> task/dummy-srv/src/main.rs:40:21
   |
40 |             let y = &args.y;
   |                     ^^^^^^^
   |
   = note: `#[warn(unaligned_references)]` on by default
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!

Compiler codegen is outside my area of expertise, but based on reading that RFC I believe (?) that reading a packed structs fields directly is fine, and the compiler should know that it needs to emit instructions suitable for unaligned loads - it's only taking references that's problematic.

[cbiffle]

...overzealous Github. Reopening this to discuss the alignment behavior.

[cbiffle]

One thing that concerned me a bit was https://rust-lang.github.io/rfcs/1240-repr-packed-unsafe-ref.html. If anyone were to take a reference to the fields of an args struct returned by one of the stub reader functions, that could introduce undefined behavior (since the compiler assumes &T matches the alignment of T).

That's right! This is why the args structs are nested inside the functions, to make them difficult to access and use incorrectly.

0x8020290 <dummy_srv::main+152>: ldr.w r0, [r0, #1] ; load args.y; this is an unaligned read if r0 % 4 != 3

I was initially surprised by this, but, reading the LLVM ARM backend, it appears that LLVM assumes that Cortex-M targets (other than M0) support misaligned loads/stores. This is kinda true -- they tend to support them by default though they can be disabled at runtime. Presumably if you did that, you'd need to add a target option (+strict_align seems like a candidate). We currently have no intention of enabling the misalignment trap.

So, I think this is correct. (And I'm pleased, because loading a misaligned u32 using ldr will cost approximately 1 extra cycle on our target parts, whereas doing it with a sequence of loads and combining the results is much more costly.)

[jgallagher]

LLVM assumes that Cortex-M targets (other than M0) support misaligned loads/stores

This is exactly what I didn't know when I opened the issue! :) Agreed everything looks fine.