Skip to content
This repository was archived by the owner on Nov 28, 2023. It is now read-only.

Ensure sp is 16-byte aligned #129

Merged
merged 1 commit into from
Nov 13, 2023
Merged

Conversation

ivq
Copy link
Contributor

@ivq ivq commented Nov 10, 2023

The problem

See #75. and rust-lang/rust#86693.
I did the small experiment to illustrate the problem:

#[repr(align(16))]
struct AlignedU8(u8);

pub fn test_align() {
    let _p = AlignedU8(3);
}

Generated code(target = riscv32imac-unknown-none-elf):

20000228 <_ZN13riscv_rt_test10test_align17hdcaf8a1e3d5da475E>:
20000228:	1141                	addi	sp,sp,-16
2000022a:	450d                	li	a0,3
2000022c:	00a10023          	sb	a0,0(sp)
20000230:	0141                	addi	sp,sp,16
20000232:	8082                	ret

We can see that rustc/llvm is supposing that sp is 16-byte aligned to allocate _p(indeed, to do any stack allocations). But if sp is not 16-byte aligned(8 or smaller), _p will be allocated at incorrect location.

Fix

According to https://github.com/riscv-non-isa/riscv-eabi-spec/blob/master/EABI.adoc#4-eabi-stack-alignment, EABI requires that RV32 stack is 8-byte aligned and 16-byte for RV64. But to achieve max compatibility and to be correct, we could simply force 16-byte. It's small drawback is memory waste of a maximum of 8 bytes.

Fix this by forcing stack pointer to be 16-byte aligned.

Fixes #75.

Signed-off-by: Chien Wong <m@xv97.com>
@ivq ivq requested a review from a team as a code owner November 10, 2023 13:19
@romancardenas
Copy link
Contributor

Thanks for the PR!

I'm a bit afraid about multi-core systems in which stacks may overlap... perhaps it is a better idea to use the linker file to force the stack pointer and the stack size to be correctly aligned to 16 bytes. What do you think? In this way, we could also leave the assembly code untouched (and slightly shorter)

@ivq
Copy link
Contributor Author

ivq commented Nov 11, 2023

Thanks for the PR!

I'm a bit afraid about multi-core systems in which stacks may overlap... perhaps it is a better idea to use the linker file to force the stack pointer and the stack size to be correctly aligned to 16 bytes. What do you think? In this way, we could also leave the assembly code untouched (and slightly shorter)

I don't think the patch would lead to stack overlapping. The equivalent effect of aligning stack pointer is reserving a few bytes(up to 15) at top of the stack.
I did consider checking in linker file but thought handling in startup code is better. Leaving the responsibility to the user sounds a bit clumsy if we could handle it easily without much cost in the runtime implementation. Also, I'm afraid checking in linker script would break building of some of the existing projects if upgrading rt crate.

Copy link
Contributor

@romancardenas romancardenas left a comment

Choose a reason for hiding this comment

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

LGTM

@romancardenas romancardenas self-requested a review November 13, 2023 11:20
@romancardenas romancardenas added this pull request to the merge queue Nov 13, 2023
Merged via the queue into rust-embedded:master with commit 4da463e Nov 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sp may not be aligned to 16 bytes
2 participants