Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove (more) CAS API from Atomic* types where not natively supported #54280

Merged
merged 1 commit into from
Sep 22, 2018

Conversation

japaric
Copy link
Member

@japaric japaric commented Sep 16, 2018

closes #54276

In PR #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 #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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 16, 2018
@japaric
Copy link
Member Author

japaric commented Sep 16, 2018

The way how I test if an API is natively supported by some target is by writing
a small crate like this:

#![no_std]

use core::sync::atomic::{AtomicUsize, Ordering};

#[no_mangle]
pub unsafe fn foo(x: &AtomicUsize) {
    x.fetch_add(1, Ordering::SeqCst);
}

And the looking at the disassembly

$ cargo objdump --target thumbv6m-none-eabi --lib --release -- -d
Disassembly of section .text.foo:
foo:
       0:       80 b5   push    {r7, lr}
       2:       bf f3 5f 8f     dmb     sy
       6:       01 21   movs    r1, #1
       8:       ff f7 fe ff     bl      #-4
       c:       bf f3 5f 8f     dmb     sy
      10:       80 bd   pop     {r7, pc}

bl #-4 means that this is calling some external function.

$ cargo nm --target thumbv6m-none-eabi --lib --release
foo-75644712abc60497.foo.e45asijs-cgu.0.rcgu.o:
         U __sync_fetch_and_add_4
00000000 T foo

nm reveals the name of the external function.

We could add tests that check that there are no undefined symbols in the output
object file when compiling a small crate like the one shown above, but this
requires the cooperation of tidy to check that a new test is added for each
newly added API.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 17, 2018

📌 Commit 8282896 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 17, 2018
pietroalbini added a commit to pietroalbini/rust that referenced this pull request 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
bors added a commit that referenced this pull request Sep 22, 2018
Rollup of 16 pull requests

Successful merges:

 - #53652 (define copy_within on slices)
 - #54261 (Make `dyn` a keyword in the 2018 edition)
 - #54280 (remove (more) CAS API from Atomic* types where not natively supported)
 - #54323 (rustbuild: drop color handling)
 - #54350 (Support specifying edition in doc test)
 - #54370 (Improve handling of type bounds in `bit_set.rs`.)
 - #54371 (add -Zui-testing to rustdoc)
 - #54374 (Make 'proc_macro::MultiSpan' public.)
 - #54402 (Use no_default_libraries for all NetBSD flavors)
 - #54409 (Detect `for _ in in bar {}` typo)
 - #54412 (add applicability to span_suggestion call)
 - #54413 (Add UI test for deref recursion limit printing twice)
 - #54415 (parser: Tweak function parameter parsing to avoid rollback on succesfull path)
 - #54420 (Compress `Liveness` data some more.)
 - #54422 (Simplify slice's first(_mut) and last(_mut) with get)
 - #54446 (Unify christianpoveda's emails)

Failed merges:

 - #54058 (Introduce the partition_dedup/by/by_key methods for slices)

r? @ghost
bors added a commit that referenced this pull request Sep 22, 2018
Rollup of 16 pull requests

Successful merges:

 - #53652 (define copy_within on slices)
 - #54261 (Make `dyn` a keyword in the 2018 edition)
 - #54280 (remove (more) CAS API from Atomic* types where not natively supported)
 - #54323 (rustbuild: drop color handling)
 - #54350 (Support specifying edition in doc test)
 - #54370 (Improve handling of type bounds in `bit_set.rs`.)
 - #54371 (add -Zui-testing to rustdoc)
 - #54374 (Make 'proc_macro::MultiSpan' public.)
 - #54402 (Use no_default_libraries for all NetBSD flavors)
 - #54409 (Detect `for _ in in bar {}` typo)
 - #54412 (add applicability to span_suggestion call)
 - #54413 (Add UI test for deref recursion limit printing twice)
 - #54415 (parser: Tweak function parameter parsing to avoid rollback on succesfull path)
 - #54420 (Compress `Liveness` data some more.)
 - #54422 (Simplify slice's first(_mut) and last(_mut) with get)
 - #54446 (Unify christianpoveda's emails)

Failed merges:

 - #54058 (Introduce the partition_dedup/by/by_key methods for slices)

r? @ghost
@bors
Copy link
Contributor

bors commented Sep 22, 2018

☔ The latest upstream changes (presumably #54457) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 22, 2018
@bors bors merged commit 8282896 into rust-lang:master Sep 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

compare_exchange_weak on AtomicUsize etc has inconsistent cfg-gating
4 participants