Repository navigation
Mutating through addr_of produces LLVM IR with UB #111502
Description
As there is a write through a *const derived pointer, this code has UB under Stacked Borrows, but is fine under Tree Borrows,
pub fn first() -> bool {
let mut a = (0., true);
let b = (0., false);
let a_ptr_and_b = (core::ptr::addr_of_mut!(a), b);
let got = unsafe { second(a_ptr_and_b.0, a_ptr_and_b, true, true) };
return got;
}
unsafe fn second(
a_ptr: *mut (f32, bool),
a_ptr_and_b: (*mut (f32, bool), (f64, bool)),
t: bool,
t_copy: bool,
) -> bool {
let b_bool_ptr = core::ptr::addr_of!(a_ptr_and_b.1 .1) as *mut bool;
let t_or_t = t | t_copy;
let t_xor_t = t ^ t_copy;
(*b_bool_ptr) = t_or_t ^ t_xor_t;
let unused = a_ptr_and_b;
return a_ptr_and_b.1.1 == (*a_ptr).1;
}
pub fn main() {
println!("{}", first());
}-C opt-level=0 results in true (which Miri agrees and is the right result), but -C opt-level=3 miscompiles it to print false
% rustc -Zmir-opt-level=0 -C opt-level=0 repro.rs && ./repro
true
% rustc -Zmir-opt-level=0 -C opt-level=3 repro.rs && ./repro
falseInterestingly, if you remove unused in function second, the miscompilation goes away. -Zmir-opt-level=0 is required to prevent mir-opt from doing this.
Directly getting a *mut through addr_of_mut! in the first line of second also makes the bug go away.
rustc --version -v
rustc 1.71.0-nightly (2a8221dbd 2023-05-11)
binary: rustc
commit-hash: 2a8221dbdfd180a2d56d4b0089f4f3952d8c2bcd
commit-date: 2023-05-11
host: aarch64-apple-darwin
release: 1.71.0-nightly
LLVM version: 16.0.2
Activity
RalfJung commentedon May 12, 2023
If you mark
a_ptr_and_basmut, does that fix the behavior? (This should be required if you useaddr_of_mutinstead ofaddr_of.)cbeuw commentedon May 12, 2023
No, it doesn't change the behaviour (and there's an
unused_mutwarning)[-]Unused variable triggers LLVM miscompilation[/-][+]Mutating through addr_of triggers LLVM miscompilation[/+]RalfJung commentedon May 12, 2023
That is very strange, I was under the impression that we generate the same code for addr_of and addr_of_mut.
Making
*constand*mutequivalent for UB everwhere is an oft-requested feature, which is why TB accepts this code.tmiasko commentedon May 12, 2023
a_ptr_and_bparameter, which is passed indirectly, is marked as readonly byDeduceReadOnlypass (the pass assumes thatNonMutatingUseContext::AddressOfis non-mutating).RalfJung commentedon May 12, 2023
AFAIK it was never intended to be considered non-mutating. In fact we still track this Miri behavior as a bug: #56604.
tmiasko commentedon May 12, 2023
Sure, I am just describing why generated LLVM IR has undefined behavior.
RalfJung commentedon May 12, 2023
@rust-lang/wg-mir-opt @rust-lang/opsem do you know any other places where we assume one cannot mutate through an
addr_of!-created pointer? It was always (since #56604) my understanding that this should work and Stacked Borrows was unnecessarily pedantic here.[-]Mutating through addr_of triggers LLVM miscompilation[/-][+]Mutating through addr_of produces LLVM IR with UB[/+]digama0 commentedon May 12, 2023
While I want to be able to say "yes this is DB", doesn't this hit issues depending on how the pointer is obtained? If you do
let x = 0;and thenaddr_of!(x), I don't think you should be allowed to mutate that pointer. For certain kinds of places (ones derived from a path for which&mutwould be valid) it might be okay to derive a mutable pointer to the place, but even then you hit mutable borrow invalidation issues if you useaddr_of!(x)twice.Why not just use the mutability the user asked for? This is literally what
addr_of_mutis for.Note that in #56604 the issue was with two ways to obtain a pointer from a
&mut, so you definitely have mutable access in that case. When it's just a path you may or may not have mutable access to begin with, and the compiler knows which.RalfJung commentedon May 12, 2023
IMO it would be strange if
addr_of!(*mutable_ref)allowed mutation butaddr_of!(local)did not.I don't consider
letvslet muton local variables to be meaningful, and in fact we don't even preserve that information to MIR I think.digama0 commentedon May 12, 2023
I agree with this, but perhaps draw the opposite conclusion from it. :)
addr_of{_mut}!is the boundary between things the compiler can track and things it can't. When the compiler thinks something is immutable, we should really try to have the opsem respect that. Once it's a pointer I think it is okay to do mutability crimes because it's not helping anyone to be pedantic about it, but ifaddr_of_mut!(x)would not compile, then it seems really wrong foraddr_of!(x) as *mut u8to work (for mutation without UB).Put another way, if you want to make it legal to mutate
let x = FOO;bindings using raw pointers, that needs a larger quorum beyond just t-opsem, and I think t-lang will have some things to say about it.JakobDegen commentedon May 12, 2023
I'm quite sympathetic to Mario's point here, but this is still the wrong order to consider this in. We should turn off the sketchy optimization while we figure out if it's ok
RalfJung commentedon May 13, 2023
I find it funny that first there was a wave of complaints when Miri started making a difference between
*constand*mut(and this confusion still regularly comes up every few months), and now there is a wave of complaints when Miri does not make a difference between*constand*mut. 😂 Some of this honestly feels like post-hoc rationalization to me: we got used to how Miri works, now we don't want it to change any more.But anyway, that's a topic for rust-lang/unsafe-code-guidelines#257.
Auto merge of rust-lang#111517 - lukas-code:addr-of-mutate, r=tmiasko
Auto merge of #111517 - lukas-code:addr-of-mutate, r=tmiasko