This is an archive of the discontinued LLVM Phabricator instance.

tsan: Adding releaseAcquire() to ThreadClock
ClosedPublic

Authored by dfava on Mar 17 2020, 1:54 PM.

Details

Reviewers
dvyukov
Summary

realeaseAcquire() is a new function added to TSan in support of the Go data-race detector. It's semantics is:

    
void ThreadClock::releaseAcquire(SyncClock *sc) const {
  for (int i = 0; i < kMaxThreads; i++) {
    tmp = clock[i];
    clock[i] = max(clock[i], sc->clock[i]);
    sc->clock[i] = tmp;
  }
}

Diff Detail

Event Timeline

dfava created this revision.Mar 17 2020, 1:54 PM
dfava updated this revision to Diff 250905.Mar 17 2020, 2:36 PM

Added changes to tsan_go.cpp which I had forgotten to include in the first diff.

Overall looks good, which some nits.

Please also add this function to compiler-rt/lib/tsan/go/test.c. It's not a super comprehensive test, but it ensures compilation and absence of stupid crashes. Which is still very useful in the situation when tsan/Go parts are split across 2 repos and are updated episodically.

compiler-rt/lib/tsan/rtl/tsan_clock.cpp
37

So this is effectively releaseStore.
There is a somewhat mismatching terminology we use in tsan runtime vs Go api.
If Go api these are called releaseMerge and release, which corresponds to release and releaseStore terminology in tsan runtime. So more complete name would be something like releaseStoreAcquire.

205

This acquired is not used (other than for stats).
We either need to remove this, or do just release and return if already acquired. I think that the expected case is that this is not acquired yet, so probably we drop this part?

dfava updated this revision to Diff 251372.Mar 19 2020, 7:02 AM
dvyukov accepted this revision.Mar 24 2020, 2:28 AM

Perfect. Thanks.

This revision is now accepted and ready to land.Mar 24 2020, 2:28 AM
pcc added a subscriber: pcc.Mar 27 2020, 1:30 PM

It looks like this change broke the sanitizer-x86_64-linux-autoconf buildbot.
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/48824

/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/tsan/rtl/tsan_clock.cpp:190:13: error: use of undeclared identifier 'dst'
  DCHECK_LE(dst->size_, kMaxTid);
            ^
1 error generated.

Can you please take a look?

dfava added a comment.Mar 27 2020, 3:07 PM
In D76322#1946849, @pcc wrote:

It looks like this change broke the sanitizer-x86_64-linux-autoconf buildbot.
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/48824

/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/tsan/rtl/tsan_clock.cpp:190:13: error: use of undeclared identifier 'dst'
  DCHECK_LE(dst->size_, kMaxTid);
            ^
1 error generated.

Can you please take a look?

Indeed a bug. It looks like the DCHECK is #defined to nothing unless SANITIZER_DEBUG is set, which is why we didn't catch the issue earlier.

I mentioned in an email just now that I would fix it, but since this is a one line fix and I don't have commit access, would it be easier if your or Dmitry makes the change?

The change is from:

DCHECK_LE(dst->size_, kMaxTid);

to:

DCHECK_LE(sc->size_, kMaxTid);

Let me know.

Sorry for the regression and thanks again for pointing it out.

In D76322#1946849, @pcc wrote:

It looks like this change broke the sanitizer-x86_64-linux-autoconf buildbot.
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/48824

/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/tsan/rtl/tsan_clock.cpp:190:13: error: use of undeclared identifier 'dst'
  DCHECK_LE(dst->size_, kMaxTid);
            ^
1 error generated.

Can you please take a look?

Indeed a bug. It looks like the DCHECK is #defined to nothing unless SANITIZER_DEBUG is set, which is why we didn't catch the issue earlier.

I mentioned in an email just now that I would fix it, but since this is a one line fix and I don't have commit access, would it be easier if your or Dmitry makes the change?

The change is from:

DCHECK_LE(dst->size_, kMaxTid);

to:

DCHECK_LE(sc->size_, kMaxTid);

Let me know.

Sorry for the regression and thanks again for pointing it out.

Done in https://github.com/llvm/llvm-project/commit/65b4695375c6518c0b51a2a4c8392ab9d461e317