tsan: Adding releaseAcquire() to ThreadClock

[dfava]

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;
  }
}

[dfava]

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

[dvyukov]

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?

[dvyukov]

Perfect. Thanks.

[dvyukov]

Committed in https://github.com/llvm/llvm-project/commit/180d211770a0a1f82d483433fab007cd4d14cbd9

[pcc]

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]

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.

[dvyukov]

In D76322#1947063, @dfava wrote:

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