[TSan] Remove ignore_interceptors_accesses flag

[yln]

Summary

It has been superseded by the ignore_noninstrumented_modules flag and is no longer needed.

Also simplify a test that checks that mmap_interceptor respects ignore annotations (thr->ignore_reads_and_writes ).
Relevant: https://reviews.llvm.org/rL269855

rdar://problem/46263073 Remove obsolete Apple-specific suppression option

[yln]

Use std::atomic to make test cleaner

[dvyukov]

compiler-rt/test/tsan/ignored-interceptors-mmap.cc

53 ↗

(On Diff #175978)

Are asserts even enabled in all builds? I would just use if.

[yln]

compiler-rt/test/tsan/ignored-interceptors-mmap.cc

53 ↗

(On Diff #175978)

This is not a test assert/check but rather an assert to ensure the sanity of the test itself.
The actual test checks are below (the FileCheck directives). Note both expect "OK", i.e., the program completes and not aborted via the assert. So I am not too sure would you would put into the if?
Let me know if you have an idea how to better express my intent here.

[dvyukov]

compiler-rt/test/tsan/ignored-interceptors-mmap.cc

53 ↗

(On Diff #175978)

OK, I see assert is used in other tests. So let's leave it.
There is usually no difference between thing-under-test failing, or the test itself failing -- in both cases the test needs to fail and a human needs to take a look. A filecheck failure can indicate both as well for most of our tests.

[yln]

No change in functionality

lib/tsan/rtl/tsan_flags.inc

 TSAN_FLAG(int, io_sync, 1,
           "Controls level of synchronization implied by IO operations. "
           "0 - no synchronization "
           "1 - reasonable level of synchronization (write->read)"
           "2 - global synchronization of all IO operations.")
 TSAN_FLAG(bool, die_after_fork, true,
           "Die after multi-threaded fork if the child creates new threads.")
 TSAN_FLAG(const char *, suppressions, "", "Suppressions file name.")
-TSAN_FLAG(bool, ignore_interceptors_accesses, false,
-          "Ignore reads and writes from all interceptors.")
 TSAN_FLAG(bool, ignore_noninstrumented_modules, SANITIZER_MAC ? true : false,
           "Interceptors should only detect races when called from instrumented "
           "modules.")
 TSAN_FLAG(bool, shared_ptr_interceptor, true,
           "Track atomic reference counting in libc++ shared_ptr and weak_ptr.")

lib/tsan/rtl/tsan_interceptors.cc

 ScopedInterceptor::ScopedInterceptor(ThreadState *thr, const char *fname,
                                      uptr pc)
     : thr_(thr), pc_(pc), in_ignored_lib_(false), ignoring_(false) {
   Initialize(thr);
   if (!thr_->is_inited) return;
   if (!thr_->ignore_interceptors) FuncEntry(thr, pc);
   DPrintf("#%d: intercept %s()\n", thr_->tid, fname);
   ignoring_ =
-      !thr_->in_ignored_lib && (flags()->ignore_interceptors_accesses ||
-                                libignore()->IsIgnored(pc, &in_ignored_lib_));
+      !thr_->in_ignored_lib && libignore()->IsIgnored(pc, &in_ignored_lib_);
   EnableIgnores();
 }
 
 ScopedInterceptor::~ScopedInterceptor() {
   if (!thr_->is_inited) return;
   DisableIgnores();
   if (!thr_->ignore_interceptors) {
     ProcessPendingSignals(thr_);

test/tsan/Darwin/ignore-noninstrumented.mm

-// Check that ignore_noninstrumented_modules=1 suppresses races from system libraries on OS X.
+// Check that ignore_noninstrumented_modules=1 suppresses reporting races from
+// system libraries on OS X. There are currently false positives coming from
+// libxpc, libdispatch, CoreFoundation and others, because these libraries use
+// TSan-invisible atomics as synchronization.
 
 // RUN: %clang_tsan %s -o %t -framework Foundation
 
 // Check that without the flag, there are false positives.
 // RUN: %env_tsan_opts=ignore_noninstrumented_modules=0 %deflake %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-RACE
 
 // With ignore_noninstrumented_modules=1, no races are reported.
 // RUN: %env_tsan_opts=ignore_noninstrumented_modules=1 %run %t 2>&1 | FileCheck %s

test/tsan/Darwin/ignored-interceptors.mm

-// Check that ignore_interceptors_accesses=1 suppresses reporting races from
-// system libraries on OS X. There are currently false positives coming from
-// libxpc, libdispatch, CoreFoundation and others, because these libraries use
-// TSan-invisible atomics as synchronization.
-
-// RUN: %clang_tsan %s -o %t -framework Foundation
-
-// Check that without the flag, there are false positives.
-// RUN: %env_tsan_opts=ignore_noninstrumented_modules=0 %deflake %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-RACE
-
-// With ignore_interceptors_accesses=1, no races are reported.
-// RUN: %env_tsan_opts=ignore_noninstrumented_modules=0:ignore_interceptors_accesses=1 %run %t 2>&1 | FileCheck %s
-
-// With ignore_interceptors_accesses=1, races in user's code are still reported.
-// RUN: %env_tsan_opts=ignore_noninstrumented_modules=0:ignore_interceptors_accesses=1 %deflake %run %t race 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-RACE
-
-#import <Foundation/Foundation.h>
-
-#import "../test.h"
-
-long global;
-
-void *Thread1(void *x) {
-  barrier_wait(&barrier);
-  global = 42;
-  return NULL;
-}
-
-void *Thread2(void *x) {
-  global = 43;
-  barrier_wait(&barrier);
-  return NULL;
-}
-
-int main(int argc, char *argv[]) {
-  fprintf(stderr, "Hello world.\n");
-  
-  // NSUserDefaults uses XPC which triggers the false positive.
-  NSDictionary *d = [[NSUserDefaults standardUserDefaults] dictionaryRepresentation];
-
-  if (argc > 1 && strcmp(argv[1], "race") == 0) {
-    barrier_init(&barrier, 2);
-    pthread_t t[2];
-    pthread_create(&t[0], NULL, Thread1, NULL);
-    pthread_create(&t[1], NULL, Thread2, NULL);
-    pthread_join(t[0], NULL);
-    pthread_join(t[1], NULL);
-  }
-
-  fprintf(stderr, "Done.\n");
-}
-
-// CHECK: Hello world.
-// CHECK-RACE: SUMMARY: ThreadSanitizer: data race
-// CHECK: Done.

test/tsan/ignored-interceptors-mmap.cc

 // RUN: %clangxx_tsan -O0 %s -o %t
-// RUN: not %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-NORMAL
-// RUN: %env_tsan_opts=ignore_interceptors_accesses=1 %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-IGNORE
+// RUN: not %run %t        2>&1 | FileCheck %s --check-prefix=CHECK-RACE
+// RUN:     %run %t ignore 2>&1 | FileCheck %s --check-prefix=CHECK-IGNORE
 // XFAIL: freebsd,netbsd
 
-#include <errno.h>
 #include <sys/mman.h>
+#include <string.h>
+#include <assert.h>
+#include <atomic>
 
 #include "test.h"
 
 extern "C" {
 void AnnotateIgnoreReadsBegin(const char *f, int l);
 void AnnotateIgnoreReadsEnd(const char *f, int l);
 void AnnotateIgnoreWritesBegin(const char *f, int l);
 void AnnotateIgnoreWritesEnd(const char *f, int l);
 }
 
-void *global_p;
+// Use atomic to ensure we do not have a race for the pointer value itself.  We
+// only want to check races in the mmap'd memory to isolate the test that mmap
+// respects ignore annotations.
+std::atomic<int*> global_p;
 
-int mmap_and_ignore_reads_and_writes() {
+void mmap_ignored(bool ignore) {
   const size_t kSize = sysconf(_SC_PAGESIZE);
-  void *p = mmap(0, kSize, PROT_READ|PROT_WRITE,
-                 MAP_PRIVATE|MAP_ANON, -1, 0);
-  if (p == MAP_FAILED)
-    return printf("mmap failed with %d\n", errno);
-  munmap(p, kSize);
-
-  void *new_p = mmap(p, kSize, PROT_READ|PROT_WRITE,
-                     MAP_PRIVATE|MAP_ANON, -1, 0);
-  if (p == MAP_FAILED || p != new_p)
-    return printf("second mmap failed with %d\n", errno);
-
-  AnnotateIgnoreWritesBegin(__FILE__, __LINE__);
-  global_p = p;
-  AnnotateIgnoreWritesEnd(__FILE__, __LINE__);
+
+  if (ignore) AnnotateIgnoreWritesBegin(__FILE__, __LINE__);
+  void *p = mmap(0, kSize, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANON, -1, 0);
+  if (ignore) AnnotateIgnoreWritesEnd(__FILE__, __LINE__);
+
+  // Use relaxed to retain the race between the mmap call and the memory write
+  global_p.store((int *)p, std::memory_order_relaxed);
   barrier_wait(&barrier);
-  return 0;
 }
 
-void *Thread(void *a) {
+void *WriteToMemory(void *unused) {
   barrier_wait(&barrier);
-
-  ((int*)global_p)[1] = 10;
-  printf("Read the zero value from mmapped memory %d\n", ((int*)global_p)[1]);
+  global_p[0] = 7;
   return 0;
 }
 
-int main() {
+// Create race between allocating (mmap) and writing memory
+int main(int argc, const char *argv[]) {
+  bool ignore = (argc > 1) && (strcmp(argv[1], "ignore") == 0);
+
   barrier_init(&barrier, 2);
   pthread_t t;
-  pthread_create(&t, 0, Thread, 0);
-  if (mmap_and_ignore_reads_and_writes())
-    return 1;
+  pthread_create(&t, 0, WriteToMemory, 0);
+  mmap_ignored(ignore);
   pthread_join(t, 0);
+
+  assert(global_p[0] == 7);
   printf("OK\n");
   return 0;
 }
 
-// CHECK-NORMAL: WARNING: ThreadSanitizer: data race
-// CHECK-NORMAL: OK
-// CHECK-IGNORE_NOT: WARNING: ThreadSanitizer: data race
+// CHECK-RACE: WARNING: ThreadSanitizer: data race
+// CHECK-RACE: OK
+// CHECK-IGNORE-NOT: WARNING: ThreadSanitizer: data race
 // CHECK-IGNORE: OK