derive Clone and Copy for InterruptStackFrame

[Wasabi375]

InterruptStackFrame is a wrapper around InterruptStackFrameValue which already implements Clone and Copy.
This just extends this to the wrapper type.

[Freax13]

Thank you for your contribution!

I'm not sure if we want to implement these traits because I think the lack of implementations for those is intentional. The idea behind InterruptStackFrame is to prevent users from acquiring a mutable reference to the InterruptStackFrameValue without unsafe code. Implementing Copy (or Clone) would make it very easy to update the contained value.

Admittedly, InterruptStackFrame currently doesn't do this job perfectly. It's still possible to use methods like core::mem::swap to update the contained value, but that's a bit harder to do. Nowadays we could use Pin for this, but that didn't exist at the time this was added.

What's your use-case for implementing Clone and Copy?

[Wasabi375]

I'm trying to implement a task system. Currently I construct an InterruptStackFrame using assembly on the stack before calling into rust code, which means I have a reference to the frame. I want to move the stack frame to the heap so that I can later jump back to it, but since I only have a reference I need to be able to copy it.

Implementing Copy (or Clone) would make it very easy to update the contained value.

I don't see how that would be possible. The inner value is still private so it can't be changed. Also it is currently not possible to construct the wrapper from the frame value.

I could see an argument for only implementing Clone and not Copy as that would make it explicit that the stack-frame is no longer just staying on the stack.

[Wasabi375]

Not sure this is a good argument. It is a bit philosophical but I think it still counts.

If you use the "x86-interrupt" abi with InterruptStackFrame it really should be Copy on a conceptual level.

Take something like this

extern "x86-interrupt" fn inerrupt_handler(isf: InterruptStackFrame) {
    foo(isf)
}
fn foo(isf: InterruptStackFrame) {
}

Here we move isf out of interrupt_handler into foo. However interrupt_handler implicitly still uses isf at the end to perform the iretq instruction. Sure, that access is outside of rust so it does not really break the idea that the value no longer exists after a move.

[Freax13]

I don't see how that would be possible. The inner value is still private so it can't be changed. Also it is currently not possible to construct the wrapper from the frame value.

Yeah, you're right, my point didn't make much sense.

Here we move isf out of interrupt_handler into foo. However interrupt_handler implicitly still uses isf at the end to perform the iretq instruction. Sure, that access is outside of rust so it does not really break the idea that the value no longer exists after a move.

Yeah, the semantics of the parameters for the x86-interrupt ABI are a bit weird. It looks like the frame is passed by value, but it's actually passed by reference under the hood. When other functions take a parameter by value, they actually get a fresh copy of the data. This makes things weird when the by-val-but-actually-by-ref value from a x86-interrupt function is passed to a regular function that takes a by-val-and-is-actually-by-val parameter.

I'm trying to implement a task system. Currently I construct an InterruptStackFrame using assembly on the stack before calling into rust code, which means I have a reference to the frame. I want to move the stack frame to the heap so that I can later jump back to it, but since I only have a reference I need to be able to copy it.

Are you using a x86-interrupt function for this? If not I'd hightly recommend using InterruptStackFrameValue instead. InterruptStackFrame is really just meant for x86-interrupt functions and all other functions that need to process stack frame values should use InterruptStackFrameValue or &mut InterruptStackFrame.

[Wasabi375]

Yeah, the semantics of the parameters for the x86-interrupt ABI are a bit weird

From what I gather the way the abi works changed in llvm as well and the more I look at it the less I understand it.
But I think the discussion about calling convention and ABI is maybe not the most useful here anyways.

Are you using a x86-interrupt function for this?

Not really? Kinda? I am using a naked function written in assembly instead so I get access to the xsave instruction which I need to later restore floating point and simd registers.

#[unsafe(naked)]
pub extern "x86-interrupt" fn context_switch_handler(_int_stack_frame: InterruptStackFrame) {
    #[inline]
    extern "C" fn rust_handler(
        stack_frame: &InterruptStackFrame,
        registers: &InterruptRegisterState,
        xsave_area: *mut u8,
    ) -> ! { ... ) }

    naked_asm!(
          ...
          "lea rdi, [rsp + {stack_frame_offset}]", // load stack-frame addr into rdi (first arg)
          "lea rsi, [rsp + {registers_offset}]", // load register-state addr into rsi (second arg)
          "mov rdx, r12",         // move xsave area into rdx (third arg).
          "call {rust_handler}",
    )
}

In the assembbly I construct a reference to the InterruptStackFrame and pass that as an argument to rust (same as registers and xsave area).

If not I'd hightly recommend using InterruptStackFrameValue instead. InterruptStackFrame is really just meant for x86-interrupt functions and all other functions that need to process stack frame values should use InterruptStackFrameValue or &mut InterruptStackFrame

I don't agree. &mut InterruptStackFrame does nothing as InteruptStackFrame has no functions that allow for modification so I don't understand why I would use that.
The real question is whether I should use InterruptStackFrame or InterruptStackFrameValue.
Correct me if I'm wrong but here is how I look at those 2 types.

InterruptStackFrameValue is for when you want to modify or create a stack frame before calling iretq. This is useful for jumping to anywhere in code.

InterruptStackFrame is a wrapper that ensures that there are no modifications to the frame value. This is used when you get the ISF from an interrupt. x86-interrupt functions give you access to that directly or in my case I construct a reference to it in assembly but in either case I have a valid ISF and I don't want it modified.

[Freax13]

I don't agree. &mut InterruptStackFrame does nothing as InteruptStackFrame has no functions that allow for modification so I don't understand why I would use that.
[...]
InterruptStackFrame is a wrapper that ensures that there are no modifications to the frame value. This is used when you get the ISF from an interrupt. x86-interrupt functions give you access to that directly or in my case I construct a reference to it in assembly but in either case I have a valid ISF and I don't want it modified.

InterruptStackFrame doesn't guarantee that there are no modifications to the value. It's possible to modify the frame value like so:

fn foo(frame: &mut InterruptStackFrame) {
    *frame = InterruptStackFrame::new(todo!(), todo!(), todo!(), todo!(), todo!());
}

InterruptStackFrame only guarantees that reassignment like this doesn't work with the x86-interrupt calling convention semantics.

The way I currently see it is that InterruptStackFrame should only be used for x86-interrupt functions and InterruptStackFrameValue should be used in all other cases.

For your use-case, could use have rust_handler take a &InterruptStackFrameValue instead of a &InterruptStackFrame?

[Wasabi375]

InterruptStackFrame only guarantees that reassignment like this doesn't work with the x86-interrupt calling convention semantics.

Maybe I misunderstood the way InterruptStackFrame is supposed to work. I don't fully understand the x86-interrupt ABI and how it interacts with normal rust features. It does not really help that I can't find any good resources about it.
I don't really understand why your above example doesn't work with the interrupt ABI but I am willing to take your word for it.

For your use-case, could use have rust_handler take a &InterruptStackFrameValue instead of a &InterruptStackFrame?

Yeah. I can change my code to use InterruptStackFrameValue instead. After all they are identical on the assembly level. I will do that and I think we can close this PR.

That said I think there is room for improvement of the InterruptStackFrame API. From what I understand now the main use is to ensure that modifications are done through as_mut, but this is only "necessary" as long as the InterruptStackFrame is not moved.
You mentioned Pin but I don't know how that would help. After all the generic type parameter for Pin needs to be a pointer to the pinned data and not an owned value.
This might be something that has to be looked at by the rust team, but I don't understand enough about the abi to feel comfortable to create an issue about that there.

[Freax13]

I don't really understand why your above example doesn't work with the interrupt ABI but I am willing to take your word for it.

Here's another angle to look at this: It's not so much that InterruptStackFrame prevents you from directly reassigning its value, the point is that InterruptStackFrame has a convenience method that allows you modify properly in a way that the compiler doesn't optimize away. Yes, it's possible to just ignore all of that and write broken code without the compiler complaining, but still InterruptStackFrame guides user towards writing proper code with volatile accesses.

Yeah. I can change my code to use InterruptStackFrameValue instead. After all they are identical on the assembly level. I will do that and I think we can close this PR.

👍

That said I think there is room for improvement of the InterruptStackFrame API. From what I understand now the main use is to ensure that modifications are done through as_mut, but this is only "necessary" as long as the InterruptStackFrame is not moved.

I wouldn't any behavior around moving an InterruptStackFrame. Rust's ABI seems to be passing large values by reference, so modifying after moving might work, but different ABIs like C take a copy of the value on the stack: https://godbolt.org/z/rGWo4WETK. Of course, Rust's ABI isn't stable, so moving might work today, but it might not work tomorrow.

You mentioned Pin but I don't know how that would help. After all the generic type parameter for Pin needs to be a pointer to the pinned data and not an owned value.

Yes, you're right that doesn't work. When I wrote that comment I mistakenly thought that x86-interrupt functions take a reference to the stack frame.

This might be something that has to be looked at by the rust team, but I don't understand enough about the abi to feel comfortable to create an issue about that there.

There's an RFC for interrupts ABIs. You could leave a comment over there.

AFAICT the RFC doesn't go into much detail regarding interrupt stack frames, but some of the example code also show the interrupt stack frame being taken by reference. Taking by reference used to work, but newer LLVM releases don't support that.

[phil-opp]

It's been a few years since I wrote that code, but I think the main reason for this InterruptStackFrame wrapper was that the x86-interrupt calling convention as defined by LLVM passes the stack frame by value, instead of by &mut reference. This is very confusing because it looks like just a normal stack variable on the Rust side. However, modifying this "stack variable" actually modifies the interrupt stack frame that will be used by iretq.

There are a few caveats to this:

• The Rust compiler does not know about the side effects of modifying these special "stack variables". So it might optimize writes away, which can change the behavior drastically.
  • To avoid this footgun, the wrapper type forces you to go through a Volatile on as_mut.
• Modification of stack variables is safe, but modifying the interrupt stack frame can lead to undefined behavior. So having a safe way to modify the interrupt stack frame would be an unsound API.
  • The wrapper type avoids this by making the as_mut method unsafe.
• Modifying the interrupt stack frame only has effects if the original stack variable is modified. If the value is copied to a different location before the modification it no longer has effects to iretq.
  • Because of this, the InterruptStackFrame type deliberately does not implement Clone or Copy. We want people to use &mut InterruptStackFrame instead of taking InterruptStackFrame by value. If you're only interested in the data, you can use the inner InterruptStackFrameValue type.
  • Note that this design is not perfect as moves are still possible (see below).

Let's take another look at the example you posted above:

If you use the "x86-interrupt" abi with InterruptStackFrame it really should be Copy on a conceptual level.

Take something like this

extern "x86-interrupt" fn inerrupt_handler(isf: InterruptStackFrame) {
    foo(isf)
}
fn foo(isf: InterruptStackFrame) {
}

Here we move isf out of interrupt_handler into foo. However interrupt_handler implicitly still uses isf at the end to perform the iretq instruction. Sure, that access is outside of rust so it does not really break the idea that the value no longer exists after a move.

The problem with this example is that the foo method is not able to modify the actual interrupt stack frame that will be used for iretq. The reason is that the stack frame is moved, which leads to a new copy of the value on the stack. The iretq only looks at the original copy passed to inerrupt_handler, so any as_mut call in foo will be without effect on iretq.

Ideally, we would forbid moves of the value, but I don't know how that is possible. The signature of the x86-interrupt function is defined by LLVM/rustc, so we cannot change it in our library. Normally, the Pin type is used to represent something that should not move in memory, but it is based on pointer types as well.

---

On a general note, I'm no longer sold on the x86-interrupt ABI. It is convenient for sure, but it is repeatedly broken in subtle ways by LLVM updates and the current design makes it very difficult to provide a sound abstraction (as we saw above). Because of these limitations, I don't think that the x86-interrupt ABI will be stabilized anytime soon, if ever. So it would probably be a good idea to provide an alternative way to use our IDT types, e.g. by providing custom naked functions (requiring unsafe of course).