[TSan] Make sure we only collect non-TSan frames for memory operations r=dvyukov,rsundahl,thetruestblue,wrotki,kubamracek!

[yln]

Summary

A previous change [1] moved retrieval of the caller PC
(__builtin_return_address(0) via CALLERPC) from an
interface-boundary function into a shared helper function
ExternalAccess. If this function does not get inlined, we fail to
collect the appropriate caller PC for the "TSan interface boundary".

[1] https://reviews.llvm.org/D32360

[yln]

compiler-rt/lib/tsan/rtl/tsan_external.cpp

62

Another potential solution would have been marking this function with ALWAYS_INLINE plus a comment explaining that we need it to ensure CALLERPC does the right thing. I chose the current solution since:

• It makes things explicit
• AFAIK, [no]inline attributes aren't guaranteed, but only hints; the compiler might discard them

[dvyukov]

compiler-rt/lib/tsan/rtl/tsan_external.cpp

71

Not related to the current change.
Not sure why we do all of FuncEntry/InsertShadowStackFrameForTag/FuncExit, when we don't handle the memory access itself. I think it only has any effect if we handle the memory access which can lead to a race report now or in future.

[yln]

compiler-rt/lib/tsan/rtl/tsan_external.cpp

71

This is the change that added handling for the external tag: https://reviews.llvm.org/D32382

To make the TSan external API work with Swift and other use cases, we need to track "tags" for individual memory accesses. Since there is no space to store this information in shadow cells, let's use the thread traces for that. This patch stores the tag as an extra frame in the stack traces (by calling FuncEntry and FuncExit with the address of a registered tag), this extra frame is then stripped before printing the backtrace to stderr.

Not sure why we do all of FuncEntry/InsertShadowStackFrameForTag/FuncExit, when we don't handle the memory access itself. I think it only has any effect if we handle the memory access which can lead to a race report now or in future.

I don't know the answer to this question. @kubamracek, do you remember?

//   - __tsan_external_read/__tsan_external_write annotates the logical reads
//       and writes of the object at the specified address. 'caller_pc' should
//       be the PC of the library user, which the library can obtain with e.g.
//       `__builtin_return_address(0)`.

https://github.com/llvm/llvm-project/blob/4c106cfdf7cf7eec861ad3983a3dd9a9e8f3a8ae/compiler-rt/include/sanitizer/tsan_interface.h#L129

My understanding of of the handling of the caller PC is this. The caller of __tsan_external_[write]read can decide who to "blame" for the memory operation. User code would want to say "myself"; library code would want to say "my caller":

• Calls from user code should provide caller_pc=nullptr so compiler-rt retrieves the caller pc (Swift compiler uses this)
• Library code can retrieve it's caller and then pass it in

So this means we execute the following sequence when we are called from user code (caller_pc=nullptr):

InsertShadowStackFrameForTag(thr, (uptr)tag);
MemoryAccess(thr, tsan_caller_pc, (uptr)addr, 1, typ);
FuncExit(thr);

For calls from library code (caller_pc=<user code addr>):

FuncEntry(thr, caller_pc);
InsertShadowStackFrameForTag(thr, (uptr)tag);
if (... || !libignore()->IsIgnored(caller_pc, &in_ignored_lib))    // <user code addr> not in ignored
    MemoryAccess(thr, tsan_caller_pc, (uptr)addr, 1, typ);
FuncExit(thr);
FuncExit(thr);

So I think the FuncEntry/Exit are superfluous in case we are calling from an ignored module into an annotated library that wants to attribute the access to the calling user code. I am also not sure why we always use tsan_caller_pc even if we have a valid, "not ignored" user address for caller_pc.

I think the function could look like this:

void ExternalAccess(void *addr, uptr caller_pc, uptr tsan_caller_pc, void *tag,
                    AccessType typ) {
  CHECK_LT(tag, atomic_load(&used_tags, memory_order_relaxed));
  bool in_ignored_lib;
  if (caller_pc && libignore()->IsIgnored(caller_pc, &in_ignored_lib))
    return;  // <-- early return in case we don't do a memory access check

  ThreadState *thr = cur_thread();
  if (caller_pc) FuncEntry(thr, caller_pc);
  InsertShadowStackFrameForTag(thr, (uptr)tag);
  MemoryAccess(thr, (caller_pc ? caller_pc : tsan_caller_pc), (uptr)addr, 1, typ);  // <-- use caller_pc if we have it to remain consistent
  FuncExit(thr);
  if (caller_pc) FuncExit(thr);
}

@dvyukov, what do you think? (as a follow-up)

[dvyukov]

what do you think? (as a follow-up)

Makes sense.

compiler-rt/lib/tsan/rtl/tsan_external.cpp

uptr TagFromShadowStackFrame(uptr pc) {
   uptr tag_count = atomic_load(&used_tags, memory_order_relaxed);
   void *pc_ptr = (void *)pc;
   if (pc_ptr < GetTagData(0) || pc_ptr > GetTagData(tag_count - 1))
     return 0;
   return (TagData *)pc_ptr - GetTagData(0);
 }
 
 #if !SANITIZER_GO
 
-void ExternalAccess(void *addr, uptr caller_pc, void *tag, AccessType typ) {
+void ExternalAccess(void *addr, uptr caller_pc, uptr tsan_caller_pc, void *tag,
+                    AccessType typ) {
   CHECK_LT(tag, atomic_load(&used_tags, memory_order_relaxed));
   ThreadState *thr = cur_thread();
   if (caller_pc) FuncEntry(thr, caller_pc);
   InsertShadowStackFrameForTag(thr, (uptr)tag);
   bool in_ignored_lib;
   if (!caller_pc || !libignore()->IsIgnored(caller_pc, &in_ignored_lib))
-    MemoryAccess(thr, CALLERPC, (uptr)addr, 1, typ);
+    MemoryAccess(thr, tsan_caller_pc, (uptr)addr, 1, typ);
   FuncExit(thr);
   if (caller_pc) FuncExit(thr);
 }
 
 extern "C" {
 SANITIZER_INTERFACE_ATTRIBUTE
 void *__tsan_external_register_tag(const char *object_type) {
   uptr new_tag = atomic_fetch_add(&used_tags, 1, memory_order_relaxed);
void __tsan_external_assign_tag(void *addr, void *tag) {
   }
   if (b) {
     b->tag = (uptr)tag;
   }
 }
 
 SANITIZER_INTERFACE_ATTRIBUTE
 void __tsan_external_read(void *addr, void *caller_pc, void *tag) {
-  ExternalAccess(addr, STRIP_PAC_PC(caller_pc), tag, kAccessRead);
+  ExternalAccess(addr, STRIP_PAC_PC(caller_pc), CALLERPC, tag, kAccessRead);
 }
 
 SANITIZER_INTERFACE_ATTRIBUTE
 void __tsan_external_write(void *addr, void *caller_pc, void *tag) {
-  ExternalAccess(addr, STRIP_PAC_PC(caller_pc), tag, kAccessWrite);
+  ExternalAccess(addr, STRIP_PAC_PC(caller_pc), CALLERPC, tag, kAccessWrite);
 }
 }  // extern "C"
 
 #endif  // !SANITIZER_GO
 
 }  // namespace __tsan

compiler-rt/test/tsan/Darwin/external-swift-debugging.cpp

 // RUN: %clangxx_tsan %s -o %t
 // RUN: %deflake %run %t 2>&1 | FileCheck %s
 
+#include <dlfcn.h>
 #include <thread>
 
 #import "../test.h"
 
 
 extern "C" {
 int __tsan_get_report_data(void *report, const char **description, int *count,
                            int *stack_count, int *mop_count, int *loc_count,
                            int *mutex_count, int *thread_count,
                            int *unique_tid_count, void **sleep_trace,
                            unsigned long trace_size);
 int __tsan_get_report_tag(void *report, unsigned long *tag);
+int __tsan_get_report_mop(void *report, unsigned long idx, int *tid, void **addr,
+                          int *size, int *write, int *atomic, void **trace,
+                          unsigned long trace_size);
 }
 
 __attribute__((no_sanitize("thread"), noinline))
 void ExternalWrite(void *addr) {
   void *kSwiftAccessRaceTag = (void *)0x1;
   __tsan_external_write(addr, nullptr, kSwiftAccessRaceTag);
 }
 
 int main(int argc, char *argv[]) {
   barrier_init(&barrier, 2);
   fprintf(stderr, "Start.\n");
   // CHECK: Start.
-  
+  fprintf(stderr, "ExternalWrite function address: %p\n", &ExternalWrite);
+  // CHECK: ExternalWrite function address: [[ExternalWrite_addr:0x[0-9a-z]+]]
+
   void *opaque_object = malloc(16);
   std::thread t1([opaque_object] {
     ExternalWrite(opaque_object);
     barrier_wait(&barrier);
   });
   std::thread t2([opaque_object] {
     barrier_wait(&barrier);
     ExternalWrite(opaque_object);
   });
   // CHECK: WARNING: ThreadSanitizer: Swift access race
   // CHECK: Modifying access of Swift variable at {{.*}} by thread {{.*}}
+  // CHECK:   #0 ExternalWrite
   // CHECK: Previous modifying access of Swift variable at {{.*}} by thread {{.*}}
+  // CHECK:   #0 ExternalWrite
   // CHECK: SUMMARY: ThreadSanitizer: Swift access race
   t1.join();
   t2.join();
 
   fprintf(stderr, "Done.\n");
 }
 
 extern "C" __attribute__((disable_sanitizer_instrumentation)) void
 __tsan_on_report(void *report) {
   const char *description;
   int count;
   int stack_count, mop_count, loc_count, mutex_count, thread_count,
       unique_tid_count;
   void *sleep_trace[16] = {0};
   __tsan_get_report_data(report, &description, &count, &stack_count, &mop_count,
                          &loc_count, &mutex_count, &thread_count,
                          &unique_tid_count, sleep_trace, 16);
-  fprintf(stderr, "report type = '%s', count = %d\n", description, count);
-  // CHECK: report type = 'external-race', count = 0
+  fprintf(stderr, "report type = '%s', count = %d, mop_count = %d\n", description, count, mop_count);
+  // CHECK: report type = 'external-race', count = 0, mop_count = 2
 
   unsigned long tag;
   __tsan_get_report_tag(report, &tag);
   fprintf(stderr, "tag = %ld\n", tag);
   // CHECK: tag = 1
+
+  int tid, size, write, atomic;
+  void *addr;
+  void *trace[16] = {0};
+  __tsan_get_report_mop(report, /*idx=*/0, &tid, &addr, &size, &write, &atomic,
+                        trace, 16);
+  fprintf(stderr, "Racy write trace (1 of 2):\n");
+  for (int i = 0; i < 16 && trace[i]; i++) {
+    Dl_info info;
+    dladdr(trace[i], &info);
+    fprintf(stderr, "  %d: frame: %p, function: %p %s\n", i, trace[i],
+            info.dli_saddr, info.dli_sname);
+  }
+  // Ensure ExternalWrite() function is top of trace
+  // CHECK: 0: frame: 0x{{[0-9a-z]+}}, function: [[ExternalWrite_addr]] _Z13ExternalWritePv
 }
 
 // CHECK: Done.
 // CHECK: ThreadSanitizer: reported 1 warnings