[PATCH] D15225: [Driver] Sanitizer support based on runtime library presence

[Alexey Samsonov via cfe-commits]

samsonov added inline comments.

================
Comment at: include/clang/Driver/ToolChain.h:410
@@ -407,1 +409,3 @@
+  /// (don't require runtime library).
+  virtual SanitizerMask getSanitizersRequiringTrap() const;
 };
----------------
I don't think this is relevant to ToolChain at all. `SanitizerArgs` has `TrappingSupported` mask, which is what you should need: if you don't have runtimes for all sanitizers you're enabling, and *some* of these sanitizers support trapping, driver may advise to use this flag.

================
Comment at: lib/Driver/ToolChains.cpp:360
@@ +359,3 @@
+  StringRef OS = "";
+  if (isTargetMacOS()) OS = "osx";
+  if (isTargetWatchOSSimulator()) OS = "watchossim";
----------------
Can we teach ToolChain (or at least Darwin toolchain) to return us OS name instead?

================
Comment at: lib/Driver/ToolChains.cpp:369
@@ +368,3 @@
+
+  return (Twine("libclang_rt.") + Sanitizer + "_" + OS + "_dynamic.dylib")
+      .str();
----------------
Oh no, please don't return a `StringRef` which points to temporary.

================
Comment at: test/Driver/fsanitize.c:14
@@ +13,3 @@
+// RUN: %clang -target x86_64-apple-darwin10 -resource-dir=%S/Inputs/resource_dir -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-DARWIN1
+// CHECK-UNDEFINED-DARWIN1: error: '-fsanitize-trap=undefined' required with '-fsanitize=undefined' option
+// RUN: %clang -target x86_64-apple-darwin10 -resource-dir=%S/Inputs/resource_dir_darwin_sanitizers -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-DARWIN2
----------------
I find this diagnostic misleading :-/ It's not the case that `-fsanitize=undefined` *requires* you to additionally provide `-fsanitize-trap=undefined`. It requires having a UBSan runtime library available. Or, if you can't provide it (it's not available on your system), you can workaround this by using `-fsanitize-trap`. The latter should be a suggestion, really.

================
Comment at: test/Driver/fsanitize.c:33
@@ -30,3 +32,3 @@
 
-// RUN: %clang -fsanitize=bounds -### -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDS
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=bounds -### -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDS
 // CHECK-BOUNDS: "-fsanitize={{((array-bounds|local-bounds),?){2}"}}
----------------
Nice catch, please commit this fix as a separate change.

================
Comment at: test/Driver/fsanitize.c:214
@@ -205,3 +213,3 @@
 
-// RUN: %clang -target x86_64-apple-darwin10 -fsanitize=memory %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-MSAN-DARWIN
+// RUN: %clang -target x86_64-apple-darwin10 -resource-dir=%S/Inputs/resource_dir -fsanitize=memory %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-MSAN-DARWIN
 // CHECK-MSAN-DARWIN: unsupported option '-fsanitize=memory' for target 'x86_64-apple-darwin10'
----------------
Why not resource_dir_darwin_sanitizers here and below?

================
Comment at: test/Driver/fsanitize.c:221
@@ +220,3 @@
+// RUN: %clang -target x86_64-apple-darwin10 -resource-dir=%S/Inputs/resource_dir -fsanitize=memory -fsanitize=thread,memory %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-MSAN-TSAN-MSAN-DARWIN1
+// CHECK-MSAN-TSAN-MSAN-DARWIN1: unsupported option '-fsanitize=thread,memory' for target 'x86_64-apple-darwin10'
+// CHECK-MSAN-TSAN-MSAN-DARWIN1-NOT: unsupported option
----------------
Again, I feel like we're lying to users here: `-fsanitize=thread` *is* supported for this target, it just requires building a runtime.


http://reviews.llvm.org/D15225