Skip to content

riscv-rt: Leave __pre_init under a feature gate #282

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

Merged
merged 2 commits into from
May 7, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions .github/workflows/clippy.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ jobs:
- name: Run clippy (no features)
run: cargo clippy --all --no-default-features -- -D warnings
- name: Run clippy (all features)
# We exclude riscv-peripheral because it's not yet stable-compliant
run: cargo clippy --exclude riscv-peripheral --all --all-features -- -D warnings
# We exclude riscv-peripheral because it's not yet stable-compliant (added -A deprecated for pre_init macro)
run: cargo clippy --exclude riscv-peripheral --all --all-features -- -D warnings -A deprecated

# Additonal clippy checks for riscv-rt
clippy-riscv-rt:
Expand Down
8 changes: 8 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,11 @@ members = [
"tests-build",
"tests-trybuild",
]

default-members = [
"riscv",
"riscv-pac",
"riscv-peripheral",
"riscv-rt",
"riscv-semihosting",
]
5 changes: 5 additions & 0 deletions riscv-rt/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
The benefits of leaving this optional are backwards compatibility and
allowing users to define less typical linker scripts that do not rely on a
`device.x` or `memory.x` file.
- New `pre-init` feature to run a `__pre_init` function before RAM initialization.

### Changed

Expand All @@ -34,6 +35,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- Replace weak definition of `_start_trap` with `PROVIDE(_start_trap = _default_start_trap)`.
- Replace weak definition of `_setup_interrupts` with `PROVIDE(_setup_interrupts = _default_setup_interrupts)`.
- Now, `_default_start_trap` is 4-byte aligned instead of target width-aligned.
- Remove `__pre_init` function from default `riscv_rt`. Now, if users want a `__pre_init` function,
they must enable the `pre-init` feature.
- Deprecate `riscv_rt::pre_init` attribute macro. It is not sound to run Rust code before initializing the RAM.
Instead, we recommend defining the `__pre_init` function with `core::arch::global_asm!`.

### Fixed

Expand Down
2 changes: 2 additions & 0 deletions riscv-rt/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ links = "riscv-rt" # Prevent multiple versions of riscv-rt being linked

[package.metadata.docs.rs]
default-target = "riscv64imac-unknown-none-elf"
features = ["pre-init"]
targets = [
"riscv32i-unknown-none-elf", "riscv32imc-unknown-none-elf", "riscv32imac-unknown-none-elf",
"riscv64imac-unknown-none-elf", "riscv64gc-unknown-none-elf",
Expand All @@ -31,6 +32,7 @@ riscv-rt-macros = { path = "macros", version = "0.4.0" }
panic-halt = "1.0.0"

[features]
pre-init = []
s-mode = ["riscv-rt-macros/s-mode"]
single-hart = []
v-trap = ["riscv-rt-macros/v-trap"]
Expand Down
10 changes: 9 additions & 1 deletion riscv-rt/macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,15 @@ fn is_correct_type(ty: &Type, name: &str) -> bool {
}

/// Attribute to mark which function will be called at the beginning of the reset handler.
/// You must enable the `pre_init` feature in the `riscv-rt` crate to use this macro.
///
/// **IMPORTANT**: This attribute can appear at most *once* in the dependency graph. Also, if you
/// # IMPORTANT
///
/// This attribute is **deprecated**, as it is not safe to run Rust code **before** the static
/// variables are initialized. The recommended way to run code before the static variables
/// are initialized is to use the `global_asm!` macro to define the `__pre_init` function.
///
/// This attribute can appear at most *once* in the dependency graph. Also, if you
/// are using Rust 1.30 the attribute must be used on a reachable item (i.e. there must be no
/// private modules between the item and the root of the crate); if the item is in the root of the
/// crate you'll be fine. This reachability restriction doesn't apply to Rust 1.31 and newer
Expand All @@ -197,6 +204,7 @@ fn is_correct_type(ty: &Type, name: &str) -> bool {
///
/// # fn main() {}
/// ```
#[deprecated(note = "Use global_asm! to define the __pre_init function instead")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Will we be removing this in the next (or another future) point release?

I wasn't aware that using Rust code before RAM init was unsound. Is this true even for code that only uses the stack?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about how Rust's ABI can break something. I guess that, if you do not use static variables, it should be fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From cortex-m documentation:

A #[pre_init] macro is also provided to run a function before RAM initialisation, but its use is deprecated as it is not defined behaviour to execute Rust code before initialisation. It is still possible to create a custom pre_init function using assembly.

Copy link
Contributor

Choose a reason for hiding this comment

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

We also discussed this on Monday's meeting. I think the consensus was that there were too many preconditions + invariants to justify keeping a pre_init implementation in Rust.

I think it may still be helpful to point to the feature-gated __pre_init function in the riscv-rt docs as an example for users that would want to implement their own. Maybe in the place currently held by #[pre_init] or above the _pre_init_trap?

#[proc_macro_attribute]
pub fn pre_init(args: TokenStream, input: TokenStream) -> TokenStream {
let f = parse_macro_input!(input as ItemFn);
Expand Down
12 changes: 4 additions & 8 deletions riscv-rt/src/asm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,10 +141,11 @@ cfg_global_asm!(

beqz a0, 4f",
);
// IF CURRENT HART IS THE BOOT HART CALL __pre_init AND INITIALIZE RAM
// IF CURRENT HART IS THE BOOT HART CALL __pre_init (IF ENABLED) AND INITIALIZE RAM
cfg_global_asm!(
"call __pre_init
// Copy .data from flash to RAM
#[cfg(feature = "pre-init")]
"call __pre_init",
"// Copy .data from flash to RAM
la t0, __sdata
la a0, __edata
la t1, __sidata
Expand Down Expand Up @@ -221,11 +222,6 @@ cfg_global_asm!(
);

cfg_global_asm!(
// Default implementation of `__pre_init` does nothing.
// Users can override this function with the [`#[pre_init]`] macro.
".weak __pre_init
__pre_init:
ret",
#[cfg(not(feature = "single-hart"))]
// Default implementation of `_mp_hook` wakes hart 0 and busy-loops all the other harts.
// Users can override this function by defining their own `_mp_hook`.
Expand Down
1 change: 0 additions & 1 deletion riscv-rt/src/exceptions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ pub static __EXCEPTIONS: [Option<unsafe extern "C" fn(&TrapFrame)>; 16] = [
///
/// This function must be called only from the [`crate::start_trap_rust`] function.
/// Do **NOT** call this function directly.
#[inline]
#[no_mangle]
pub unsafe extern "C" fn _dispatch_exception(trap_frame: &TrapFrame, code: usize) {
extern "C" {
Expand Down
2 changes: 1 addition & 1 deletion riscv-rt/src/interrupts.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! Interrupt handling for targets that comply with the RISC-V interrupt handling standard.
//!
//! In direct mode (i.e., `v-trap` feature disabled), interrupt dispatching is performed by the
//! [`_dispatch_core_interrupt`] function. This function is called by the [crate::start_trap_rust]
//! `_dispatch_core_interrupt` function. This function is called by the [crate::start_trap_rust]
//! whenever an interrupt is triggered. This approach relies on the [`__CORE_INTERRUPTS`] array,
//! which sorts all the interrupt handlers depending on their corresponding interrupt source code.
//!
Expand Down
20 changes: 18 additions & 2 deletions riscv-rt/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
//! - Before main initialization of the `.bss` and `.data` sections.
//!
//! - [`#[entry]`][attr-entry] to declare the entry point of the program
//! - [`#[pre_init]`][attr-pre-init]to run code *before* `static` variables are initialized
//! - [`#[exception]`][attr-exception] to override an exception handler.
//! - [`#[core_interrupt]`][attr-core-interrupt] to override a core interrupt handler.
//! - [`#[external_interrupt]`][attr-external-interrupt] to override an external interrupt handler.
Expand Down Expand Up @@ -476,6 +475,20 @@
//!
//! # Cargo Features
//!
//! ## `pre-init`
//!
//! The pre-init feature (`pre-init`) can be activated via [Cargo features](https://doc.rust-lang.org/cargo/reference/features.html).
//! When enabled, the runtime will execute the `__pre_init` function to be run **before RAM is initialized**.
//! If the feature is enabled, the `__pre_init` function must be defined in the user code (i.e., no default implementation is
//! provided by this crate). If the feature is disabled, the `__pre_init` function is not required.
//!
//! As `__pre_init` runs before RAM is initialised, it is not sound to use a Rust function for `__pre_init`, and
//! instead it should typically be written in assembly using `global_asm` or an external assembly file.
//!
//! Alternatively, you can use the [`#[pre_init]`][attr-pre-init] attribute to define a pre-init function with Rust.
//! Note that using this macro is discouraged, as it may lead to undefined behavior.
//! We left this option for backwards compatibility, but it is subject to removal in the future.
//!
//! ## `single-hart`
//!
//! The single hart feature (`single-hart) can be activated via [Cargo features](https://doc.rust-lang.org/cargo/reference/features.html).
Expand Down Expand Up @@ -570,7 +583,10 @@ use riscv::register::scause as xcause;
use riscv::register::mcause as xcause;

pub use riscv_pac::*;
pub use riscv_rt_macros::{core_interrupt, entry, exception, external_interrupt, pre_init};
pub use riscv_rt_macros::{core_interrupt, entry, exception, external_interrupt};

#[cfg(feature = "pre-init")]
pub use riscv_rt_macros::pre_init;

/// We export this static with an informative name so that if an application attempts to link
/// two copies of riscv-rt together, linking will fail. We also declare a links key in
Expand Down
1 change: 1 addition & 0 deletions tests-build/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ riscv = { path = "../riscv", version = "0.13.0" }
riscv-rt = { path = "../riscv-rt", version = "0.14.0" }

[features]
pre-init = ["riscv-rt/pre-init"]
single-hart = ["riscv-rt/single-hart"]
v-trap = ["riscv-rt/v-trap"]
device = ["riscv-rt/device"]
Expand Down
4 changes: 4 additions & 0 deletions tests-build/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,7 @@ pub enum Exception {
LoadPageFault = 13,
StorePageFault = 15,
}

#[cfg(feature = "pre-init")]
#[cfg_attr(feature = "pre-init", riscv_rt::pre_init)]
unsafe fn pre_init() {}