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

[Dan Liew via Phabricator via llvm-commits]

delcypher added inline comments.


================
Comment at: test/tsan/Darwin/external-swift-debugging.cc:39
+  });
+  // CHECK: WARNING: ThreadSanitizer: Swift access race
+  // CHECK: Modifying access of Swift variable at {{.*}} by thread {{.*}}
----------------
george.karpenkov wrote:
> kubamracek wrote:
> > delcypher wrote:
> > > kubamracek wrote:
> > > > delcypher wrote:
> > > > > I don't understand this. There's no Swift code in this test case so how is this a Swift access race?
> > > > The test is faking what a Swift code would do via the `__tsan_external_write` call.
> > > 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` ).
> > I don't want to expand the scope of this patch. Let's strictly focus on the added API.
> @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.
@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.


================
Comment at: test/tsan/Darwin/external-swift-debugging.cc:20
+void ExternalWrite(void *addr) {
+  static void *kSwiftAccessRaceTag = (void *)0x1;
+  __tsan_external_write(addr, nullptr, kSwiftAccessRaceTag);
----------------
@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.


https://reviews.llvm.org/D46661