[PATCH] IRGen: __c11/__atomic compare-and-exchange should respect the standard

[Richard Smith]

  I believe this change is correct, but I want someone else with knowledge of the memory model to look at it before it goes in.


================
Comment at: lib/CodeGen/CGAtomic.cpp:234
@@ +233,3 @@
+    // Update the memory at Expected with Old's value.
+    llvm::StoreInst *StoreExpected = CGF.Builder.CreateStore(Old, Val1);
+    StoreExpected->setAlignment(Align);
----------------
Richard Smith wrote:
> This is still not correct, as far as I can see.
> 
> According to the C++ standard, the value at `Val1` is replaced as part of the atomic operation of `atomic_compare_exchange_{weak|strong}{_explicit}`, and that update has the memory order given by the second memory order argument to the function.
> 
> The first half of this requirement seems highly problematic, and is probably a bug in the standard. The second half of this requirement seems to require that `Val1` is updated by an atomic store, using the 'failure' ordering for the atomic compare exchange operation.
OK, so... the standard does imply that the 'expected' access is non-atomic, which means that the case described in PR18899 already contains a data race (between the load of 'expected' in the __atomic_compare_exchange and the hypothesized store in another thread). So our current behavior is at worst introducing a "benign" race against a store of the already-present value, which is much less bad than this seemed, but still very much worth fixing.

The memory order here is still wrong, though, as far as I can see. A memory_order_seq_cst atomic_compare_exchange is required to publish the new value of 'expected' according to the current wording in the standard. That too is probably a standard defect.


http://llvm-reviews.chandlerc.com/D2922