[PATCH] D31553: [tsan] Ignore memory accesses for libignored modules for "external" races

[Dmitry Vyukov via Phabricator via llvm-commits]

dvyukov accepted this revision.
dvyukov added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lib/tsan/rtl/tsan_external.cc:61
   thr->external_tag = (uptr)tag;
-  FuncEntry(thr, (uptr)caller_pc);
-  MemoryRead(thr, CALLERPC, (uptr)addr, kSizeLog8);
-  FuncExit(thr);
+  if (caller_pc) FuncEntry(thr, (uptr)caller_pc);
+  bool in_ignored_lib;
----------------
I think I understand why I was confused. I assumed that caller_pc is literally caller pc. And caller can well point into the same library, and then this all does not make sense. But you seem to assume that caller_pc points user code (can be caller pc or several frames above that).

We now have include/sanitizer/tsan_interface.h, so please add __tsan_external_* functions there with proper comments that document these assumptions.


================
Comment at: lib/tsan/rtl/tsan_external.cc:64
+  if (!caller_pc || !libignore()->IsIgnored((uptr)caller_pc, &in_ignored_lib)) {
+    MemoryRead(thr, CALLERPC, (uptr)addr, kSizeLog8);
+  }
----------------
Unrelated to this change, but why did we decide to use kSizeLog8? It would be more reasonable to use kSizeLog1. A library may need several different addresses to model several unrelated things and if the object is only 8 bytes, it won't be able to do so. Also if size of the object is only, say, 4 bytes, 8-byte access can lead to false positives.
Is there any reason to use 8? If not, please change it to 1.



Repository:
  rL LLVM

https://reviews.llvm.org/D31553