Bump riscv-rt to v0.11.0

[a-gavin]

Fix #34

[a-gavin]

riscv-rt v0.10.* have been yanked, use v0.11.0 instead

[almindor]

Hmm odd linker errors in the builds, i'll try and look at them later

[a-gavin]

I tested on rustc 1.65.0. Running same command in build on rustc 1.67.0 fails :thinking:

[a-gavin]

The following examples fail to compile when building w/o --release (all require enabling feature lcd or sdcard):

• led_shell
• display
• ferris
• sdcard_test <-- Only one to not compile when building w/ --release

Edit: Spoke too soon w/ prev message!

[almindor]

Ok I had a look on local. I don't currently have this board to actually test anything with.

I also updated riscv to latest (this needs a fix to the interrupt::free calls but it's a simple one). The main i ssue is that the sdcard example for some reason no longer fits into the flash memory. I'm guessing riscv-rt increased in size making it overflow by ~460bytes now.

Switching to the memory-cb.x variant makes it work of course since we have bigger flash but it'd effectively mean that the sdcard wouldn't be usable anymore.

I'm a bit strapped for time atm. We need to look at by how much the riscv-rt change grew our code. What is a bit odd is that the DATA portion is also claimed to be insufficient now. I can understand RODATA growing, but why did DATA got bigger is a mystery to me.

[almindor]

@Disasm any ideas on this one?

[Disasm]

I'd suggest merging this PR and excluding sdcard_test from CI (or maybe replacing release build with a simpler check).

[almindor]

I'd suggest merging this PR and excluding sdcard_test from CI (or maybe replacing release build with a simpler check).

We could just switch to using memory-cb.x for the CI tests and they all work, but I'm really curious as to why bumping riscv-rt causes this to be ~500b less in free memory.

[a-gavin]

Sorry I wasn't able to look into this sooner, real life has taken over my free time lately.

I did some digging into this, and I'm pretty sure that this isn't a riscv-rt version issue but is instead a breakage when using rustc versions v1.61+. I'm not sure what the best way to fix this issue is, but it would be a bummer if we couldn't get sdcard_test to build using memory-c8.x.

Using rustc 1.64 (pre-LLVM issue which requires bumping riscv-rt) and --release to build both the current master 328be12 and this PR, both fail to build the sdcard_test example using the default memory-c8.x. Both do succeed, though, when using memory-cb.x, given the larger flash size.

This lead me to testing other rustc versions using commit 328be12, --release, and memory-c8.x to see when the sdcard_test example fails to build. After some binary searching rustc versions, this configuration compiles the sdcard_test example with rustc v1.60 but fails with rustc v1.61.

Somewhat related, building the sdcard_test example (successfully!) using commit 328be12, --release, and memory-cb.x for both rustc v1.60 and v1.61, the text size increases by just under 2000 bytes from v1.60 to v1.61. Other sections stay the same:

$ size sdcard_test_1.60_memory-cb
   text    data     bss     dec     hex filename
  64928       8   32760   97696   17da0 sdcard_test_1.60_memory-cb
$ size sdcard_test_1.61_memory-cb
   text    data     bss     dec     hex filename
  66780       8   32760   99548   184dc sdcard_test_1.61_memory-cb

In case you'd like to check as well, here's what I did for each:

# Checkout current master
git checkout 328be12

# Build with rustc 1.60
cargo clean
rustup install 1.60
rustup target add riscv32imac-unknown-none-elf --toolchain 1.60-x86_64-unknown-linux-gnu
cargo +1.60-x86_64-unknown-linux-gnu build --example sdcard_test --all-features --release # Should succeed

# Build with rustc 1.61
cargo clean
rustup install 1.61
rustup target add riscv32imac-unknown-none-elf --toolchain 1.61-x86_64-unknown-linux-gnu
cargo +1.61-x86_64-unknown-linux-gnu build --example sdcard_test --all-features --release # Should fail

[a-gavin]

Also, I'm not really sure what changed between v1.60 to v1.61 that would affect this. I can try to look into it, but I'm not sure if that will bear fruit. Here is the v1.61 changelog for reference.

[almindor]

I think it has to do with this:

Previously native static libraries were linked as whole-archive in some cases, but now rustc tries not to use whole-archive unless explicitly requested. This https://github.com/rust-lang/rust/pull/93901/ may result in linking errors in some cases. To fix such errors, native libraries linked from the command line, build scripts, or [#[link] attributes](https://doc.rust-lang.org/stable/reference/items/external-blocks.html#the-link-attribute) need to
(more common) either be reordered to respect dependencies between them (if a depends on b then a should go first and b second)
(less common) or be updated to use the [+whole-archive](https://doc.rust-lang.org/stable/rustc/command-line-arguments.html#linking-modifiers-whole-archive) modifier.

However using RUSTFLAGS=-Clink-arg=--whole-archive cargo build --target riscv32imac-unknown-none-elf --example sdcard_test --features=sdcard --release errors out with rust-lld: error: undefined symbol: _mp_hook and others so I'm not sure.

It does seem to point to an ordering issue with our link scripts though. @Disasm any thoughts? This is a bit above my paygrade wrt. linking knowledge :)

[almindor]

After looking into this a bit more, I couldn't find anything specific that caused it but it seems rust v1.60 -> 1.61 had a size regression.

Using

[profile.release]
opt-level = "z"  # Optimize for size.
codegen-units = 1
lto = true

Will make this work even with the smaller memory profile. @a-gavin could you please add that, at least to the CI? We should be good to go afterwards.

[a-gavin]

Iirc Rust weekly newsletter posts weekly regression statistics. Off the top of your head, do you know if this exists for rustc updates as well?

No idea :(

Setting the same options for development builds works as well. Is that something I should add too? That's a bit of problem coz the idea behind dev builds is that they should be quick. Maybe just the opt-level = z if it works on its own.

Also unrelated, but is there a way to build an example using a development profile but build its dependencies using their release profiles?

Not that I am aware of, but I am not sure.

[a-gavin]

As per your suggestion, I added a commit w/ a release profile verbatim as well as a development profile with only opt-level = z.

All examples build locally using rustc 1.67 for both release and development using the following commands :

cargo build --examples --all-features
cargo build --examples --all-features --release

[almindor]

As per your suggestion, I added a commit w/ a release profile verbatim as well as a development profile with only opt-level = z.

All examples build locally using rustc 1.67 for both release and development using the following commands :

cargo build --examples --all-features
cargo build --examples --all-features --release

Perfect. As a last point, sorry I forgot to mention this before, but could you also bump riscv to latest as well? It does need one small change in the interrupt handling code (param in the closures) but it should be trivial. It seems to me we'd want to keep the riscv dependency in sync together with riscv-rt

[a-gavin]

Perfect. As a last point, sorry I forgot to mention this before, but could you also bump riscv to latest as well? It does need one small change in the interrupt handling code (param in the closures) but it should be trivial. It seems to me we'd want to keep the riscv dependency in sync together with riscv-rt Totally. I'll take care of that as soon as I can.

Also, kind of found what I was looking for regarding rustc regression testing--maybe you'll find useful as well. Regression tests and associated triaging documents (which happen on a weekly basis) can be found in this repo. Performance results can be found here, with specific test results on the compare page. A summary of these results appear in the This Week in Rust newsletter.

[a-gavin]

Just pushed the riscv crate update to v0.10.1 w/ required interrupt closure changes. As detailed in the commit message, using the recently-changed riscv interrupt free section impl is still safe for the Longan Nano as the GD32VF103 chip it uses only supports a single hart (which is the limitation of the recently-changed interrupt free section impl)