This looks logical, but it's been a while since I read up on memory orderings.

@RalfJung want to take a look?

Good catch, the example indeed seems wrong. However, the fence is not defined in terms of "reordering", it is defined in terms of happens-before and other relationships, so that's also what the comment should refer to.

Cc @rust-lang/opsem @chorman0773

The regular fence examples use the synchronizes-with term in particular, I'd recommend the same term here.

https://doc.rust-lang.org/nightly/core/sync/atomic/fn.fence.html#examples

Iff the signal_handler ABI of the platform implies a logical compiler_fence(Acquire) when it starts (which isn't an unreasonable guarantee to have), then the current example would be acceptable.

But I agree that the example would be better written with an explicit acquire edge, avoiding implying logic of “I observed the (relaxed) flag write after the fence, therefore this is okay.”¹

Iff the signal_handler ABI of the platform implies a logical compiler_fence(Acquire) when it starts (which isn't an unreasonable guarantee to have), then the current example would be acceptable.

I don't see how that would fix this example. If we assume an implicit acquire at the start of the signal handler here:

use std::sync::atomic::{AtomicBool, AtomicUsize};
use std::sync::atomic::Ordering;
use std::sync::atomic::compiler_fence;

static IMPORTANT_VARIABLE: AtomicUsize = AtomicUsize::new(0);
static IS_READY: AtomicBool = AtomicBool::new(false);

fn main() {
    IMPORTANT_VARIABLE.store(42, Ordering::Relaxed);
    // prevent earlier writes from being moved beyond this point
    compiler_fence(Ordering::Release);
    IS_READY.store(true, Ordering::Relaxed);
}

fn signal_handler() {
+   compiler_fence(Ordering::Acquire);
    if IS_READY.load(Ordering::Relaxed) {
        assert_eq!(IMPORTANT_VARIABLE.load(Ordering::Relaxed), 42);
    }
}

To me it looks like there is still no happens-before relation between IS_READY.store(true, Ordering::Relaxed) and IMPORTANT_VARIABLE.load(Ordering::Relaxed). To quote from the fence docs:

A fence ‘A’ which has (at least) Release ordering semantics, synchronizes with a fence ‘B’ with (at least) Acquire semantics, if and only if there exist operations X and Y, both operating on some atomic object ‘M’ such that A is sequenced before X, Y is sequenced before B and Y observes the change to M. This provides a happens-before dependence between A and B.

    Thread 1                                          Thread 2

fence(Release);      A --------------
x.store(3, Relaxed); X ---------    |
                               |    |
                               |    |
                               -------------> Y  if x.load(Relaxed) == 3 {
                                    |-------> B      fence(Acquire);
                                                     ...
                                                 }

In this example, "B" is compiler_fence(Ordering::Acquire) and "Y" is IS_READY.load(Ordering::Relaxed) , but Y is not sequenced-before B, so there is no synchronizes-with relation between the fences, even if Y observes the change to M (IS_READY).

Now, here I wanted to make a point that this fence is actually important to prevent LLVM from reordering the reads in the signal handler, but it looks like LLVM happily miscompiles this anyway, even if you add the fence 😕 Nevermind, I misread the assembly https://rust.godbolt.org/z/Mjq4x5PaK

use std::sync::atomic::AtomicBool;
use std::sync::atomic::Ordering;
use std::sync::atomic::{compiler_fence, fence};

#[no_mangle]
static mut IMPORTANT_VARIABLE: usize = 0;

static IS_READY: AtomicBool = AtomicBool::new(false);

#[no_mangle]
fn main() {
    unsafe { IMPORTANT_VARIABLE = 42; }
    // prevent earlier writes from being moved beyond this point
    fence(Ordering::Release);
    IS_READY.store(true, Ordering::Relaxed);
}

// SAFETY: `unknown` must be 0
#[no_mangle]
fn signal_handler(unknown: usize) -> usize {
    let mut r = 0;
    for _ in 0..unknown {
        // Trick LLVM into reading IMPORTANT_VARIABLE early.
        // By our safety condition, this read does not actually happen,
        // so we don't have a data race here.
        r += unsafe { IMPORTANT_VARIABLE };
    }

    if IS_READY.load(Ordering::Relaxed) {
        // prevent later reads from being moved beyond this point
        fence(Ordering::Acquire);
        if unsafe { IMPORTANT_VARIABLE } == 42 {
            return 42;
        }
    }

    r
}

I don't see how that would fix this example.

Ah, whoops, you're correct, of course. I had mentally adjusted the loop outside the signal hook, because the loop is just useless in this example; the compiler_fence doesn't help at all if the write comes from a different physical thread than the signal hook is looping on, which is needed for the value of the atomic to change during a busy loop.

So this example really is just busted currently. It seems to be trying to use a compiler fence to ensure the store to IMPORTANT_VARIABLE is visible before the store to IS_READY, which is definitely not how a compiler fence works.

I proposed a fix for the example in #136079, please have a look.