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

[Dan Liew via Phabricator via cfe-commits]

delcypher requested changes to this revision.
delcypher added a comment.
This revision now requires changes to proceed.

Seems mostly fine apart from some minor nits.

If I'm honest I don't see any reason why this should be Darwin specific. Sure the naming convention and location of the runtime libraries is different for other platforms but other than that the same logic used here is applicable.
I don't feel that strongly about it and we could refactor later if other platforms need this if you really don't want to support this for other platforms.



================
Comment at: clang/lib/Driver/ToolChains/Darwin.h:134
+  /// \return Directory to find the runtime library in.
+  SmallString<128> runtimeLibDir(bool IsEmbedded=false) const;
+
----------------
Why is this a `SmallString<128>` but in other places we're just using `std::string`?


================
Comment at: clang/lib/Driver/ToolChains/Darwin.h:427
+  /// containing the sanitizer {@code SanitizerName}.
+  std::string sanitizerToRelativeLibName(StringRef SanitizerName,
+                                         bool Shared = true) const;
----------------
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?


================
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
 
----------------
Are these `-resource-dir %S/Inputs/resource_dir` needed because compiler-rt might not be present and now the driver checks for these?


https://reviews.llvm.org/D15225