Initialize RAM in assembly

rust-embedded / cortex-m-rt

Minimal startup / runtime for Cortex-M microcontrollers

https://rust-embedded.github.io/cortex-m-rt/

Apache License 2.0

359 stars 85 forks source link

Initialize RAM in assembly #301

Closed jonas-schievink closed 3 years ago

jonas-schievink commented 4 years ago

Fixes #300

rust-highfive commented 4 years ago

r? @thalesfragoso

(rust-highfive has picked a reviewer for you, use r? to override)

adamgreig commented 3 years ago

GitHub won't let me comment outside the diff itself, so..

The comments above PreResetTrampoline still refer to it being an ARMv6-M-only thing that sets LR; they need updating to say it's actually the reset vector on all architectures and initialises bss and data.
This change causes us to write LR on every architecture, instead of only ARMv6-M, unnecessarily. Not sure what the cleanest way to only do it on v6 is, but I'd rather we didn't always set it on v7.
Not sure if the names are still right. Perhaps the assembly stub should be Reset now, since it's always the reset vector and contains memory initialisation?
We still expose pre_init before bss/data are initialised, which I think is the most useful option but the whole reason for this PR is because "life before main" is UB; we should update the documentation around pre_init in this PR too. Or add post_init?

jonas-schievink commented 3 years ago

This change causes us to write LR on every architecture, instead of only ARMv6-M, unnecessarily. Not sure what the cleanest way to only do it on v6 is, but I'd rather we didn't always set it on v7.

It actually gets immediately overwritten by the call to __pre_init, so it seems like this might break unwinding? Seems like we either have to write it again after __pre_init returns or save it in another register and write the appropriate CFI directive (no idea what that would look like).

jonas-schievink commented 3 years ago

Updated the docs in the assembly. Also made it restore LR after __pre_init returns.

We still expose pre_init before bss/data are initialised, which I think is the most useful option but the whole reason for this PR is because "life before main" is UB; we should update the documentation around pre_init in this PR too. Or add post_init?

Do we want to remove/deprecate #[pre_init] entirely? It already has a big warning about accessing statics inside of it. If we do want to remove or deprecate it I think we might want to wait until asm!/#[naked] are stable so that users have a viable alternative by writing their __pre_init in inline assembly.

post_init is just entry, so I don't see what that would let people do that they can't already.

Sympatron commented 3 years ago

I think #303 shows that #[pre_init] can be useful in some cases. Without it there would be no way to do this. With enough warnings in the docs, I would vote to leave it there in case someone needs it.

jonas-schievink commented 3 years ago

To be clear, we're not taking away the ability to run code before RAM initialization in general, but allowing arbitrary Rust code in a #[pre_init] function is problematic.

bors[bot] commented 3 years ago

Build succeeded: