Merge pull request #282 from rust-embedded/pre-init

`riscv-rt`: Leave `__pre_init` under a feature gate

Author: romancardenas

.github/workflows/clippy.yaml

@@ -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:

Cargo.toml

@@ -10,3 +10,11 @@ members = [
     "tests-build",
     "tests-trybuild",
 ]
+
+default-members = [
+    "riscv",
+    "riscv-pac",
+    "riscv-peripheral",
+    "riscv-rt",
+    "riscv-semihosting",
+]

riscv-rt/CHANGELOG.md

@@ -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
 
@@ -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
 

riscv-rt/Cargo.toml

@@ -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",
@@ -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"]

riscv-rt/macros/src/lib.rs

@@ -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
@@ -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")]
 #[proc_macro_attribute]
 pub fn pre_init(args: TokenStream, input: TokenStream) -> TokenStream {
     let f = parse_macro_input!(input as ItemFn);

riscv-rt/src/asm.rs

@@ -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
@@ -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`.

riscv-rt/src/exceptions.rs

@@ -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" {

riscv-rt/src/interrupts.rs

@@ -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.
 //!

riscv-rt/src/lib.rs

@@ -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.
@@ -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).
@@ -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

tests-build/Cargo.toml

@@ -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"]

tests-build/src/lib.rs

@@ -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() {}