Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Add realign_stack attribute to rustc #3594

Open
wants to merge 34 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
538ef0e
first commit
benisxdxd Mar 26, 2024
835cedd
rename to md
benisxdxd Mar 26, 2024
4afefe0
change to realign_stack
benisxdxd Mar 26, 2024
8d1cb35
missing .
benisxdxd Mar 26, 2024
9e897ae
add missing ret value in example
benisxdxd Mar 26, 2024
0624ecc
Rename md
benisxdxd Mar 26, 2024
1de9072
forgot in rename
benisxdxd Mar 26, 2024
50d0ee8
removed quotes
benisxdxd Mar 26, 2024
95b1f21
more changes
benisxdxd Mar 26, 2024
e90efd6
add align option
benisxdxd Mar 26, 2024
ce1e109
Remove word
benisxdxd Mar 26, 2024
cadcee7
added align example
benisxdxd Mar 26, 2024
7ab70ad
mainly rename
benisxdxd Mar 28, 2024
4a41527
Add paragraph about ISR
benisxdxd Mar 30, 2024
72b05ca
Change a bit
benisxdxd Mar 31, 2024
44dc5e8
Removed `align` argument from realign_stack
benisxdxd Mar 31, 2024
78f5c71
Update text/3594-expose-stackrealign-attribute.md
benisxdxd Mar 31, 2024
8b5a1a2
Added a paragraph in the motivation and the guide-level explanation
benisxdxd Mar 31, 2024
5309370
data layout comment
benisxdxd Apr 2, 2024
5f002ab
Update text/3594-expose-stackrealign-attribute.md
benisxdxd Apr 2, 2024
106ee87
added sentence about gcc machine depndent options
benisxdxd Apr 5, 2024
6245137
future -mstackrealign
benisxdxd Apr 5, 2024
092d378
Update text/3594-expose-stackrealign-attribute.md
benisxdxd Apr 8, 2024
ee47897
Update text/3594-expose-stackrealign-attribute.md
benisxdxd Apr 8, 2024
caebda5
Update text/3594-expose-stackrealign-attribute.md
benisxdxd Apr 13, 2024
8e9ecf4
Update text/3594-expose-stackrealign-attribute.md
benisxdxd Apr 13, 2024
8266d77
Add why this RFC was proposed and not any of the alternatives
benisxdxd Apr 13, 2024
23d2be2
Added more points in the Rationale and alternatives
benisxdxd Apr 14, 2024
fb9d206
Minor word change
benisxdxd Apr 26, 2024
058cf63
Add example in the reference level explanation.
benisxdxd Apr 26, 2024
f40b834
Comment on force add to all extern c function
benisxdxd Apr 26, 2024
8b39d24
extra paragraph about new ABI option
benisxdxd Apr 26, 2024
2b7d7b7
Spacing and extra paragraph
benisxdxd Apr 26, 2024
4f58520
Minor cleanup
benisxdxd Apr 30, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 107 additions & 0 deletions text/3594-expose-stackrealign-attribute.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
- Feature Name: (`realign-stack-attr`)
- Start Date: (2024-03-26)
- RFC PR: [rust-lang/rfcs#3594](https://github.com/rust-lang/rfcs/pull/3594)
- Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000)

# Summary
[summary]: #summary


Provide a way to generate functions that are "robust" to being called on a misaligned stack.


# Motivation
[motivation]: #motivation

There are situations where functions will be called with a lower stack alignment than what is typically expected on the current target:

- Interrupt service routines (ISRs) being called by the CPU. When an interrupt occurs, the processor saves the current execution context onto the stack before transferring control to the ISR. However, the stack might not be aligned to the required boundary, especially in embedded systems where memory constraints are tight.
- Interacting with legacy code that was built using a compiler flag that reduces the stack alignment, such as `-mpreferred-stack-boundary` on GCC. This flag says in the documentation "It is recommended that libraries that use callbacks always use the default setting", but not all libraries heed this advice. To make it possible to link those libraries with Rust code, the Rust functions they call must be "robust" to being called on a stack that was not properly aligned.

By exposing a stack realignment feature to Rust, developers working on embedded systems or performance-critical applications gain the ability to guarantee stack alignment within ISRs and other critical code paths directly from Rust code. This not only simplifies development but also enhances the reliability and performance of systems that rely on interrupt handling.

In the example above the "contract" is defined by hardware and very hard\impossible to work around.

The proposed attribute will tell the compiler that the precondition that the stack is aligned might not be true, and that it might need to fix it up in order to execute the function correctly.


# Guide-level explanation
[guide-level-explanation]: #guide-level-explanation
The `[realign_stack]` attribute can be added to a function to tell the compiler to add stack re-alignment to that function if necessary.
Useful in cases where your code is called from a thread or a binary compiled with another compiler, that uses different alignment and thus lead to a corruption.
An example of one such setting could be `-mpreferred-stack-boundary=2` in GCC which would set the stack alignment to 4 instead of the default value for the ABI which is 16.
Other such settings could be present at GCC's "Machine-Dependent Options", which there are many of, and many of them can break ABI compatibility.

```
#[realign_stack]
#[no_mangle]
pub extern "C" fn callback_function() -> i32 {
println!("Called from callback!!");

0
}
```

# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation
The realign_stack attribute is specified as follows:
```
#[realign_stack]
```

When the `realign_stack` attribute is applied to a function, the compiler no longer assumes the stack is properly aligned when the function is called, and so will insert code to forcefully realign the stack as needed for calling other functions, variables requiring alignment, etc.
This alignment is achieved by adjusting the stack pointer accordingly to the stack alignment specified in the target ABI's data layout.

In LLVM the `alignstack` gets an argument that specifies the alignment in bytes(also must be a power of 2).
Below is an example of how it would work for an example data-layout:
`e-m:e-i64:64-f80:128-n8:16:32:64-S128`.
The `S128` is the part that describes the natural stack alignment in bits.
So practically, we just need to divide that value by 8, and place it as the argument of `alignstack`.
So in the `S128` case it would look like this: `alignstack=16`.

```
define i32 @callback_function() #0 {
start:
ret i32 0
}

attributes #0 = { alignstack=16 }
```


# Drawbacks
[drawbacks]: #drawbacks
- Introducing a new attribute adds complexity to the language.
- Limited use cases: Stack realignment may not be necessary in most Rust codebases, potentially making this feature less relevant for many developers.

# Rationale and alternatives
[rationale-and-alternatives]: #rationale-and-alternatives
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another alternative would be to add a new ABI that captures "function which can be called with any stack alignment". This section should discuss why that alternative was not proposed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a paragraph about why this was chosen

An alternative could be a macro workaround instead of adding the attribute.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Macros don't extend what can be expressed in Rust: anything that can be done by a macro can also be done without a macro (just replace the macro invocation with its expansion)—they just make it less verbose/more ergonomic. It therefore isn't clear to me what is meant by this alternative? A more ergonomic way to define a naked function in assembly that realigns its stack, perhaps?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah.
I meant that maybe this RFC could already be implemented purely in rust already with some naked function and inline assembly shenanigans

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've said that you're currently blocked on this issue. Have you explored this macro idea as a possible solution? @Amanieu's naked_function::naked macro might be a good starting point?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really good enough with macros and such to do that but yeah I think I will maybe try exploring something along that way.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest that you start by using that naked macro just to implement a stack-realigning prelude that then calls or jumps to your actual logic defined separately in a regular function.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems very interesting.
Why did you choose a macro_rules and not proc_macro_attribute?
Also seems very nice that paste crate

Copy link
Member

@programmerjake programmerjake May 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just thought of a waay easier workaround that's practically always correct: just guarantee you have a local variable with alignment greater than required by the ABI (so > 16 bytes for x86), LLVM will then have to align the stack for that variable, no complex inline asm required: https://rust.godbolt.org/z/WaExsWvGj

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you choose a macro_rules and not proc_macro_attribute?

Mostly for ease of implementation, but also is faster to compile. If you'd prefer the API of an attribute macro, it'd be pretty easy to make one emit an invocation of the macro above.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just thought of a waay easier workaround that's practically always correct: just guarantee you have a local variable with alignment greater than required by the ABI

But isn't the problem that the arguments are placed on the misaligned stack before such forcibly aligned local variable? I can't see any way to cope with that on stable Rust today other than via global_asm (here via naked-function crate). The problem then becomes how one gets from such asm to a function body that has been written in Rust... and alas, I think that has to involve copying the (on-stack) arguments and making a call.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just thought of a waay easier workaround that's practically always correct: just guarantee you have a local variable with alignment greater than required by the ABI

But isn't the problem that the arguments are placed on the misaligned stack before such forcibly aligned local variable? I can't see any way to cope with that on stable Rust today other than via global_asm (here via naked-function crate). The problem then becomes how one gets from such asm to a function body that has been written in Rust... and alas, I think that has to involve copying the (on-stack) arguments and making a call.

make the function called by the trampoline #[inline(never)]:
either:

#[repr(align(32))]
struct Aligned;

pub unsafe extern "C" fn the_fn(a: u32, b: u64, c: &u8) -> u64 {
    let align = Aligned;
    asm!("/* {} */", in(reg) &align);
    the_fn_impl(a, b, c)
}

#[inline(never)]
fn the_fn_impl(a: u32, b: u64, c: &u8) -> u64 {
    todo!()
}

or:

/// workaround unstable attributes on expressions
#[inline(always)]
fn identity<T>(v: T) -> T {
    v
}

#[repr(align(32))]
struct Aligned;

pub unsafe extern "C" fn the_fn(a: u32, b: u64, c: &u8) -> u64 {
    let align = Aligned;
    asm!("/* {} */", in(reg) &align);
    identity(#[inline(never)] || the_fn_impl(a, b, c))()
}

fn the_fn_impl(a: u32, b: u64, c: &u8) -> u64 {
    todo!()
}

However it would be more like band-aid than an actual solution as llvm exports this functionality anyways.

A different alternative could be adding the any extern "C" function the `stackrealign` attribute implicitly which would solve the main use-case. However, this is at the cost of added overhead where callers abide by the target's stack alignment, as in the majority of cases.
Also, this flag could be exported as an option in the Cargo.toml file so only projects that need it can use it and other projects will remain unaffected.
The downside for this is less control on which function has it's stack realigned.

An extra option could be not verifying data-layout for custom targets provided via `--target=`, which would allow users to patch the "natural stack alignment" in their custom target which should relax LLVM stack alignment assumptions that are present in the system.
Another alternative could be adding a new ABI that captures "function which can be called with any stack alignment".
I chose to propose this RFC and not any of the alternatives because it seems to me that this proposition provides the simplest solution, a solution that is very close to `force_align_arg_pointer` function attribute in GCC and a solution that is easy to implement for rustc.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"easy to implement" and "it's like what GCC does" are not very strong arguments, IMO. The question is what is best for Rust programmers.

@Lokathor raised some points for why they did not think a separate ABI was a good idea; those should be reflected here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added more explaining there

While creating a different ABIs to handle stack realignment could be a viable alternative, introducing a new function attribute like realign_stack in Rust offers several advantages. Firstly, leveraging function attributes aligns with Rust's philosophy of providing clear and concise language features, ensuring that developers can easily understand and apply stack realignment where necessary. Also, if the realign_stack was a part of the ABI we would need to practically duplicate every ABI and create a copy where one has that attribute and the other does not. This would lead to a higher level of complexity and would require higher maintenance over time.
Copy link

@eggyal eggyal Apr 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key point here is that having the ability to be called with an unaligned stack is orthogonal to Rust's existing ABI definitions; perhaps another approach could be for the language to support ABIs as a composable set of features along different axes, e.g. extern "C" + "realign_stack" fn ... or somesuch? That way, it could still form part of the function's type signature/ABI while avoiding exponential growth in the number of explicitly-defined ABIs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good enough to me but im not sure it is much different from something like the no_mangle attribute which some people also think should be unsafe.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already have duplicate ABIs for supporting unwinding extern "C-unwind", so I quite like the ABIs as a composable set of features idea! extern "C" + "unwind" + "align_stack" fn()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not know that actually and this does look nice.
However I think a different RFC might be better for that.
Also, should be noted, there is still #[unwind(allowed)] and #[unwind(abort)] which might suggest that maybe we should be doing both the ABI and attribute solutions

Copy link
Member

@RalfJung RalfJung Apr 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Firstly, leveraging function attributes aligns with Rust's philosophy of providing clear and concise language features, ensuring that developers can easily understand and apply stack realignment where necessary.

I don't know what you mean by this. Rust's general philosophy is to have a strong type system that captures everything needed to ensure soundness. The "Rust approach" to this would IMO clearly be to reflect this in the function type.

I don't think we have a single existing attribute that works like yours. This is not a "concise language feature", it is an attribute that leaks low-level implementation concerns without properly reflecting them in the type system.

If we follow your arguments we should remove the borrow checker since it is a high level of complexity and requires high maintenance over time. But obviously that would be a bad idea, since we put a high priority on having the compiler catch your mistakes. IMO you should at the very least acknowledge that there is a trade-off here and that there are good arguments for both versions. Currently the RFC doesn't read like you are seriously considering the alternative of doing proper ABI treatment of this issue, instead you are dismissing the alternative on vague grounds without giving it a proper chance. In an RFC you are expected to give good arguments for things you do not agree with -- that's how good discussions look like.

But anyway this is getting into the editorial part of how to write a text that weighs the arguments for two sides of a trade-off in a fair manner and justifies the conclusion taken -- I'm afraid I don't currently have the capacity to help with that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an extra paragraph about the whole new ABI option.
I would be glad to more pros for that option if I can think of more (or if you tell me them :) ).
I think we are overthinking this whole thing though. If we see in the future that this causes problems could remove it and do the ABI option instead.

Copy link
Member

@programmerjake programmerjake Apr 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we see in the future that this causes problems could remove it and do the ABI option instead.

removing it may not be backward compatible, so we'd have to do something like deprecate it in all editions < some cutoff and just not have it in editions after that, and have the alternate ABI stuff instead.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah true you are correct I was not thinking about that.
I hope we won't need to remove it, just like it is still present in GCC and clang c still but yeah.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I was looking into the ABI option and came across this, a file @RalfJung actually edited not too away from now.
Seems to me that if every time we want to add something like this we need to change the ABI, it would become very difficult to scale and maintain. @eggyal comment on that seems nice though.

Using a function attribute offers finer granularity and control, enabling developers to selectively apply stack realignment to specific functions without affecting the entire ABI.

In the future if it is seems import enough, we should reconsider the added ABI option which also has benifits of its own: stack alignment is really part of the ABI so it could be perhaps easier to new comers of the language to find this "realign_stack" feature there. New ABIs can be standardized later for other languages as well which would improve interoperability overall.
The main thing we get with `realign_stack` function attribute as opposed to a new ABI is the simplicity of implementation and "closeness" of implementation to c with `force_align_arg_pointer`.

# Prior art
[prior-art]: #prior-art
This feature is present in GCC via the `force_align_arg_pointer` attribute.
Also present in LLVM in general.

# Unresolved questions
[unresolved-questions]: #unresolved-questions

# Future possibilities
[future-possibilities]: #future-possibilities
- Explore additional LLVM features that could be exposed to Rust for performance optimization purposes.
- Add a rustc compilation flag that adds this attribute to every `pub extern "C"` function (similiar to `-mstackrealign` which does this globally in GCC).
- Similar to `extern "C-unwind"` and `#[unwind(allowed)]` we could add new ABIs (something like `extern "C-realign-stack"`) which would do the same as `#[realign_stack]`.