[PATCH] D28264: [tsan] Implement a 'ignore_noninstrumented_modules' flag to better suppress false positive races

[Kuba (Brecka) Mracek via Phabricator via llvm-commits]

kubabrecka added inline comments.


================
Comment at: lib/tsan/rtl/tsan_interceptors.cc:270
   if (flags()->ignore_interceptors_accesses) ThreadIgnoreBegin(thr_, pc_);
+  if (flags()->ignore_noninstrumented_modules &&
+      !libignore()->IsPcInstrumented(pc_)) {
----------------
dvyukov wrote:
> These changes to ScopedInterceptor does not feel right to me.
> We already have checks of libignore()->IsIgnored() and undo of ignoring in all the same places you added new code. So it is better to piggy back on the existing logic to avoid duplication. We can either (1) pass the new flag to IsIgnored:
> 
> if (!thr_->in_ignored_lib && libignore()->IsIgnored(pc, flags()->ignore_noninstrumented_modules))
> 
> then it will also check the pc against instrumented modules; or (2) collect instrumented modules in LibIgnore only if the flag is set, then IsIgnored will check:
> 
>   if pc is in ignored libraries { return true }
>   if instrumented libraries are not empty and pc is not in instrumented libraries { return true }
>   return false
> 
> Or (3) if the flag is set add non-instrumented libraries to ignored libraries.
> As far as I understand you don't want to do (3) because there can be lots of non-instrumented libraries.
> 
> 
I'd go with (2), but the current behavior of thr->in_ignored_lib being set is that *all* interceptors called when we're within an ignored module are ignored:

    #define SCOPED_TSAN_INTERCEPTOR(func, ...) \
    ... \
    if (!thr->is_inited || thr->ignore_interceptors || thr->in_ignored_lib) \
      return REAL(func)(__VA_ARGS__); \

This also means we're missing a lot of synchronization (mutexes, semaphores, etc.).  Is that behavior of thr->in_ignored_lib intentional?


Repository:
  rL LLVM

https://reviews.llvm.org/D28264