Change TSAN external symbolization API to support returning multiple frames

[asmundak]

Summary

New API passes a callback function to the external symbolizer, allowing it to add multiple frames to the traceback. Note that the old interface API will be still supported until the clients migrate to the new one.

[dvyukov]

Looks good. Comitting.

[dvyukov]

Submitted in rev 328079.

[dvyukov]

This broke darwin build. I've fixed it up in:
http://llvm.org/viewvc/llvm-project?view=revision&revision=328082
Both external symbolization tests still pass, so I hope it works for your use case.

[asmundak]

Sorry, I forgot (and don't know how) to build on Darwin.
Thanks for the help.

lib/sanitizer_common/sanitizer_common.h

 static const uptr kErrorMessageBufferSize = 1 << 16;
 
 // Denotes fake PC values that come from JIT/JAVA/etc.
-// For such PC values __tsan_symbolize_external() will be called.
+// For such PC values __tsan_symbolize_external_ex() will be called.
 const u64 kExternalPCBit = 1ULL << 60;
 
 extern const char *SanitizerToolName;  // Can be changed by the tool.

lib/tsan/rtl/tsan_rtl_report.cc

     // callback. Most likely, TraceTopPC will now return a EventTypeFuncExit
     // event. Later we subtract -1 from it (in GetPreviousInstructionPc)
     // and the resulting PC has kExternalPCBit set, so we pass it to
-    // __tsan_symbolize_external. __tsan_symbolize_external is within its rights
-    // to crash since the PC is completely bogus.
+    // __tsan_symbolize_external_ex. __tsan_symbolize_external_ex is within its
+    // rights to crash since the PC is completely bogus.
     // test/tsan/double_race.cc contains a test case for this.
     toppc = 0;
   }

lib/tsan/rtl/tsan_symbolize.cc

   thr->ignore_interceptors--;
 }
 
+// Legacy API.
 // May be overriden by JIT/JAVA/etc,
 // whatever produces PCs marked with kExternalPCBit.
 SANITIZER_WEAK_DEFAULT_IMPL

   return false;
 }
 
+// New API: call __tsan_symbolize_external_ex only when it exists.
+// Once old clients are gone, provide dummy implementation.
+extern "C" SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
+void __tsan_symbolize_external_ex(uptr pc,
+                                  void (*add_frame)(void *, const char *,
+                                                    const char *, int, int),
+                                  void *ctx);
+
+struct SymbolizedStackBuilder {
+  SymbolizedStack *head;
+  SymbolizedStack *tail;
+  uptr addr;
+};
+
+static void AddFrame(void *ctx, const char *function_name, const char *file,
+                     int line, int column) {
+  SymbolizedStackBuilder *ssb = (struct SymbolizedStackBuilder *)ctx;
+  if (ssb->tail) {
+    ssb->tail->next = SymbolizedStack::New(ssb->addr);
+    ssb->tail = ssb->tail->next;
+  } else {
+    ssb->head = ssb->tail = SymbolizedStack::New(ssb->addr);
+  }
+  AddressInfo *info = &ssb->tail->info;
+  if (function_name) {
+    info->function = internal_strdup(function_name);
+  }
+  if (file) {
+    info->file = internal_strdup(file);
+  }
+  info->line = line;
+  info->column = column;
+}
+
 SymbolizedStack *SymbolizeCode(uptr addr) {
   // Check if PC comes from non-native land.
   if (addr & kExternalPCBit) {
+    if (__tsan_symbolize_external_ex) {
+      SymbolizedStackBuilder ssb = {nullptr, nullptr, addr};
+      __tsan_symbolize_external_ex(addr, AddFrame, &ssb);
+      return ssb.head ? ssb.head : SymbolizedStack::New(addr);
+    }
+    // Legacy code: remove along with the declaration above
+    // once all clients using this API are gone.
     // Declare static to not consume too much stack space.
     // We symbolize reports in a single thread, so this is fine.
     static char func_buf[1024];

test/tsan/java_symbolization.cc

 #include "java.h"
 #include <memory.h>
 
-extern "C" bool __tsan_symbolize_external(jptr pc,
-                                          char *func_buf, jptr func_siz,
-                                          char *file_buf, jptr file_siz,
-                                          int *line, int *col) {
+extern "C" void __tsan_symbolize_external_ex(
+    jptr pc, void (*add_frame)(void *, const char *, const char *, int, int),
+    void *ctx) {
   if (pc == (1234 | kExternalPCBit)) {
-    memcpy(func_buf, "MyFunc", sizeof("MyFunc"));
-    memcpy(file_buf, "MyFile.java", sizeof("MyFile.java"));
-    *line = 1234;
-    *col = 56;
-    return true;
+    add_frame(ctx, "MyInnerFunc", "MyInnerFile.java", 1234, 56);
+    add_frame(ctx, "MyOuterFunc", "MyOuterFile.java", 4321, 65);
   }
-  return false;
 }
 
 void *Thread(void *p) {

 }
 
 // CHECK: WARNING: ThreadSanitizer: data race
-// CHECK:     #0 MyFunc MyFile.java:1234:56
+// CHECK:     #0 MyInnerFunc MyInnerFile.java:1234:56
+// CHECK:     #1 MyOuterFunc MyOuterFile.java:4321:65
 // CHECK: DONE

test/tsan/java_symbolization_legacy.cc

+// RUN: %clangxx_tsan -O1 %s -o %t && %deflake %run %t | FileCheck %s
+#include "java.h"
+#include <memory.h>
+
+extern "C" bool __tsan_symbolize_external(jptr pc,
+                                          char *func_buf, jptr func_siz,
+                                          char *file_buf, jptr file_siz,
+                                          int *line, int *col) {
+  if (pc == (1234 | kExternalPCBit)) {
+    memcpy(func_buf, "MyFunc", sizeof("MyFunc"));
+    memcpy(file_buf, "MyFile.java", sizeof("MyFile.java"));
+    *line = 1234;
+    *col = 56;
+    return true;
+  }
+  return false;
+}
+
+void *Thread(void *p) {
+  barrier_wait(&barrier);
+  __tsan_write1_pc((jptr)p, 1234 | kExternalPCBit);
+  return 0;
+}
+
+int main() {
+  barrier_init(&barrier, 2);
+  int const kHeapSize = 1024 * 1024;
+  jptr jheap = (jptr)malloc(kHeapSize + 8) + 8;
+  __tsan_java_init(jheap, kHeapSize);
+  const int kBlockSize = 16;
+  __tsan_java_alloc(jheap, kBlockSize);
+  pthread_t th;
+  pthread_create(&th, 0, Thread, (void*)jheap);
+  __tsan_write1_pc((jptr)jheap, 1234 | kExternalPCBit);
+  barrier_wait(&barrier);
+  pthread_join(th, 0);
+  __tsan_java_free(jheap, kBlockSize);
+  fprintf(stderr, "DONE\n");
+  return __tsan_java_fini();
+}
+
+// CHECK: WARNING: ThreadSanitizer: data race
+// CHECK:     #0 MyFunc MyFile.java:1234:56
+// CHECK: DONE