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

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline 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"))
----------------
george.karpenkov wrote:
> 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.
Asserts are usually off in shipping compilers. If an assumption is being violated, as a developer it's better than you know about it during debugging.


================
Comment at: clang/lib/Driver/ToolChains/Darwin.h:427
+  /// containing the sanitizer {@code SanitizerName}.
+  std::string sanitizerToRelativeLibName(StringRef SanitizerName,
+                                         bool Shared = true) const;
----------------
george.karpenkov wrote:
> 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.
The name implies that it's a file name and not a path. It could be `getSanitizerLibFileName()` or `getFileNameForSanitizerLib()` but that seemed a little verbose.


https://reviews.llvm.org/D15225