[PATCH] D31630: [tsan] Detect races on modifying accesses in Swift code

Dmitry Vyukov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Apr 21 06:42:58 PDT 2017


dvyukov added a comment.

I don't like that we sprinkle swift-specific bits throughout the code when the external API was meant to be general enough to cover all such cases.
We have the special tag for swift, so it really seems to me that it should be the only swift-specific bit here and the rest must be handled by the external API on general basis. We have 3 potential users for the external API (Go, Java, races on fd, potentially more in future). So I would be really interested in making the external API general and flexible enough to support all of them, rather than later sprinkle Go/Java/fd/etc-specific bits throughout the code again later.

Why do we say "Mutating" for external and "Modifying" for swift? Please settle on 1 form.
Why is the tag for switch includes "modifying"? Modifying/non-modifying is orthogonal to the object type and is controlled by __tsan_external_read/write. It really should be kExternalTagSwiftAccess.
If we want to say "swift" in header line, why don't we want the same level of support for other external objects?

I propose we say for usual data races:

ThreadSanitizer: data race

  Write of size X at P by S
  Previous write of size X at P by S

and for external accesses:

ThreadSanitizer: race on TAG_NAME

  Mutating access at P by S
  Previous mutating access at P by S

(or Modifying, I don't care too much)

This will allow us to have:

ThreadSanitizer: race on file descriptor
ThreadSanitizer: race on curl handle
ThreadSanitizer: race on Swift array
ThreadSanitizer: race on Swift map
ThreadSanitizer: race on Go map

Also, could swift runtime register a tag at startup? Or performance overhead is a concern?



================
Comment at: test/tsan/Darwin/external-swift.cc:10
+extern "C" {
+void *__tsan_external_register_tag(const char *object_type);
+void *__tsan_external_assign_tag(void *addr, void *tag);
----------------
Declare them in test.h. They will be used by other tests in future.


https://reviews.llvm.org/D31630





More information about the llvm-commits mailing list