[tsan] Add debugging API to retrieve the "external tag" from reports

[delcypher]

lib/tsan/rtl/tsan_debugging.cc

89

Why return 1? What does that mean?

lib/tsan/rtl/tsan_interface.h

122

There's no documentation on the meaning of the arguments or the return value here.

test/tsan/Darwin/external-swift-debugging.cc

11

I know for ASan we ship the interface header with Clang. Do we not do that with TSan? If we do can you just include that so you don't have to redefine this here?

40

I don't understand this. There's no Swift code in this test case so how is this a Swift access race?

[kubamracek]

lib/tsan/rtl/tsan_debugging.cc

89

All the APIs return 1 on success, 0 on failure.

lib/tsan/rtl/tsan_interface.h

122

See comments around. "report" is an opaque pointer representing the report we want to extract data from, "tag" is the output variable. All these APIs return non-zero on success, zero on failure.

Sorry about the lack of formal documentation here, it's because this is only for debugger integration (and not really a public API) and this header file isn't shipped anywhere.

test/tsan/Darwin/external-swift-debugging.cc

11

Yeah, we do not. The reason is that these are not really stable APIs.

40

The test is faking what a Swift code would do via the __tsan_external_write call.

[george.karpenkov]

LGTM, but would be better with a comment in the header describing the function

lib/tsan/rtl/tsan_interface.h

122

I would also prefer a basic doxygen comment.

[kubamracek]

Added a doxy-style comment describing the function.

[kubamracek]

@delcypher @george.karpenkov added a comment. looks good now?

[george.karpenkov]

Yep!

[delcypher]

test/tsan/Darwin/external-swift-debugging.cc

19

Why does tag need to be a global here? Can't it just be a literal in ExternalWrite which is the only place its used.

40

This is still incredibly unclear. Why when using __tsan_external_write

• Is it okay to pass nullptr as caller_pc?
• The fact that 0x1 means a swift race is not all obvious. A bit of grepping shows this is defined in lib/tsan/rtl/tsan_defs.h but this is not at all obvious.

Please add some comments to ExternalWrite to explain what is going on (i.e. that you're faking a swift access will a special tag that is defined in lib/tsan/rtl/tsan_defs.h ).

63

This variable is hiding the tag global variable of the same name. Removing the global variable will fix this.

[kubamracek]

test/tsan/Darwin/external-swift-debugging.cc

40

I don't want to expand the scope of this patch. Let's strictly focus on the added API.

[george.karpenkov]

test/tsan/Darwin/external-swift-debugging.cc

40

@delcypher this file is just a test. The race itself is immaterial here, from my understanding the test is there to check that we can get the correct tag from the report. In fact, all the CHECK: lines about the race could be even dropped.

[kubamracek]

Updated the test file to avoid name clash on "tag".

[delcypher]

test/tsan/Darwin/external-swift-debugging.cc

21

@kubamracek Great. Thanks for using a clearer name. A comment probably isn't necessary anymore. Out of curiosity, why does this variable have static storage? I can't think of a reason why that would be necessary.

40

@george.karpenkov Although this file is a test, it's important to make the test as understandable as possible without putting too much burden on the test writer. This test in the original patch wasn't very easy to understand and a simple comment explaining what ExternalWrite was doing would help with understanding.

That being said now that @kubamracek has moved the tag definition into ExternalWrite and named it kSwiftAccessRaceTag to code is clearer and a comment probably isn't needed any more.

[kubamracek]

Dropping 'static'.

[delcypher]

In D46661#1095150, @kubamracek wrote:

@delcypher @george.karpenkov added a comment. looks good now?

Yep. Much better.

[delcypher]

Thanks for making changes. LGTM.

lib/tsan/rtl/tsan_debugging.cc

int __tsan_get_report_data(void *report, const char **description, int *count,
   *mutex_count = rep->mutexes.Size();
   *thread_count = rep->threads.Size();
   *unique_tid_count = rep->unique_tids.Size();
   if (rep->sleep) CopyTrace(rep->sleep->frames, sleep_trace, trace_size);
   return 1;
 }
 
 SANITIZER_INTERFACE_ATTRIBUTE
+int __tsan_get_report_tag(void *report, uptr *tag) {
+  const ReportDesc *rep = (ReportDesc *)report;
+  *tag = rep->tag;
+  return 1;
+}
+
+SANITIZER_INTERFACE_ATTRIBUTE
 int __tsan_get_report_stack(void *report, uptr idx, void **trace,
                             uptr trace_size) {
   const ReportDesc *rep = (ReportDesc *)report;
   CHECK_LT(idx, rep->stacks.Size());
   ReportStack *stack = rep->stacks[idx];
   if (stack) CopyTrace(stack->frames, trace, trace_size);
   return stack ? 1 : 0;
 }

lib/tsan/rtl/tsan_interface.h

 // one was involved in the issue).
 SANITIZER_INTERFACE_ATTRIBUTE
 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,
                            uptr trace_size);
 
+/// Retrieves the "tag" from a report (for external-race report types). External
+/// races can be associated with a tag which give them more meaning. For example
+/// tag value '1' means "Swift access race". Tag value '0' indicated a plain
+/// external race.
+///
+/// \param report opaque pointer to the current report (obtained as argument in
+///               __tsan_on_report, or from __tsan_get_current_report)
+/// \param [out] tag points to storage that will be filled with the tag value
+///
+/// \returns non-zero value on success, zero on failure
+SANITIZER_INTERFACE_ATTRIBUTE
+int __tsan_get_report_tag(void *report, uptr *tag);
+
 // Returns information about stack traces included in the report.
 SANITIZER_INTERFACE_ATTRIBUTE
 int __tsan_get_report_stack(void *report, uptr idx, void **trace,
                             uptr trace_size);
 
 // Returns information about memory operations included in the report.
 SANITIZER_INTERFACE_ATTRIBUTE
 int __tsan_get_report_mop(void *report, uptr idx, int *tid, void **addr,

test/tsan/Darwin/external-swift-debugging.cc

+// RUN: %clangxx_tsan %s -o %t
+// RUN: %deflake %run %t 2>&1 | FileCheck %s
+
+#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);
+}
+
+__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.
+  
+  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: Previous modifying access of Swift variable at {{.*}} by thread {{.*}}
+  // CHECK: SUMMARY: ThreadSanitizer: Swift access race
+  t1.join();
+  t2.join();
+
+  fprintf(stderr, "Done.\n");
+}
+
+extern "C"
+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
+
+  unsigned long tag;
+  __tsan_get_report_tag(report, &tag);
+  fprintf(stderr, "tag = %ld\n", tag);
+  // CHECK: tag = 1
+}
+
+// CHECK: Done.
+// CHECK: ThreadSanitizer: reported 1 warnings