Skip to content
This repository was archived by the owner on Jan 24, 2022. It is now read-only.

Initialize RAM in assembly #301

Merged
merged 6 commits into from
Jan 21, 2021
Merged

Initialize RAM in assembly #301

merged 6 commits into from
Jan 21, 2021

Conversation

jonas-schievink
Copy link
Contributor

Fixes #300

@rust-highfive
Copy link

r? @thalesfragoso

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

@adamgreig
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

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.

* Use arm-none-eabi-gcc to assemble, allowing use of preprocessor to
  conditionally enable the FPU for eabihf targets.
* Remove has_fpu configuration from build.rs.
* Remove FpuTrampoline as no longer required.
* Remove the Rust Reset method entirely, since the asm Reset can now
  enable FPU and jump to user main.
Copy link
Member

@adamgreig adamgreig left a comment

Choose a reason for hiding this comment

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

Thanks!

bors merge

@bors
Copy link
Contributor

bors bot commented Jan 21, 2021

@bors bors bot merged commit fc919a2 into rust-embedded:master Jan 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RAM initialization code violates pointer provenance
8 participants