Skip to content

Conversation

@gibbz00
Copy link
Contributor

@gibbz00 gibbz00 commented Jun 29, 2025

This PR pulls in minilink; a small wrapper around MiniJinja for linker scripts. I believe that this can make linker script customization far easier. For example, esp-hal places .trap sections in RAM (esp-rs/esp-hal#541), and I would like to open a PR which does the same. Using templated linker scripts would make the addition fairly trivial:

{% if contains(cfg.feature, "trap-region") %}
.trap : ALIGN(4)
{
  *(.trap);
  *(.trap.*);
} > REGION_TRAP
{% endif %}

.text _stext :
{
  ...
  {% if not contains(cfg.feature, "trap-region") %}
  *(.trap);
  *(.trap.rust);
  {% endif %}
}

The biggest downside is the addition of build dependencies, and all that comes with it:

minilink v0.2.0
├── minijinja v2.11.0
│   └── serde v1.0.219
└── serde v1.0.219

Here are some cargo build --timings --package riscv-rt results for {debug,release} x {warm,cold} builds when executed on my laptop.

master templated diff
debug cold 2.1s 2.3s +0.2s (10%)
debug warm 0.2s 0.2s 0.0s (0%)
release cold 2.0s 2.2s +0.2s (10%)
release warm 0.2s 0.2s 0.0s (0%)

Test varied with about ±0.05s when rerun. As you can see, warm build times remain pretty much unaffected. Cold builds had about 0.2s or 10% added to them. I personally think this is manageable given the advantages of templating with MiniJinja.

I chose to create a separate crate for this, as I believe that the linker script templating may be useful outside this repo. However, since minilink is about 200 lines of code—documentation excluded—there's not much in the way of copying relevant parts directly into riscv-rt.

Alternatives

esp-hal has a very simple C-like preprocessor. Suppose something similar could be used. But there is something to be said about using a less bespoke, and well documented solution.

@gibbz00 gibbz00 marked this pull request as ready for review June 29, 2025 19:32
@gibbz00 gibbz00 requested a review from a team as a code owner June 29, 2025 19:32
Copy link
Contributor

@rmsyn rmsyn left a comment

Choose a reason for hiding this comment

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

I personally don't see a need to bring in a new dependency for our use-case in riscv-rt. The build script handles the cfg dependencies fairly well (although, the minilink-based config does look cleaner). If more of the linker script contains cfg-dependent sections, I could see more justification for the additional dependency.

Not strongly opposed to the idea, just offering my opinion.

@gibbz00
Copy link
Contributor Author

gibbz00 commented Jun 30, 2025

Fair! This may as you say be a bit premature. My initial intent with this PR was to get a feel for how others stand on it before diving head first into sending PRs using it. Perhaps it would be better to do the opposite instead? That is; send some planned PRs which depend on this one, and then discuss if templating is really needed?

On a side note. minilink-based config may look slightly cleaner here in the runtime crate, but one goal was for it to be even more so for end-users and BSP crate writers (ex.). This crate's build scripts instructions could be simplified to:

// build.rs
fn main() {
  minilink::include_template("./ld/linkall.ld.in", "linkall.ld");
}

Where the Cargo build instructions are hidden away, (which may or may not be a good thing), and there's no need to add linker flags in .cargo/config.toml. Bringing in minilink for this is only justified if it's already in the dependency tree. A chicken or the egg problem, so to speak.

@romancardenas
Copy link
Contributor

I like the idea! Let's keep this PR as a draft and then we can decide whether it is worth it.

I will probably open a few PRs that tidy up the linker file a bit, so they might affect this draft. The idea is approaching slowly to a state in which esp-riscv-rt can be built on top of riscv-rt, so probably people from esp-rs don't want to give up linking features.

@gibbz00 gibbz00 marked this pull request as draft June 30, 2025 09:27
@gibbz00
Copy link
Contributor Author

gibbz00 commented Jul 3, 2025

Closing this for now as I'll be looking at some alternative ways for running bare metal ESP :) May come back to this later

@gibbz00 gibbz00 closed this Jul 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants