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

Some functions in core::sync are annotated to only support targets with 8-bit atomic support #106845

Closed
tomerze opened this issue Jan 14, 2023 · 1 comment · Fixed by #106856
Closed

Comments

@tomerze
Copy link
Contributor

tomerze commented Jan 14, 2023

This issue was discussed at #106795
and at #106796 (comment)

Some functions in core::sync are defined to only compile if the target has 8-bit atomic support. While they should be compiled for all targets with atomic support.

This can cause unnecessary compilation issues when using a custom target json.

This can probably be solved by changing the definitions to

#[cfg(any(
    target_has_atomic = "8",
    target_has_atomic = "16",
    target_has_atomic = "32",
    target_has_atomic = "64",
    target_has_atomic = "128",
    target_has_atomic = "ptr"
))]

from

#[cfg(target_has_atomic = "8")]

like you can see at vadorovsky@041978b

Examples:

#[cfg(target_has_atomic = "8")]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
unsafe fn atomic_add<T: Copy>(dst: *mut T, val: T, order: Ordering) -> T {

#[cfg(target_has_atomic = "8")]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
unsafe fn atomic_compare_exchange<T: Copy>(

#[cfg(target_has_atomic = "8")]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
unsafe fn atomic_nand<T: Copy>(dst: *mut T, val: T, order: Ordering) -> T {

@vadorovsky @alessandrod

@vadorovsky
Copy link
Contributor

Thanks for creating the issue! I will submit a PR soon (I'm AFK now).

vadorovsky added a commit to vadorovsky/rust that referenced this issue Jan 17, 2023
Atomic operations for different widths (8-bit, 16-bit, 32-bit etc.) are
guarded by `target_has_atomic = "value"` symbol (i.e. `target_has_atomic
= "8"`) (and the other derivatives), but before this change, there was
no width-agnostic symbol indicating a general availability of atomic
operations.

This change introduces:

* `target_has_atomic_load_store` symbol when atomics for any integer
  width are supported by the target.
* `target_has_atomic` symbol when also CAS is supported.

Fixes rust-lang#106845

Signed-off-by: Michal Rostecki <[email protected]>
vadorovsky added a commit to vadorovsky/rust that referenced this issue Jan 18, 2023
Before this change, the following functions and macros were annotated
with `#[cfg(target_has_atomic = "8")]` or
`#[cfg(target_has_atomic_load_store = "8")]`:

* `atomic_int`
* `strongest_failure_ordering`
* `atomic_swap`
* `atomic_add`
* `atomic_sub`
* `atomic_compare_exchange`
* `atomic_compare_exchange_weak`
* `atomic_and`
* `atomic_nand`
* `atomic_or`
* `atomic_xor`
* `atomic_max`
* `atomic_min`
* `atomic_umax`
* `atomic_umin`

However, none of those functions and macros actually depend on 8-bit
width and they are needed for all atomic widths (16-bit, 32-bit, 64-bit
etc.). Some targets might not support 8-bit atomics (i.e. BPF, if we
would enable atomic CAS for it).

This change fixes that by removing the `"8"` argument from annotations,
which results in accepting the whole variety of widths.

Fixes rust-lang#106845

Signed-off-by: Michal Rostecki <[email protected]>
vadorovsky added a commit to vadorovsky/rust that referenced this issue Jan 25, 2023
Before this change, the following functions and macros were annotated
with `#[cfg(target_has_atomic = "8")]` or
`#[cfg(target_has_atomic_load_store = "8")]`:

* `atomic_int`
* `strongest_failure_ordering`
* `atomic_swap`
* `atomic_add`
* `atomic_sub`
* `atomic_compare_exchange`
* `atomic_compare_exchange_weak`
* `atomic_and`
* `atomic_nand`
* `atomic_or`
* `atomic_xor`
* `atomic_max`
* `atomic_min`
* `atomic_umax`
* `atomic_umin`

However, none of those functions and macros actually depend on 8-bit
width and they are needed for all atomic widths (16-bit, 32-bit, 64-bit
etc.). Some targets might not support 8-bit atomics (i.e. BPF, if we
would enable atomic CAS for it).

This change fixes that by removing the `"8"` argument from annotations,
which results in accepting the whole variety of widths.

Fixes rust-lang#106845
Fixes rust-lang#106795

Signed-off-by: Michal Rostecki <[email protected]>
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jan 27, 2023
… r=joshtriplett

core: Support variety of atomic widths in width-agnostic functions

Before this change, the following functions and macros were annotated with `#[cfg(target_has_atomic = "8")]` or
`#[cfg(target_has_atomic_load_store = "8")]`:

* `atomic_int`
* `strongest_failure_ordering`
* `atomic_swap`
* `atomic_add`
* `atomic_sub`
* `atomic_compare_exchange`
* `atomic_compare_exchange_weak`
* `atomic_and`
* `atomic_nand`
* `atomic_or`
* `atomic_xor`
* `atomic_max`
* `atomic_min`
* `atomic_umax`
* `atomic_umin`

However, none of those functions and macros actually depend on 8-bit width and they are needed for all atomic widths (16-bit, 32-bit, 64-bit etc.). Some targets might not support 8-bit atomics (i.e. BPF, if we would enable atomic CAS for it).

This change fixes that by removing the `"8"` argument from annotations, which results in accepting the whole variety of widths.

Fixes rust-lang#106845
Fixes rust-lang#106795

Signed-off-by: Michal Rostecki <[email protected]>
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jan 27, 2023
… r=joshtriplett

core: Support variety of atomic widths in width-agnostic functions

Before this change, the following functions and macros were annotated with `#[cfg(target_has_atomic = "8")]` or
`#[cfg(target_has_atomic_load_store = "8")]`:

* `atomic_int`
* `strongest_failure_ordering`
* `atomic_swap`
* `atomic_add`
* `atomic_sub`
* `atomic_compare_exchange`
* `atomic_compare_exchange_weak`
* `atomic_and`
* `atomic_nand`
* `atomic_or`
* `atomic_xor`
* `atomic_max`
* `atomic_min`
* `atomic_umax`
* `atomic_umin`

However, none of those functions and macros actually depend on 8-bit width and they are needed for all atomic widths (16-bit, 32-bit, 64-bit etc.). Some targets might not support 8-bit atomics (i.e. BPF, if we would enable atomic CAS for it).

This change fixes that by removing the `"8"` argument from annotations, which results in accepting the whole variety of widths.

Fixes rust-lang#106845
Fixes rust-lang#106795

Signed-off-by: Michal Rostecki <[email protected]>
@bors bors closed this as completed in 1cd7dbf Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants