Skip to content

Add the missing inttoptr when we ptrtoint in ptr atomics #122917

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 15, 2024
Merged

Add the missing inttoptr when we ptrtoint in ptr atomics #122917

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions compiler/rustc_codegen_llvm/src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1136,12 +1136,12 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {
order: rustc_codegen_ssa::common::AtomicOrdering,
) -> &'ll Value {
// The only RMW operation that LLVM supports on pointers is compare-exchange.
if self.val_ty(src) == self.type_ptr()
&& op != rustc_codegen_ssa::common::AtomicRmwBinOp::AtomicXchg
{
let requires_cast_to_int = self.val_ty(src) == self.type_ptr()
&& op != rustc_codegen_ssa::common::AtomicRmwBinOp::AtomicXchg;
if requires_cast_to_int {
src = self.ptrtoint(src, self.type_isize());
}
unsafe {
let mut res = unsafe {
llvm::LLVMBuildAtomicRMW(
self.llbuilder,
AtomicRmwBinOp::from_generic(op),
Expand All @@ -1150,7 +1150,11 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {
AtomicOrdering::from_generic(order),
llvm::False, // SingleThreaded
)
};
if requires_cast_to_int {
res = self.inttoptr(res, self.type_ptr());
}
res
}

fn atomic_fence(
Expand Down
38 changes: 38 additions & 0 deletions tests/codegen/atomicptr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// LLVM does not support some atomic RMW operations on pointers, so inside codegen we lower those
// to integer atomics, surrounded by casts to and from integer type.
// This test ensures that we do the round-trip correctly for AtomicPtr::fetch_byte_add, and also
// ensures that we do not have such a round-trip for AtomicPtr::swap, because LLVM supports pointer
// arguments to `atomicrmw xchg`.

//@ compile-flags: -O -Cno-prepopulate-passes
#![crate_type = "lib"]

#![feature(strict_provenance)]
#![feature(strict_provenance_atomic_ptr)]

use std::sync::atomic::AtomicPtr;
use std::sync::atomic::Ordering::Relaxed;
use std::ptr::without_provenance_mut;

// Portability hack so that we can say [[USIZE]] instead of i64/i32/i16 for usize.
// CHECK: @helper([[USIZE:i[0-9]+]] noundef %_1)
#[no_mangle]
pub fn helper(_: usize) {}

// CHECK-LABEL: @atomicptr_fetch_byte_add
#[no_mangle]
pub fn atomicptr_fetch_byte_add(a: &AtomicPtr<u8>, v: usize) -> *mut u8 {
// CHECK: %[[INTPTR:.*]] = ptrtoint ptr %{{.*}} to [[USIZE]]
// CHECK-NEXT: %[[RET:.*]] = atomicrmw add ptr %{{.*}}, [[USIZE]] %[[INTPTR]]
// CHECK-NEXT: inttoptr [[USIZE]] %[[RET]] to ptr
Comment on lines +25 to +27
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be clearer (and not too fragile) if you match the whole thing end-to-end, i.e.

Suggested change
// CHECK: %[[INTPTR:.*]] = ptrtoint ptr %{{.*}} to [[USIZE]]
// CHECK-NEXT: %[[RET:.*]] = atomicrmw add ptr %{{.*}}, [[USIZE]] %[[INTPTR]]
// CHECK-NEXT: inttoptr [[USIZE]] %[[RET]] to ptr
// CHECK: %[[V_PTR:.*]] = inttoptr [[USIZE]] %v to ptr
// CHECK: %[[V_INT:.*]] = ptrtoint ptr %[[V_PTR]] to [[USIZE]]
// CHECK: %[[RET_INT:.*]] = atomicrmw add ptr %a, [[USIZE]] %[[V_INT]]
// CHECK: %[[RET_PTR:.*]] = inttoptr [[USIZE]] %[[RET]] to ptr
// CHECK: ret ptr %[[RET_PTR]]

(Could also match the argument names from the signature instead of hardcoding %a and %v, but I think it's fine, it's pretty unlikely that LLVM will start changing/dropping them in the future.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This then leads to the question--why do we roundtrip %v through inttoptr/ptrtoint?

I suppose this is because in MIR the atomic op takes two pointer arguments, but does it really make sense to add two pointers?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, adding two pointers doesn't make sense. The problem is that we're implementing an operation which takes a pointer operand and an integer operand. There's no atomic ptradd.

In terms of provenance, this is cursed if there are two pointer operands because which provenance is written back through the pointer? It's also cursed if we do this with integers, because then haven't we written an integer over a pointer, wiping the provenance from the memory? (I'm sort of hand-waving the kind of provenance semantics in Miri over LLVM IR which is itself a bit dubious)

Copy link
Contributor

@erikdesjardins erikdesjardins Mar 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that makes sense. I should've been more explicit--what I mean is, the relevant part of the MIR for your example currently looks like:

fn atomicptr_fetch_byte_add(_1: &AtomicPtr<u8>, _2: usize) -> *mut u8 {
  ...
  _4 = move _7 as *mut *mut u8 (PtrToPtr);
  _6 = _2 as *mut u8 (Transmute);
  _3 = atomic_xadd_relaxed::<*mut u8>(move _4, move _6) -> [return: bb1, unwind unreachable];

i.e. the intrinsic we're implementing here does take two pointer operands. Isn't that wrong? (Well, maybe not wrong but suboptimal.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what way would an intrinsic based on a pointer to an integer and and an integer operand be better? Or are you suggesting that we add an intrinsic fn atomic_ptradd_relaxed(*mut *mut T, usize)?

Copy link
Contributor

@erikdesjardins erikdesjardins Mar 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or are you suggesting that we add an intrinsic fn atomic_ptradd_relaxed(*mut *mut T, usize)?

Yes. (And the same for every atomic operation on pointers (xor, or, etc.) that only makes sense with an integer second operand.)

Given that the only sensible operation here is offsetting a pointer by an integer, wouldn't it be better for the intrinsic to match that?

(I guess this isn't so much a problem because we can just define atomic_xadd_relaxed::<*mut u8> to always use the provenance of the first argument, and I assume this is what Miri implements. Having to convert the offset to/from a pointer "just" makes the codegen more complex.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

always use the provenance of the first argument, and I assume this is what Miri implements.

Yes indeed.

But ideally we'd convince LLVM to provide an intrinsic that takes (*mut *mut T, usize); that is the only way to avoid pointer-integer transmutation.

Copy link
Contributor

@erikdesjardins erikdesjardins Mar 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, we need the LLVM change regardless, I don't mean to imply that this would solve the provenance issue. It would just remove some other unnecessary weirdness.

It doesn't need to be done as part of this PR though, I can do it separately.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @erikdesjardins that the Rust intrinsic should accept usize rather than pointer for the second arg.

a.fetch_byte_add(v, Relaxed)
}

// CHECK-LABEL: @atomicptr_swap
#[no_mangle]
pub fn atomicptr_swap(a: &AtomicPtr<u8>, ptr: *mut u8) -> *mut u8 {
// CHECK-NOT: ptrtoint
// CHECK: atomicrmw xchg ptr %{{.*}}, ptr %{{.*}} monotonic
// CHECK-NOT: inttoptr
a.swap(ptr, Relaxed)
}