[TSan] Add read/write range interface functions with PC

[protze.joachim]

Summary

For most memory access functions like __tsan_write8 there is a variant to provide a pc: __tsan_write8_pc.
This is not the case for __tsan_write_range, which allows to annotate memory access for a range of memory. The main advantage we see in the range version is, that it only ticks once and therefore saves entries in the history ring buffer. Therefore, we would prefer to call __tsan_write_range_pc over looping on __tsan_write8_pc.

This patch adds the two new interface functions __tsan_write_range_pc and __tsan_read_range_pc, which take the additional PC argument.

[dvyukov]

Who are "we"? And you would prefer to call them where? ;)
Also, tests.

[protze.joachim]

In D66885#1649241, @dvyukov wrote:

Who are "we"? And you would prefer to call them where? ;)

I'm working with some students in my group on the MPI and OpenMP runtime correctness checking tools called MUST and Archer.
For MPI communication calls we are working on annotation for the memory access semantics of the buffer accesses. Since these memory accesses are typically done by the NIC, TSan could never observe such accesses directly. In multi-threaded MPI applications this annotation allows us to detect data race between communication and other threads accessing the buffer.

A simplified example of such annotation would look like:

int MPI_Send(const void *buf, int count, MPI_Datatype datatype, int dest, int tag, MPI_Comm comm){
  int size;
  MPI_Type_size(datatype, &size);
  __tsan_read_range_pc(buf, (uptr)size*count, __builtin_return_address(0));
  return PMPI_Send(buf, count, datatype, dest, tag, comm);
}

Is there a better interface for doing such annotations?
In our specific use case, also asan and msan might be interesting. I didn't look into interfaces of those, but a generic interface for all those sanitizers might be interesting.

Also, tests.

Ok, I'll look into tests for this. But will take some time.

[dvyukov]

Thanks for the background. Good to know.

I think we could actually do common cross-sanitizer annotation for this in common_interface_defs.h. We already have e.g. sanitizer_unaligned_load/storeN there that are implemented by all sanitizers.
Access annotations are historically separate, but it makes sense to have a common one. Asan also has asan_address_is_poisoned/__asan_region_is_poisoned.
Kostya, Eugenii, Vitaly, any objections to adding something like __sanitizer_read/write_range_pc?
There is a question of what to do in msan versions. Should read to the checking? Should write unpoison? But we could choose what we expect to be more common case and add other special versions for msan if necessary later.

[dvyukov]

Re tests. Check test/tsan/ dir, e.g. simple_race.c, stack_race.cc, test/tsan/unaligned_norace.cc. There are not too hard to write, we just need to check it builds, detects a race on the annotation and the PC is used in the stack.

[protze.joachim]

I derived a test from java_race_pc.cc.

In this new test, I see the same issue related to race on stack variables as reported here:
https://github.com/google/sanitizers/issues/1134

[dvyukov]

In this new test, I see the same issue related to race on stack variables as reported here:
https://github.com/google/sanitizers/issues/1134

Does it make the test fail? Or just produces a warning? We could switch to a heap-allocated block or a global.

[protze.joachim]

In D66885#1651428, @dvyukov wrote:

In this new test, I see the same issue related to race on stack variables as reported here:
https://github.com/google/sanitizers/issues/1134

Does it make the test fail? Or just produces a warning? We could switch to a heap-allocated block or a global.

The test succeeds, it just produces the additional messages from llvm-symbolizer.

[dvyukov]

Generally looks good to me.

Joachim, do you have commit access? Or you want us to land it?

Vitaly, please take a look too and land if necessary.

lib/tsan/rtl/tsan_interface.h

 SANITIZER_INTERFACE_ATTRIBUTE
 void __tsan_external_write(void *addr, void *caller_pc, void *tag);
 
 SANITIZER_INTERFACE_ATTRIBUTE
 void __tsan_read_range(void *addr, unsigned long size);  // NOLINT
 SANITIZER_INTERFACE_ATTRIBUTE
 void __tsan_write_range(void *addr, unsigned long size);  // NOLINT
 
+SANITIZER_INTERFACE_ATTRIBUTE
+void __tsan_read_range_pc(void *addr, unsigned long size, void *pc);  // NOLINT
+SANITIZER_INTERFACE_ATTRIBUTE
+void __tsan_write_range_pc(void *addr, unsigned long size, void *pc);  // NOLINT
+
 // User may provide function that would be called right when TSan detects
 // an error. The argument 'report' is an opaque pointer that can be used to
 // gather additional information using other TSan report API functions.
 SANITIZER_INTERFACE_ATTRIBUTE
 void __tsan_on_report(void *report);
 
 // If TSan is currently reporting a detected issue on the current thread,
 // returns an opaque pointer to the current report. Otherwise returns NULL.

lib/tsan/rtl/tsan_interface_inl.h

 
 void __tsan_read_range(void *addr, uptr size) {
   MemoryAccessRange(cur_thread(), CALLERPC, (uptr)addr, size, false);
 }
 
 void __tsan_write_range(void *addr, uptr size) {
   MemoryAccessRange(cur_thread(), CALLERPC, (uptr)addr, size, true);
 }
+
+void __tsan_read_range_pc(void *addr, uptr size, void *pc) {
+  MemoryAccessRange(cur_thread(), (uptr)pc, (uptr)addr, size, false);
+}
+
+void __tsan_write_range_pc(void *addr, uptr size, void *pc) {
+  MemoryAccessRange(cur_thread(), (uptr)pc, (uptr)addr, size, true);
+}

test/tsan/race_range_pc.cc

+// RUN: %clangxx_tsan -O1 %s -o %t && %deflake %run %t | FileCheck %s
+// This test fails on powerpc64 big endian.
+// The Tsan report is returning wrong information about
+// the location of the race.
+// XFAIL: powerpc64-unknown-linux-gnu
+
+#include "test.h"
+
+typedef unsigned long uptr;
+extern "C" void __tsan_read_range_pc(uptr addr, uptr size, uptr pc);
+extern "C" void __tsan_write_range_pc(uptr addr, uptr size, uptr pc);
+
+void foobar() {
+}
+
+void barbaz() {
+}
+
+void *Thread(void *p) {
+  barrier_wait(&barrier);
+  __tsan_read_range_pc((uptr)p, 32, (uptr)foobar + kPCInc);
+  return 0;
+}
+
+int main() {
+  barrier_init(&barrier, 2);
+  int a[128];
+  pthread_t th;
+  pthread_create(&th, 0, Thread, (void*)a);
+  __tsan_write_range_pc((uptr)(a+2), 32, (uptr)barbaz + kPCInc);
+  barrier_wait(&barrier);
+  pthread_join(th, 0);
+  fprintf(stderr, "DONE\n");
+  return 0;
+}
+
+// CHECK: WARNING: ThreadSanitizer: data race
+// CHECK:     #0 foobar
+// CHECK:     #0 barbaz
+// CHECK: DONE