[PATCH] D16191: [tsan] Add TSan debugger APIs

[Dmitry Vyukov via llvm-commits]

dvyukov added a comment.

So this is for lldb.
I though that you want to write tsan tests in such style using __tsan_on_report callback.
lldb is fine.


================
Comment at: lib/tsan/rtl/tsan_debugging.cc:57
@@ +56,3 @@
+SANITIZER_INTERFACE_ATTRIBUTE
+void *__tsan_get_current_report() {
+  const ReportDesc *rep = cur_thread()->current_report;
----------------
Do we need this function?
User already gets report pointer as argument to __tsan_on_report. So this function looks unnecessary and subject to calls at a wrong time, and provoking dangling cur_thread()->current_report (as I can see below).
I would remove it.

================
Comment at: lib/tsan/rtl/tsan_debugging.cc:63
@@ +62,3 @@
+SANITIZER_INTERFACE_ATTRIBUTE
+int __tsan_get_report_data(void *report, const char **description, int *count,
+                           void **sleep_trace, uptr trace_size) {
----------------
Argument set for this function looks somewhat strange.
I would either move sleep_trace to a separate function (sleep trace is very auxiliary). Or make this function returns as much assorted data as possible (e.g. also return number of stacks, number of locations, etc).

================
Comment at: lib/tsan/rtl/tsan_rtl_report.cc:51
@@ -50,2 +50,3 @@
 bool OnReport(const ReportDesc *rep, bool suppressed);
+extern "C" void __tsan_on_report(const ReportDesc *rep);
 #else
----------------
I afraid this will break some of out internal setups which define TSAN_EXTERNAL_HOOKS and define OnReport function. They don't define __tsan_on_report, so they will break on next llvm update.

Define it as:

SANITIZER_WEAK_DEFAULT_IMPL
void __tsan_on_report(const ReportDesc *rep) {
  (void)rep;
}

outside of #ifdef/else/endif TSAN_EXTERNAL_HOOKS.


================
Comment at: lib/tsan/rtl/tsan_rtl_report.cc:522
@@ -515,3 +521,3 @@
     if (suppressed)
       return false;
   }
----------------
Forget to reset thr->current_report here.


================
Comment at: test/tsan/debugging.cc:44
@@ +43,3 @@
+  fprintf(stderr, "__tsan_on_report(%p)\n", report);
+  fprintf(stderr, "__tsan_get_current_report() = %p\n", report);
+  // CHECK: __tsan_on_report([[REPORT:0x[0-9a-f]+]])
----------------
I guess you wanted to print result of __tsan_get_current_report() here.


http://reviews.llvm.org/D16191