Rahix / avr-hal

embedded-hal abstractions for AVR microcontrollers
Apache License 2.0
1.33k stars 225 forks source link

Upgrade rust toolchain to nightly-2024-11-05 #585

Open Rahix opened 2 months ago

Rahix commented 2 months ago

Time for another toolchain upgrade to fix a number of known compiler bugs.

Rahix commented 2 months ago

@Patryk27, some CI jobs are failing with

out of range branch target (expected an integer in the range -4096 to 4095)

is this still one of the known problems or something new?

Patryk27 commented 2 months ago

This is known, it’s already fixed in LLVM, but not yet backported - it should be in rustc within a couple days hopefully; I’ll let you know, so we can wait before bumping the toolchain here to get all of the fixes πŸ™‚

Patryk27 commented 1 month ago

@Rahix I think we should be good now - could you bump to the newest toolchain? :eyes:

Rahix commented 1 month ago

@Patryk27, unfortunately it looks like we are still hitting the dreaded error: out of range branch target with nightly-2024-10-11? :disappointed:

Just to check, I also tried nightly-2024-10-12, but I can still reproduce the error message...

Patryk27 commented 1 month ago

Ouch, my fault! It seems that https://github.com/llvm/llvm-project/pull/109124 wasn't backported - I'll handle it tomorrow. Other commits seem to be in place, though (e.g. https://github.com/Rahix/avr-hal/issues/573 is fixed), so we're mostly ready πŸ˜„

Rahix commented 1 month ago

No worries! Just ping me once the patch has hit nightly Rust, then I'll give it another shot :)

Thanks for all your compiler work!

Patryk27 commented 3 weeks ago

Alright, let's try now - the appropriate fix should've gone with https://github.com/rust-lang/rust/pull/132352.

Rahix commented 3 weeks ago

Updated to nightly-2024-11-03, but CI still reports the error: out of range branch target for some targets. Disabled fail-fast so we get the full list of failing targets - I checked, they are all failing on the same error.

Is the fix maybe not actually in this version I tried?

Patryk27 commented 3 weeks ago

Could you try 2024-11-05? The fix is there, but since it got merged on 2024-11-02, the 2024-11-03 build might not contain it just yet πŸ‘€

Either that or it's going to be a new kind of error, we'll see πŸ˜„

Rahix commented 3 weeks ago

Updated to 2024-11-05, still same targets failing... So I guess this is a new thing then? :sweat_smile:

Patryk27 commented 2 weeks ago

Looks this way πŸ˜… I'll try reproducing locally and report back.

Patryk27 commented 1 week ago

Status: reproduced!

$ cd ~/avr-hal/mcu/attiny-hal
$ RUSTFLAGS='--emit=llvm-ir' cargo +stage1 build --features attiny85 -Z build-std=core --target ../../avr-specs/avr-attiny85.json --release
$ llc -march=avr -mcpu=attiny85 -filetype=obj ~/avr-hal/target/avr-attiny85/release/deps/core-*.ll

... will crash, saying out of range branch target.

I've altered the error message in LLVM to print the offset it's trying to generate instruction for and it's pretty obvious something's off:

<unknown>:0: error: out of range branch target (expected an integer in the range -4096 to 4095, got 4488)
/* ... */
<unknown>:0: error: out of range branch target (expected an integer in the range -4096 to 4095, got 102544)
/* ... */
<unknown>:0: error: out of range branch target (expected an integer in the range -4096 to 4095, got 18446744073709481512)
/* ... */

I'll try narrowing the issue down 🫑

Patryk27 commented 5 days ago

Got it!

Reduced: https://gist.github.com/Patryk27/a68ed01eeb57b59c3025c395059f906d (and then llc -march=avr -mcpu=attiny85 -filetype=obj core.reduced.ll)

The issue boils down to too large jumps - basically, given:

foo:
  rjmp bar

# lots of code

bar:
  # something

... if there's too many opcodes between rjmp and the target label, we might not have enough space in the opcode's encoding to represent the offset (rjmp allows for 12 bits, for instance).

To avoid this issue, LLVM has a pass called branch relaxation that detects too large jumps and upgrades them into different instructions - in the case of AVR, that's where the backend might decide to emit jmp instead; it's one clock cycle more, but allows for a larger offset.

Everything's fine if the target AVR supports jmp - attiny85 does not, so the backend decides to stick with rjmp:

https://github.com/llvm/llvm-project/blob/a76609dd72743c0d678504042b00d434e6cebe1a/llvm/lib/Target/AVR/AVRInstrInfo.cpp#L568

... just to crash a moment later, in sort of a Β―_(ツ)_/Β― whaddya-want-me-to-do fashion.

Fortunately, not all hope is lost! - there's two ways out of this situation:

We didn't have to worry about those offsets, because in the past - up until https://github.com/llvm/llvm-project/commit/6859685a87ad093d60c8bed60b116143c0a684c7 aka https://github.com/llvm/llvm-project/pull/106722 - LLVM handled jumps by relying on relocations. Relocations allow to store larger offsets with the assumption that "the linker will somehow deal with them later", and it did: (most likely) by applying the wrapping trick.

tl;dr linker smart, linker can solve "large jumps" issue - but llvm no longer rely on linker for jump, llvm must be taught to emit long jumps on its own

I'll try preparing the patch in the upcoming days.

Rahix commented 5 days ago

Damn, nice work!

we can exploit the fact that AVR memory wraps, so instead of generating e.g. rjmp +5000, we can emit rjmp -3192 (cursed, but practical)

haha, i love it :laughing: cursed, indeed