compare_exchange_weak on AtomicUsize etc has inconsistent cfg-gating #54276
Comments
japaric
added a commit
to japaric/rust
that referenced
this issue
Sep 16, 2018
pietroalbini
added a commit
to pietroalbini/rust
that referenced
this issue
Sep 22, 2018
…richton remove (more) CAS API from Atomic* types where not natively supported closes rust-lang#54276 In PR rust-lang#51953 I made the Atomic* types available on targets like thumbv6m and msp430 with the intention of *only* exposing the load and store API on those types -- the rest of the API doesn't work on those targets because the are no native instructions to implement CAS loops. Unfortunately, it seems I didn't properly cfg away all the CAS API on those targets, as evidenced in rust-lang#54276. This PR amends the issue by removing the rest of the CAS API. This is technically a breaking change because *libraries* that were using this API and were being compiled for e.g. thumbv6m-none-eabi will stop compiling. However, using those libraries (before this change) in programs (binaries) would lead to linking errors when compiled for e.g. thumbv6m so this change effectively shifts a linker error in binaries to a compiler error in libraries. On a side note: extending the Atomic API is a bit error prone because of these non-cas targets. Unless the author of the change is aware of these targets and properly uses `#[cfg(atomic = "cas")]` they could end up exposing new CAS API on these targets. I can't think of a test to check that an API is not present on some target, but we could extend the `tidy` tool to check that *all* newly added atomic API has the `#[cfg(atomic = "cas")]` attribute unless it's whitelisted in `tidy` then the author of the change would have to verify if the API can be used on non-cas targets. In any case, I'd like to plug this hole ASAP. We can revisit testing in a follow-up issue / PR. r? @alexcrichton cc @mvirkkunen
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Compare-and-swap methods on the atomic types in
core::sync::atomicseem to be gated by#[cfg(target_has_atomic = "cas")]to only define them on targets that support CAS in hardware. Howevercompare_exchange_weakisn't gated at all. This looks like an oversight, if the intention is to never define those methods if the target doesn't actually support them.N.b. on at least the thumbv6m-none-eabi target,
compare_exchange_weakcompiles to calls to the same internal functions (__sync_val_compare_and_swap_4and similar, presumably in compiler-rt or something) ascompare_exchange.The text was updated successfully, but these errors were encountered: