Fix armv4t- and armv5te- bare metal targets

[thejpster]

These two targets currently force on the LLVM feature +atomics-32. LLVM doesn't appear to actually be able to emit 32-bit load/store atomics for these targets despite this feature, and emits calls to a shim function called __sync_lock_test_and_set_4, which nothing in the Rust standard library supplies.

See #t-compiler/arm > __sync_lock_test_and_set_4 on Armv5TE for more details.

Experimenting with clang and gcc (as logged in that zulip thread) shows that C code cannot do atomic load/stores on that architecture either (at least, not without a library call inserted).

So, the safest thing to do is probably turn off +atomics-32 for these two Tier 3 targets.

I asked @Lokathor and he said he didn't even use atomics on the armv4t-none-eabi/thumbv4t-none-eabi target he maintains.

I was unable to reach @QuinnPainter for comment for armv5te-none-eabi/thumbv5te-none-eabi.

The second commit renames the base target spec spec::base::thumb to spec::base::arm_none and changes armv4t-none-eabi/thumbv4t-none-eabi and armv5te-none-eabi/thumbv5te-none-eabi to use it. This harmonises the frame-pointer and linker options across the bare-metal Arm EABI and EABIHF targets.

You could make an argument for harmonising armv7a-none-*, armv7r-none* and armv8r-none-* as well, but that can be another PR.

[rustbot]

Some changes occurred in src/doc/rustc/src/platform-support

cc @Noratrieb

These commits modify compiler targets.
(See the Target Tier Policy.)

[rustbot]

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[taiki-e]

+atomics-32 was required to fix build error in compiler-builtins (#100619).

__sync* builtin is emitted for non-relaxed atomic load/store, so +atomics-32 is meaningful for relaxed atomic load/store.

If you don't want to pass +atomics-32, AFAIK the correct approach is to completely disable atomics (by max_atomic_width: Some(0) ¹), similar to the MIPS-I target (mipsel-sony-psx), which does not support the SYNC instruction.

Footnotes

1. The default (max_atomic_width: None) means that the target having pointer width atomics. ↩

[Lokathor]

I'm leaving a formal comment of approval to this general idea as the armv4t maintainer, and I'll leave the exact details of the implementation up to the rest of you.

View changes since this review

[thejpster]

__sync* builtin is emitted for non-relaxed atomic load/store, so +atomics-32 is meaningful for relaxed atomic load/store.

Oh! Yes, I see now that relaxed loads do work.

Do we want to support core::sync::atomic where only Ordering::Relaxed works? It's awfully confusing.

I guess we have two options:

• Add +atomics-32 back in and document that only Ordering::Relaxed is expected to work unless the appropriate LLVM library functions are added (which we should document)
• Carry on and remove +atomics-32 and let people use https://crates.io/crates/portable-atomic to get full atomic support.

[QuinnPainter]

Sorry, I must have missed your request earlier.

Similar to Lokathor, I haven't been using atomics on armv5te.
To me, the second solution (disabling atomics) makes more sense than having a confusing partial implementation.

Either way, the docs for core::sync::atomic should probably be updated. Right now they indicate that armv5te can use any atomic load/store.

[thejpster]

Right now they indicate that armv5te can use any atomic load/store.

Where do we document that?

[QuinnPainter]

https://doc.rust-lang.org/core/sync/atomic/, under the "Portability" section:

ARM platforms like armv5te that aren’t for Linux only provide load and store operations, and do not support Compare and Swap (CAS) operations, such as swap, fetch_add, etc. Additionally on Linux, these CAS operations are implemented via operating system support, which may come with a performance penalty.

[thejpster]

Given that documentation (which I had missed), I think I'm inclined to leave in the relaxed load/store support. It's better than nothing? Let me close this and open another PR that just does the docs updates, because those should be uncontroversial. We can come back to dropping atomic-32 later if desired.

Edit: Nope.

[thejpster]

Oh, no.

@taiki-e - the load/store is only lowered to a plain LDR/STR when optimisations are on. When building for the debug profile, you get a call to core::sync::atomic::atomic_load::<u32>, which does a run-time lookup on the ordering argument. And that means the function always contains both the LDR version and the library call verison.

lib.rs:

#![no_std]

use core::sync::atomic;

pub static VALUE: atomic::AtomicU32 = atomic::AtomicU32::new(0);

pub fn example() -> u32 {
        VALUE.load(atomic::Ordering::Relaxed)
}

pub fn example2() -> u32 {
        VALUE.load(atomic::Ordering::Acquire)
}

rustc +source --target=armv4t-none-eabi --emit asm src/lib.rs --crate-type rlib -o - | rustfilt gives:

lib::example:
        .fnstart
        .save   {r11, lr}
        push    {r11, lr}
        .setfp  r11, sp
        mov     r11, sp
        ldr     r0, .LCPI1_0
        mov     r1, #0
        bl      <core::sync::atomic::AtomicU32>::load
        pop     {r11, lr}
        bx      lr
// ... stuff snipped for brevity
core::sync::atomic::atomic_load::<u32>:
        .fnstart
        .save   {r11, lr}
        push    {r11, lr}
        .setfp  r11, sp
        mov     r11, sp
        .pad    #16
        sub     sp, sp, #16
        str     r0, [sp, #4]
        and     r0, r1, #255
        str     r0, [sp, #8]
        ldr     r1, [sp, #8]
        adr     r0, .LJTI0_0
        ldr     r0, [r0, r1, lsl #2]
        mov     pc, r0
        .p2align        2
.LJTI0_0:
        .long   .LBB0_4
        .long   .LBB0_5
        .long   .LBB0_6
        .long   .LBB0_7
        .long   .LBB0_8
        .inst   0xe7ffdefe
.LBB0_4:
        ldr     r0, [sp, #4]
        ldr     r0, [r0]
        str     r0, [r11, #-4]
        b       .LBB0_9
.LBB0_5:
        ldr     r0, .LCPI0_2
        ldr     r2, .LCPI0_3
        mov     r1, #81
        bl      core::panicking::panic_fmt
.LBB0_6:
        ldr     r0, [sp, #4]
        mov     r2, #0
        mov     r1, r2
        bl      __sync_val_compare_and_swap_4
        str     r0, [r11, #-4]
        b       .LBB0_9
.LBB0_7:
        ldr     r0, .LCPI0_0
        ldr     r2, .LCPI0_1
        mov     r1, #99
        bl      core::panicking::panic_fmt
.LBB0_8:
        ldr     r0, [sp, #4]
        mov     r2, #0
        mov     r1, r2
        bl      __sync_val_compare_and_swap_4
        str     r0, [r11, #-4]
        b       .LBB0_9
.LBB0_9:
        ldr     r0, [r11, #-4]
        mov     sp, r11
        pop     {r11, lr}
        bx      lr

[fee1-dead]

Not familiar with this, sorry..

@rustbot reroll

[taiki-e]

the load/store is only lowered to a plain LDR/STR when optimisations are on. When building for the debug profile, you get a call to core::sync::atomic::atomic_load::<u32>, which does a run-time lookup on the ordering argument. And that means the function always contains both the LDR version and the library call verison.

Ah, that's right. I had mentioned that behavior before (#101300 (comment)), but I had forgotten.

I think very few people actually use opt-level=0 in projects targeting embedded systems), I now agree that disabling atomics is the more preferred behavior for these targets.

[thejpster]

I think very few people actually use opt-level=0 in projects targeting embedded systems

For the build you put on the board, sure. But I do a lot of debug profile builds for testing the code compiles in CI, and the default debug profile is opt-level=0. Also when it’s running unit tests QEMU I leave it in debug profile.

I now agree that disabling atomics is the more preferred behavior for these targets

I don’t love it, but I think it’s the least worst solution

[davidtwco]

r? @davidtwco

I'll give this a read through and ask internally before getting back to you on the PR.

[davidtwco]

@bors r+

[bors]

📌 Commit 120a464 has been approved by davidtwco

It is now in the queue for this repository.

[matthiaskrgr]

@bors rollup=iffy