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

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

@delcypher sorry i don't agree with the change requests.
@mracek any comments?



================
Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2298
   SanitizerMask Res = ToolChain::getSupportedSanitizers();
-  Res |= SanitizerKind::Address;
-  Res |= SanitizerKind::Leak;
-  Res |= SanitizerKind::Fuzzer;
-  Res |= SanitizerKind::FuzzerNoLink;
+
+  if (sanitizerRuntimeExists("asan"))
----------------
delcypher wrote:
> I feel that we should assert that `Res` doesn't already contain the SanitizerKind we are decided whether or not to set.
> E.g.
> 
> ```
> assert(!(Res & SanitizerKind::Address));
> if (sanitizerRuntimeExists("asan")) {
>   Res |= SanitizerKind::Address;
> }
> ```
I'm not sure it would be worth crashing the driver.


================
Comment at: clang/lib/Driver/ToolChains/Darwin.h:134
+  /// \return Directory to find the runtime library in.
+  SmallString<128> runtimeLibDir(bool IsEmbedded=false) const;
+
----------------
delcypher wrote:
> Why is this a `SmallString<128>` but in other places we're just using `std::string`?
For driver it does not matter which datastructure to use.
Here SmallString is used because that's what is more convenient to return.


================
Comment at: clang/lib/Driver/ToolChains/Darwin.h:427
+  /// containing the sanitizer {@code SanitizerName}.
+  std::string sanitizerToRelativeLibName(StringRef SanitizerName,
+                                         bool Shared = true) const;
----------------
delcypher wrote:
> I don't like this name very much. Given that the path is relative to the directory containing the library, what this function really does is given the **file name** for a sanitizer library. Mentioning "relative" is just confusing.
> 
> Wouldn't something like `getSanitizerLibName()` or `getNameForSanitizerLib()` be much clearer?
Then it's not clear whether the returned path is relative or absolute though.


================
Comment at: clang/test/Driver/darwin-asan-nofortify.c:3
 
-// RUN: %clang -fsanitize=address  %s -E -dM -target x86_64-darwin | FileCheck %s
+// RUN: %clang -fsanitize=address  %s -E -dM -target x86_64-darwin -resource-dir %S/Inputs/resource_dir | FileCheck %s
 
----------------
delcypher wrote:
> Are these `-resource-dir %S/Inputs/resource_dir` needed because compiler-rt might not be present and now the driver checks for these?
By default -resource-dir points to the resource dir in the "bin" directory. The contents of that is sort of random, and depends on what ninja commands have you ran before.


https://reviews.llvm.org/D15225