One small change and I'm happy with this.
================
Comment at: lib/CodeGen/CGAtomic.cpp:216
@@ -214,1 +215,3 @@
+ llvm::Value *Cmp = CGF.Builder.CreateICmpEQ(Old, Expected);
CGF.EmitStoreOfScalar(Cmp, CGF.MakeAddrLValue(Dest, E->getType()));
+
----------------
This should really be done in the continue block below IMO. There is no reason
for this to be live across them. So I think that would move this to be
essentially at the end of the block again.
================
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:
> 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.
There is no memory ordering concern here. This is merely storing to the local
variable used by expected, which is a non-atomic value. The ordering for the
failure atomic load is an ordering on cmpxchg itself rather than anything here.
Tim Northover has some commits to make this work correctly.
http://llvm-reviews.chandlerc.com/D2922
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
