[openmp] Allow all possible argument separators in TSAN_OPTIONS

[upsj]

Summary

Currently, the parser used to tokenize the TSAN_OPTIONS in libomp uses
only spaces as separators, even though TSAN in compiler-rt supports
other separators like ':' or ','. This causes TSAN used with OpenMP to
spuriously display the following warning:

Warning: please export TSAN_OPTIONS='ignore_noninstrumented_modules=1'
to avoid false positive reports from the OpenMP runtime!

when TSAN_OPTIONS contains multiple arguments and
ignore_noninstrumented_modules follows a non-space separator.

CTest uses ':' to separate sanitizer options by default, which is
where I encountered this issue first in practice.
The documentation for other sanitizers mentions ':' as separator,
but TSAN only lists spaces, which is probably where this mismatch originated.

---

I wasn't sure where to best add tests for this bugfix, so I wanted to leave it at this stage until I can get some pointers in the right direction :)

[protze.joachim]

Thanks for submitting this patch!

The archer tool has it's own test sub-directory. I think, it's best to add the test there.

[protze.joachim]

Can you add a test?

[upsj]

Sure, I just had to figure out how the LLVM test system works :) (and fix an off-by-one error in the tokenization)
I added a new test with disabled suppression and modified the default suppression to see that the tokenization works.

[upsj]

Shorten test checks for clang-format

[protze.joachim]

LGTM

Can you push the patch or should I?

[upsj]

I don't have commit access, so you will have to do it.

openmp/tools/archer/ompt-tsan.cpp

*/
   //
   //===----------------------------------------------------------------------===//
 
 
 #ifndef __STDC_FORMAT_MACROS
 #define __STDC_FORMAT_MACROS
 #endif
 
+#include <algorithm>
 #include <atomic>
 #include <cassert>
 #include <cstdlib>
 #include <cstring>
 #include <inttypes.h>
 #include <iostream>
+#include <list>
 #include <mutex>
 #include <sstream>
 #include <stack>
-#include <list>
 #include <string>
-#include <iostream>
 #include <unordered_map>
 #include <vector>
 
 #if (defined __APPLE__ && defined __MACH__)
 #include <dlfcn.h>
 #endif
 
 #include <sys/resource.h>

 
 class TsanFlags {
 public:
   int ignore_noninstrumented_modules;
 
   TsanFlags(const char *env) : ignore_noninstrumented_modules(0) {
     if (env) {
       std::vector<std::string> tokens;
-      std::string token;
       std::string str(env);
-      std::istringstream iss(str);
-      while (std::getline(iss, token, ' '))
-        tokens.push_back(token);
+      auto end = str.end();
+      auto it = str.begin();
+      auto is_sep = [](char c) {
+        return c == ' ' || c == ',' || c == ':' || c == '\n' || c == '\t' ||
+               c == '\r';
+      };
+      while (it != end) {
+        auto next_it = std::find_if(it, end, is_sep);
+        tokens.emplace_back(it, next_it);
+        it = next_it;
+        if (it != end) {
+          ++it;
+        }
+      }
 
-      for (std::vector<std::string>::iterator it = tokens.begin();
-           it != tokens.end(); ++it) {
+      for (const auto &token : tokens) {
         // we are interested in ignore_noninstrumented_modules to print a
         // warning
-        if (sscanf(it->c_str(), "ignore_noninstrumented_modules=%d",
+        if (sscanf(token.c_str(), "ignore_noninstrumented_modules=%d",
                    &ignore_noninstrumented_modules))
           continue;
       }
     }
   }
 };
 
 #if (LLVM_VERSION) >= 40

openmp/tools/archer/tests/lit.cfg

 
 # to run with icc INTEL_LICENSE_FILE must be set
 if 'INTEL_LICENSE_FILE' in os.environ:
     config.environment['INTEL_LICENSE_FILE'] = os.environ['INTEL_LICENSE_FILE']
 
 # Race Tests
 config.substitutions.append(("%libarcher-compile-and-run-race", \
     "%libarcher-compile && %libarcher-run-race"))
+config.substitutions.append(("%libarcher-compile-and-run-nosuppression", \
+                             "%libarcher-compile && %libarcher-run-nosuppression"))
 config.substitutions.append(("%libarcher-compile-and-run", \
                              "%libarcher-compile && %libarcher-run"))
 config.substitutions.append(("%libarcher-cxx-compile-and-run", \
     "%libarcher-cxx-compile && %libarcher-run"))
 config.substitutions.append(("%libarcher-cxx-compile", \
     "%clang-archerXX %openmp_flags %archer_flags %flags -std=c++14 %s -o %t" + libs))
 config.substitutions.append(("%libarcher-compile", \
                              "%clang-archer %openmp_flags %archer_flags %flags %s -o %t" + libs))
 config.substitutions.append(("%libarcher-run-race", "%suppression %deflake %t 2>&1 | tee %t.log"))
+config.substitutions.append(("%libarcher-run-nosuppression", "%nosuppression %t 2>&1 | tee %t.log"))
 config.substitutions.append(("%libarcher-run", "%suppression %t 2>&1 | tee %t.log"))
 config.substitutions.append(("%clang-archerXX", config.test_cxx_compiler))
 config.substitutions.append(("%clang-archer", config.test_c_compiler))
 config.substitutions.append(("%openmp_flags", config.test_openmp_flags))
 config.substitutions.append(("%archer_flags", config.archer_flags))
 config.substitutions.append(("%flags", config.test_flags))
-config.substitutions.append(("%suppression", "env TSAN_OPTIONS='ignore_noninstrumented_modules=1'"))
+config.substitutions.append(("%nosuppression", "env TSAN_OPTIONS='ignore_noninstrumented_modules=0'"))
+config.substitutions.append(("%suppression", "env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1'"))
 config.substitutions.append(("%deflake", os.path.join(os.path.dirname(__file__), "deflake.bash")))
 
 config.substitutions.append(("FileCheck", config.test_filecheck))
 config.substitutions.append(("%not", config.test_not))
 config.substitutions.append(("%sort-threads", "sort --numeric-sort --stable"))
 if config.operating_system == 'Windows':
     # No such environment variable on Windows.
     config.substitutions.append(("%preload-tool", "true ||"))
 elif config.operating_system == 'Darwin':
     config.substitutions.append(("%preload-tool", "env DYLD_INSERT_LIBRARIES=%T/tool.so"))
 else:
     config.substitutions.append(("%preload-tool", "env LD_PRELOAD=%T/tool.so"))

openmp/tools/archer/tests/parallel/parallel-nosuppression.c

 /*
- * parallel-simple.c -- Archer testcase
+ * parallel-nosuppression.c -- Archer testcase
  */
 
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 //
 // See tools/archer/LICENSE.txt for details.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 
-// RUN: %libarcher-compile-and-run | FileCheck %s
+// RUN: %libarcher-compile-and-run-nosuppression | FileCheck %s
 // REQUIRES: tsan
 #include <omp.h>
 #include <stdio.h>
 
 int main(int argc, char *argv[]) {
   int var = 0;
 
 #pragma omp parallel num_threads(2) shared(var)
   {
     if (omp_get_thread_num() == 1) {
       var++;
     }
   } // implicit barrier
 
   var++;
 
   fprintf(stderr, "DONE\n");
   int error = (var != 2);
   return error;
 }
 
 // CHECK-NOT: ThreadSanitizer: data race
 // CHECK-NOT: ThreadSanitizer: reported
+// CHECK: Warning: please export TSAN_OPTIONS
 // CHECK: DONE

openmp/tools/archer/tests/parallel/parallel-simple.c

#pragma omp parallel num_threads(2) shared(var)
 
   fprintf(stderr, "DONE\n");
   int error = (var != 2);
   return error;
 }
 
 // CHECK-NOT: ThreadSanitizer: data race
 // CHECK-NOT: ThreadSanitizer: reported
+// CHECK-NOT: Warning: please export TSAN_OPTIONS
 // CHECK: DONE