Page MenuHomePhabricator

[TSAN] Honor acquire failure mode on AtomicCAS
Needs RevisionPublic

Authored by bruno on Fri, Mar 26, 12:12 PM.

Details

Summary

The lack of failure mode support for compare_exchange causes false positives when using release/acquire (success/failure) memory orders. Add acquire fmo handling and a testcase.

While here add a testcase for release/relaxed (success/failure) memory orders leading to a race.

https://github.com/google/sanitizers/issues/970

Diff Detail

Event Timeline

bruno created this revision.Fri, Mar 26, 12:12 PM
bruno requested review of this revision.Fri, Mar 26, 12:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Mar 26, 12:12 PM
bruno added a reviewer: yln.
delcypher added a reviewer: kubamracek.

@bruno Thanks for the patch. TSan's runtime isn't my specialty so I've added other reviewers.

compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp
433

Is this something user facing code can set? If yes, then we might want to emit a warning rather than crashing the process.

yln accepted this revision.Tue, Apr 6, 9:38 AM

I am happy with the mechanics and quality of the patch. Ideally @dvyukov could give a final sign-off.

compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp
433

I think calls are generated by the compiler in ThreadSanitizer::instrumentAtomic():

Value *Args[] = {IRB.CreatePointerCast(Addr, PtrTy),
                 CmpOperand,
                 NewOperand,
                 createOrdering(&IRB, CASI->getSuccessOrdering()),
                 createOrdering(&IRB, CASI->getFailureOrdering())};
CallInst *C = IRB.CreateCall(TsanAtomicCAS[Idx], Args);
This revision is now accepted and ready to land.Tue, Apr 6, 9:38 AM
bruno added a comment.Tue, Apr 6, 5:33 PM

Thanks both of you for the review @delcypher and @yln, will wait for @dvyukov to sign-off!

compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp
433

Right, but before things are expanded for instrumentation, there's some sanitization of the failure memory order in emitAtomicCmpXchgFailureSet, so an invalid combination shouldn't get to this point.

dvyukov added inline comments.Wed, Apr 14, 1:40 AM
compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp
434

Can't fmo be consume/acq_rel/seq_cst? If yes, please add a test.

436

I think we should not search the object and re-acquire the mutex second time for performance reasons and complexity (won't need to re-read the value).
Note that we may have already acquired due to mo. We should not acquire second time in that case.

443

I don't think this is correct. Can't this lead to false positives?
Consider that a thread does CAS-release to hand off the object to another thread, and that thread frees the object. I think this memory access can race with the free. It's generally not OK to touch memory after atomic operations.
If if leads to a false positive, please add a test that catches it.

compiler-rt/test/tsan/compare_exchange_release_relaxed.cpp
18

If this test differs only by memory order, it can make sense to use parametrized tests. It would also be good to test other mo combinations (e.g. that the CHECK(IsLoadOrder(fmo)) does not fail).
See e.g. test/tsan/ignore_lib0.cpp for an example.

Also I wonder if we could first evaluate CAS and then use either mo or fmo accordingly, so that we don't over-synchronize on failure if mo is stronger than fmo.

Not directly related to the change but we could also episodically fail weak CASes to test fmo case better.

yln requested changes to this revision.Tue, Apr 20, 6:26 PM

Unapprove until Dmitry's comments are addressed.

This revision now requires changes to proceed.Tue, Apr 20, 6:26 PM