Potentially-observable store gets elided: asm block does not act as a compiler fence

[RalfJung]

I tried this code:

#![feature(atomic_from_mut)]

use std::arch::asm;
use std::sync::atomic::*;

unsafe extern "C" {
    fn bar();
}
#[unsafe(no_mangle)]
pub unsafe fn foo(y: *mut *mut i32) {
    let mut val: i32 = 0;
    let x: *mut i32 = &mut val;

    *x = 42; 
    asm!(""); // compiler_fence(Ordering::Release);
    AtomicPtr::from_mut(&mut *y).store(x, Ordering::Relaxed);
    loop {}
}

I would have expected the generated code to still contain the store of 42 in x, since the empty asm block must be considered to be a release fence by LLVM -- and then some other thread might wait for the value of y to change, and read through that pointer.

However, the resulting LLVM IR after optimizations is (godbolt: https://rust.godbolt.org/z/r86soTWaa):

define void @foo(ptr nocapture noundef writeonly %y) unnamed_addr {
start:
  %val = alloca [4 x i8], align 4
  call void @llvm.lifetime.start.p0(i64 4, ptr nonnull %val)
  tail call void asm sideeffect alignstack inteldialect "", "~{dirflag},~{fpsr},~{flags},~{memory}"() #2
  store atomic ptr %val, ptr %y monotonic, align 8
  br label %bb2

bb2:
  br label %bb2
}

If I replace the asm block by an actual fence or even just a compiler_fence, the result looks as expected:

define void @foo(ptr nocapture noundef writeonly %y) unnamed_addr {
start:
  %val = alloca [4 x i8], align 4
  call void @llvm.lifetime.start.p0(i64 4, ptr nonnull %val)
  store i32 42, ptr %val, align 4
  fence syncscope("singlethread") release
  store atomic ptr %val, ptr %y monotonic, align 8
  br label %bb1

bb1:
  br label %bb1
}

This is based on an example by @Amanieu.

@nikic this is a bug, right? Surely an empty asm block with all clobbers must have at least all the effects of an arbitrary compiler fence?

@fg-cfh points out that the LLVM documentation for the memory clobber just says that the asm block can write arbitrary memory, but does not mention reading memory. The corresponding docs for GCC also mention reading. I assume this can only be an oversight, since surely in general asm! blocks are allowed to read memory they have access to.

However, I think this problem here is more subtle: the asm block couldn't read from *x after all, since at the point the asm block runs, neither x nor val have been escaped. The issue is that reading and writing are not the only things one can do with memory. We have to also consider "synchronization effects", such as release/acquire fences, which are not associated with a memory access at all -- they change some other part of global state which records which memory events the next relaxed store can "publish" to other threads (and which events the next acquire fences "receives" into the current thread).

Cc @rust-lang/opsem

[RalfJung]

Ah, I should have read this comment more carefully... if I hide the compiler_fence inside a function, the same issue occurs: https://rust.godbolt.org/z/WvTsjW4vv. So this is indeed an instance of llvm/llvm-project#64188.

However, given that LLVM can apparently correctly recognize a fence that is literally visible directly in the source code, it seems reasonable to expect the same with an asm block directly in the source code. I understand doing this for all unknown function calls has significant perf impact (though prioritizing performance over correctness still irks me), but doing it specifically only for asm blocks should be fine -- right?

[nikic]

I think this is basically the llvm/llvm-project#64188 class of problem? That is, "not escaped before" reasoning is incompatible with the memory model.

[RalfJung]

I think this is basically the llvm/llvm-project#64188 class of problem? That is, "not escaped before" reasoning is incompatible with the memory model.

Yeah, I thought it wasn't because it worked fine with the explicit fence, but I didn't realize LLVM treats explicit fences and fences "hidden" inside functions differently.

But then, couldn't it also treat explicit asm blocks like explicit fences, at least?

[nikic]

Sure (in the sense that it's technically possible...) Is the motivation here that people may use asm blocks specifically for fence instructions?

[RalfJung]

Yeah, there are two situations where people would do that:

• The Linux kernel folks are doing it as part of implementing the LKMM, so recognizing these asm blocks would reduce the chance of kernel miscompilations caused by A miscompilation bug in LICMPass (concurrency) llvm/llvm-project#64188.
• Our plans for supporting interactions with DMA devices involve (a) making volatile accesses also relaxed atomic and (b) telling people to use asm blocks as fences between the DMA accesses and the volatile access that tells the device that the data in the DMA zone is ready (and similar for the other direction, when receiving data via DMA). Some hardware apparently doesn't guarantee that a "normal" fence suffices for that.

[Amanieu]

An asm block is treated the same way as a function call.

[bjorn3]

I don't get why this store must be preserved. It is to a memory ___location that isn't escaping and as such there should be no way at all that any other code can observe it. Also wouldn't a rule that requires it to be preserved break even optimizations as basic as mem2reg (which rustc heavily depends on to clean up the bad LLVM IR it produces)?

[RalfJung]

@bjorn3 Assume *y is null before we start. Imagine a concurrent thread (or, for the compiler_fence variant, a signal handler which gets triggered after the AtomicPtr::store) that loads from y, and then if it is non-null loads from that, and prints the result:

loop {
  let ptr = AtomicPtr::from_mut(&mut *y).load(Ordering::Acquire);
  if !ptr.is_null() {
    println!("{}", ptr.read()); // will print 42
    break
  }
}

If the store does not get preserved, this will not print 42 any more.

[bjorn3]

I missed the .store(x) when I was searching for uses of val and x. With the .store(x) it makes sense to me that this is an invalid optimization.