make Atomic*::min/max work on armv5te #52586
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
|
cc @infinity0 |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Probably #52493 |
nagisa
reviewed
Jul 21, 2018
| #[cfg(target_arch = "arm5vte")] | ||
| #[inline(always)] | ||
| fn inner(&self, val: $int_type, order: Ordering) -> $int_type { | ||
| self.fetch_update(|v| Some(v.max(val)), order, order).unwrap() |
There was a problem hiding this comment.
You need to specially handle the order and derive correct orders for fetch_order here.
Namely, fetch_order of fetch_update does not permit AcqRel or Release as an ordering here, but the fetch_max permits all orderings.
src/libcore/sync/atomic.rs
Outdated
| #[cfg(target_arch = "arm5vte")] | ||
| #[inline(always)] | ||
| fn inner(&self, val: $int_type, order: Ordering) -> $int_type { | ||
| self.fetch_update(|v| Some(v.min(val)), order, order).unwrap() |
|
So, here’s a question now. Supposedly we are exposing Atomics only to targets that support them in hardware. ARMv5 does not have any native hardware support for atomics. Why are the types exposed then? |
|
I'm a bit puzzled: If armv5te doesn't support atomics in hardware, how do the tests work at all? How can we extend the tests to ensure this works? |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
I have no idea. Even the most trivial
|
"armv5te" is not a value that the built-in target_arch cfg supports. target_arch has value "arm" for the $ rustc --print cfg --target armv5te-unknown-linux-gnueabi
debug_assertions
target_arch="arm"
target_endian="little"
target_env="gnu"
target_family="unix"
target_has_atomic="16"
target_has_atomic="32"
target_has_atomic="8"
target_has_atomic="cas"
target_has_atomic="ptr"
target_os="linux"
target_pointer_width="32"
target_thread_local
target_vendor="unknown"
unixAnd these are the cfg values for $ rustc --print cfg --target arm-unknown-linux-gnueabi
debug_assertions
target_arch="arm"
target_endian="little"
target_env="gnu"
target_family="unix"
target_has_atomic="16"
target_has_atomic="32"
target_has_atomic="64"
target_has_atomic="8"
target_has_atomic="cas"
target_has_atomic="ptr"
target_os="linux"
target_pointer_width="32"
target_thread_local
target_vendor="unknown"
unixRight now, there's no way built-in way to differentiate the armv5 from the armv6 subarchitecture. |
kennytm
added
S-waiting-on-author
|
@japaric so the correct way to solve this would be to change the |
|
I think that is the correct approach, yes. |
We are exposing atomics only to targets that support them as a whole. On its own, ARMv5 doesn't have the instructions required for a CAS loop, but Linux provides kernel helpers (*) to implement CAS loops on that architecture. So "ARMv5 Linux" having (*) The compiler-builtins crate exposes the Linux kernel helpers as the symbols that LLVM emits calls of when lowering e.g. intrinsics::atomic_xchg.
I don't know what the problem that needs solving is; I was simply commenting that To be able to conditionally compile different code for ARMv5 you would probably need to a "v6" target feature (For an example, see #52120 which adds the "mclas" target feature) and use something like |
Care to point where exactly? |
|
So the right fix would be to add the necessary functions there. |
|
@nagisa What functions are necessary besides |
|
r? @nagisa |
|
@llogiq LLVM calls the atomic_rmw!(__sync_fetch_and_max_1, i8, |a: i8, b: i8| if a > b { a } else { b });
atomic_rmw!(__sync_fetch_and_max_2, i16, |a: i16, b: i16| if a > b { a } else { b });
atomic_rmw!(__sync_fetch_and_max_4, i32, |a: i32, b: i32| if a > b { a } else { b });
atomic_rmw!(__sync_fetch_and_umax_1, u8, |a: u8, b: u8| if a > b { a } else { b });
atomic_rmw!(__sync_fetch_and_umax_2, u16, |a: u16, b: u16| if a > b { a } else { b });
atomic_rmw!(__sync_fetch_and_umax_4, u32, |a: u32, b: u32| if a > b { a } else { b });
atomic_rmw!(__sync_fetch_and_min_1, i8, |a: i8, b: i8| if a < b { a } else { b });
atomic_rmw!(__sync_fetch_and_min_2, i16, |a: i16, b: i16| if a < b { a } else { b });
atomic_rmw!(__sync_fetch_and_min_4, i32, |a: i32, b: i32| if a < b { a } else { b });
atomic_rmw!(__sync_fetch_and_umin_1, u8, |a: u8, b: u8| if a < b { a } else { b });
atomic_rmw!(__sync_fetch_and_umin_2, u16, |a: u16, b: u16| if a < b { a } else { b });
atomic_rmw!(__sync_fetch_and_umin_4, u32, |a: u32, b: u32| if a < b { a } else { b }); |
|
Maybe LLVM tries to generate the wrong instructions? Something breaks, but we're in the dark what it is. Maybe we could look into the assembly. |
|
Well below is what gets generated for atomic_max_usize:
.fnstart
.save {r11, lr}
push {r11, lr}
.pad #8
sub sp, sp, #8
ldr r0, [r0]
ldr r1, [r1]
str r0, [sp, #4]
add r0, sp, #4
bl __sync_fetch_and_umax_4
add sp, sp, #8
pop {r11, pc}The genrated code is very similar for i16 (calls |
|
That doesn't look very atomic to me, but I must confess that my ARM assembly knowledge is rusty at the moment. |
TimNN
added
A-allocators
|
Ping from triage, @llogiq! What is the status of this? Is this blocked on some changes to the compiler built ins? |
|
I'm afraid I'm out of my depth here. I thought I could circumvent the problem, but it appears that I'm not really understanding the root cause, and I have no ARM5 hardware at home to test on. Closing this. |
|
Well we still have 40 failing tests on armv5te - and any rust code that depends on it won't compile on that platform, @nagisa any ideas? |
|
@infinity0 maybe it would help to compile a similar code snippet with GCC and Clang and compare the resulting assembly? |
|
@infinity0 I have no idea why My susipicion is that they are simply not exported for some reason. |
|
@nagisa The build.rs for compiler_builtins is very specific and only includes the sync_and_fetch compiler-rt assembly implementations when being built on arm7, is that the problem? |
|
Could be. As pointed out in comments above, there’s a pure-rust implementation here as well, but it is likely that those are not exported for some reason. |
|
I'm very sketchy on my binary knowledge but I thought "exporting" was only for shared library symbols. In any case, building rust-lang/compiler-builtins@5b05a98 with whereas the same command If anyone has any ideas on what to try please let me know, I have access to both armv5 and armv7 targets via Debian's porting machines. |
|
The failing tests may be due to a Debian-specific patch that I forgot about - it was given to me by someone more familiar with architecture-specific porting issues. The ongoing discussion is here: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=906520 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
r? @kennytm
This fixes #52586