[PATCH] D28836: [tsan] Provide API for libraries for race detection on custom objects

[Dmitry Vyukov via Phabricator via llvm-commits]

dvyukov added a comment.

> I do already have a buy in from 2 major libraries on our side and we're in the talks with a 3rd library. The library owners did preliminary performance tests and they are fine with inserting those callbacks. Sorry for being vague here :)

OK, looks reasonable then.

But please post full reports for the tests.

This is first round of comments. Need to figure out what happens when this new callbacks race with the old callback and/or tag is not assigned, or old callbacks + tag.



================
Comment at: lib/tsan/rtl/tsan_external.cc:22
+
+uptr CreateTag(const char *library_name, const char *object_type) {
+  CHECK_LT(used_tags, kMaxTag);
----------------
Inline this into __tsan_external_register_tag, as that's the only caller and the only thing it does is calling this function.


================
Comment at: lib/tsan/rtl/tsan_external.cc:25
+  uptr new_tag = used_tags;
+  used_tags++;
+  registered_tags[new_tag].library_name = internal_strdup(library_name);
----------------
This is, well, a data race.
Use an atomic for tag allocation.


================
Comment at: lib/tsan/rtl/tsan_external.cc:32
+TagDescription *TagDescriptionFromTag(uptr tag) {
+  return &registered_tags[tag];
+}
----------------
Needs a CHECK that tag is correct. Or probably better to return NULL and let callers deal with absence of tag (treat the same as tag==0). The header potentially can be corrupted due to e.g. data races with tag assignment, or block freeing in a different thread.


================
Comment at: lib/tsan/rtl/tsan_external.cc:42
+SANITIZER_INTERFACE_ATTRIBUTE
+void __tsan_external_assign_tag(void *addr, void *tag) {
+  Allocator *a = allocator();
----------------
This needs a CHECK that tag was previously allocated.


================
Comment at: lib/tsan/rtl/tsan_external.cc:55
+  
+SANITIZER_INTERFACE_ATTRIBUTE void __tsan_external_read(void *addr, void *caller_pc, void *tag) {
+  ThreadState *thr = cur_thread();
----------------
caller_pc is unsed
80 columns please


================
Comment at: lib/tsan/rtl/tsan_external.cc:58
+  thr->is_external_race = true;
+  thr->external_tag = (uptr)tag;
+  MemoryRead(thr, CALLERPC, (uptr)addr, kSizeLog8);
----------------
I would prefer if is_external_race/external_tag into a single variable. We already have tag==0 reserved for "no tag". So it looks reasonable to just threat external_tag!=0 as is_external_race.


================
Comment at: lib/tsan/rtl/tsan_external.cc:64
+  
+SANITIZER_INTERFACE_ATTRIBUTE void __tsan_external_write(void *addr, void *caller_pc, void *tag) {
+  ThreadState *thr = cur_thread();
----------------
ditto


================
Comment at: lib/tsan/rtl/tsan_external.h:20
+
+static const uptr kMaxTag = 128;  // Limited to 65,536, since MBlock only stores
+                                  // tags as 16-bit values, see tsan_defs.h.
----------------
This does not need to be in header.

p.s. consts are static by default


================
Comment at: lib/tsan/rtl/tsan_external.h:27
+
+uptr CreateTag(const char *library_name, const char *object_type);
+TagDescription *TagDescriptionFromTag(uptr tag);
----------------
This does not need to be in header.

TagDescription will go away as well, if we do just 1 string.

What's left is only TagDescriptionFromTag, so I would just declare it in tsan_rtl.h.


Repository:
  rL LLVM

https://reviews.llvm.org/D28836