-
Notifications
You must be signed in to change notification settings - Fork 13.6k
atomicrmw on pointers: move integer-pointer cast hacks into backend #144192
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
Conversation
rustbot has assigned @Mark-Simulacrum. Use |
This comment has been minimized.
This comment has been minimized.
Some changes occurred in compiler/rustc_codegen_gcc |
@antoyo, @GuillaumeGomez I took a wild guess with those GCC changes, please take a look. @bjorn3 this seems to somehow break cranelift, which is surprising since I thought |
3494e9c
to
a5a87d3
Compare
Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 |
I took a guess at what I have to do in cranelift, I hope it makes sense. |
This comment has been minimized.
This comment has been minimized.
577b7f8
to
c8c38b3
Compare
This comment has been minimized.
This comment has been minimized.
1eb3567
to
bd0cb3c
Compare
This PR modifies cc @jieyouxu |
@@ -35,51 +35,62 @@ impl<T: ?Sized> Copy for *mut T {} | |||
impl ConstParamTy for AtomicOrdering {} | |||
|
|||
#[cfg(target_has_atomic = "8")] | |||
#[unsafe(no_mangle)] // let's make sure we actually generate a symbol to check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test might actually have been ineffective ever since we gained that heuristic that automatically marks small functions as #[inline]
.
☔ The latest upstream changes (presumably #144249) made this pull request unmergeable. Please resolve the merge conflicts. |
r? @nikic |
@bors r+ |
atomicrmw on pointers: move integer-pointer cast hacks into backend Conceptually, we want to have atomic operations on pointers of the form `fn atomic_add(ptr: *mut T, offset: usize, ...)`. However, LLVM does not directly support such operations (llvm/llvm-project#120837), so we have to cast the `offset` to a pointer somewhere. This PR moves that hack into the LLVM backend, so that the standard library, intrinsic, and Miri all work with the conceptual operation we actually want. Hopefully, one day LLVM will gain a way to represent these operations without integer-pointer casts, and then the hack will disappear entirely. Cc `@nikic` -- this is the best we can do right now, right? Fixes rust-lang#134617
atomicrmw on pointers: move integer-pointer cast hacks into backend Conceptually, we want to have atomic operations on pointers of the form `fn atomic_add(ptr: *mut T, offset: usize, ...)`. However, LLVM does not directly support such operations (llvm/llvm-project#120837), so we have to cast the `offset` to a pointer somewhere. This PR moves that hack into the LLVM backend, so that the standard library, intrinsic, and Miri all work with the conceptual operation we actually want. Hopefully, one day LLVM will gain a way to represent these operations without integer-pointer casts, and then the hack will disappear entirely. Cc ``@nikic`` -- this is the best we can do right now, right? Fixes rust-lang#134617
Rollup of 7 pull requests Successful merges: - #144039 (Use `tcx.short_string()` in more diagnostics) - #144192 (atomicrmw on pointers: move integer-pointer cast hacks into backend) - #144823 (coverage: Extract HIR-related helper code out of the main module) - #144987 (Enable f16 and f128 on targets that were fixed in LLVM21) - #145001 (regression test for intrinsics may not inline properly on pclmulqdq) - #145080 (Escape diff strings in MIR dataflow graphviz) - #145083 (Fix cross-compilation of Cargo) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 8 pull requests Successful merges: - #139451 (Add `target_env = "macabi"` and `target_env = "sim"`) - #144039 (Use `tcx.short_string()` in more diagnostics) - #144192 (atomicrmw on pointers: move integer-pointer cast hacks into backend) - #144545 (In rustc_pattern_analysis, put `true` witnesses before `false` witnesses) - #144579 (Implement declarative (`macro_rules!`) attribute macros (RFC 3697)) - #144649 (Account for bare tuples and `Pin` methods in field searching logic) - #144775 (more strongly dissuade use of `skip_binder`) - #144987 (Enable f16 and f128 on targets that were fixed in LLVM21) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #144192 - RalfJung:atomicrmw-ptr, r=nikic atomicrmw on pointers: move integer-pointer cast hacks into backend Conceptually, we want to have atomic operations on pointers of the form `fn atomic_add(ptr: *mut T, offset: usize, ...)`. However, LLVM does not directly support such operations (llvm/llvm-project#120837), so we have to cast the `offset` to a pointer somewhere. This PR moves that hack into the LLVM backend, so that the standard library, intrinsic, and Miri all work with the conceptual operation we actually want. Hopefully, one day LLVM will gain a way to represent these operations without integer-pointer casts, and then the hack will disappear entirely. Cc ```@nikic``` -- this is the best we can do right now, right? Fixes #134617
Rollup of 8 pull requests Successful merges: - rust-lang/rust#139451 (Add `target_env = "macabi"` and `target_env = "sim"`) - rust-lang/rust#144039 (Use `tcx.short_string()` in more diagnostics) - rust-lang/rust#144192 (atomicrmw on pointers: move integer-pointer cast hacks into backend) - rust-lang/rust#144545 (In rustc_pattern_analysis, put `true` witnesses before `false` witnesses) - rust-lang/rust#144579 (Implement declarative (`macro_rules!`) attribute macros (RFC 3697)) - rust-lang/rust#144649 (Account for bare tuples and `Pin` methods in field searching logic) - rust-lang/rust#144775 (more strongly dissuade use of `skip_binder`) - rust-lang/rust#144987 (Enable f16 and f128 on targets that were fixed in LLVM21) r? `@ghost` `@rustbot` modify labels: rollup
Culprit PR: rust-lang/rust#144192 The upstream PR adds a new generic argument `U` for some atomic intrinsics, so our tests need to include those. Solely changing the tests to infer the generic argument makes compilation succeed, but [this `atomic_xor` harness](https://github.com/model-checking/kani/blob/41849d25092483d1dd92d472cc8c457908903927/tests/kani/Intrinsics/Atomic/Stable/AtomicPtr/main.rs#L70) fails with this error: ``` thread 'rustc' (1068557) panicked at cprover_bindings/src/goto_program/expr.rs:1147:9: BinaryOperation Expression does not typecheck Bitor Expr { value: Dereference(Expr { value: Symbol { identifier: "_RINvNtNtCslIVxtpUQXYK_4core4sync6atomic9atomic_orOxjECs63P9ci4LCSm_4main::1::var_1::dst" }, typ: Pointer { typ: Pointer { typ: Signedbv { width: 64 } } }, location: None, size_of_annotation: None }), typ: Pointer { typ: Signedbv { width: 64 } }, location: None, size_of_annotation: None } Expr { value: Symbol { identifier: "_RINvNtNtCslIVxtpUQXYK_4core4sync6atomic9atomic_orOxjECs63P9ci4LCSm_4main::1::var_2::val" }, typ: CInteger(SizeT), location: None, size_of_annotation: None } note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace Kani unexpectedly panicked during compilation. Please file an issue here: https://github.com/model-checking/kani/issues/new?labels=bug&template=bug_report.md [Kani] current codegen item: codegen_function: std::sync::atomic::atomic_or::<*mut i64, usize> _RINvNtNtCslIVxtpUQXYK_4core4sync6atomic9atomic_orOxjECs63P9ci4LCSm_4main [Kani] current codegen location: Loc { file: "/Users/cmzech/.rustup/toolchains/nightly-2025-08-10-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/sync/atomic.rs", function: None, start_line: 4183, start_col: Some(1), end_line: 4183, end_col: Some(81), pragmas: [] } ``` This happens because `codegen_atomic_binop` currently handles the case where `T` is a pointer by checking if `var2` is a pointer, then casting `var1` and `var2` to `size_t`: https://github.com/model-checking/kani/blob/41849d25092483d1dd92d472cc8c457908903927/kani-compiler/src/codegen_cprover_gotoc/codegen/intrinsic.rs#L241-L245 But rust-lang/rust#144192 makes this change to `fetch_xor`: ```diff diff --git a/library/core/src/sync/atomic.rs b/library/core/src/sync/atomic.rs index 546f3d91a80..44a6895f90a 100644 --- a/library/core/src/sync/atomic.rs +++ b/library/core/src/sync/atomic.rs pub fn fetch_xor(&self, val: usize, order: Ordering) -> *mut T { // SAFETY: data races are prevented by atomic intrinsics. - unsafe { atomic_xor(self.p.get(), core::ptr::without_provenance_mut(val), order).cast() } + unsafe { atomic_xor(self.p.get(), val, order).cast() } } ``` since `var2` is no longer a pointer, we hit the else case, and thus the solution from #3047 no longer takes effect. The solution is to instead check if `var1` is a pointer. We also remove the cast of `var2` to a size_t, since per the new documentation: > `U` must be the same as `T` if that is an integer type, or `usize` if `T` is a pointer type. `U` is guaranteed to be a `usize` if `T` is a pointer, and we [codegen usizes as `size_t`s already](https://github.com/model-checking/kani/blob/41849d25092483d1dd92d472cc8c457908903927/kani-compiler/src/codegen_cprover_gotoc/codegen/typ.rs#L713 ). Resolves #4284 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.
atomicrmw on pointers: move integer-pointer cast hacks into backend Conceptually, we want to have atomic operations on pointers of the form `fn atomic_add(ptr: *mut T, offset: usize, ...)`. However, LLVM does not directly support such operations (llvm/llvm-project#120837), so we have to cast the `offset` to a pointer somewhere. This PR moves that hack into the LLVM backend, so that the standard library, intrinsic, and Miri all work with the conceptual operation we actually want. Hopefully, one day LLVM will gain a way to represent these operations without integer-pointer casts, and then the hack will disappear entirely. Cc ```@nikic``` -- this is the best we can do right now, right? Fixes rust-lang#134617
Rollup of 8 pull requests Successful merges: - rust-lang#139451 (Add `target_env = "macabi"` and `target_env = "sim"`) - rust-lang#144039 (Use `tcx.short_string()` in more diagnostics) - rust-lang#144192 (atomicrmw on pointers: move integer-pointer cast hacks into backend) - rust-lang#144545 (In rustc_pattern_analysis, put `true` witnesses before `false` witnesses) - rust-lang#144579 (Implement declarative (`macro_rules!`) attribute macros (RFC 3697)) - rust-lang#144649 (Account for bare tuples and `Pin` methods in field searching logic) - rust-lang#144775 (more strongly dissuade use of `skip_binder`) - rust-lang#144987 (Enable f16 and f128 on targets that were fixed in LLVM21) r? `@ghost` `@rustbot` modify labels: rollup
Conceptually, we want to have atomic operations on pointers of the form
fn atomic_add(ptr: *mut T, offset: usize, ...)
. However, LLVM does not directly support such operations (llvm/llvm-project#120837), so we have to cast theoffset
to a pointer somewhere.This PR moves that hack into the LLVM backend, so that the standard library, intrinsic, and Miri all work with the conceptual operation we actually want. Hopefully, one day LLVM will gain a way to represent these operations without integer-pointer casts, and then the hack will disappear entirely.
Cc @nikic -- this is the best we can do right now, right?
Fixes #134617