[PATCH] D70440: [Driver] Use VFS to check if sanitizer blacklists exist

[Kadir Cetinkaya via Phabricator via llvm-commits]

kadircet added inline comments.


================
Comment at: clang/unittests/Driver/SanitizerArgsTest.cpp:37
+
+static constexpr const char *ClangPath = "/opt/llvm/bin/clang";
+static constexpr const char *InputFile = "/sources/foo.c";
----------------
can't this be just `clang` ? as we are passing the resource-dir explicitly anyway

would be nice to not depend on any relative path deduction in the test.


================
Comment at: clang/unittests/Driver/SanitizerArgsTest.cpp:44
+                                          std::vector<std::string> ExtraFiles) {
+    assert(!Driver && "Running emulateCompilation() twice is not allowed");
+
----------------
nit s/emulateCompilation/emulateSingleCompilation/


================
Comment at: clang/unittests/Driver/SanitizerArgsTest.cpp:53
+
+    std::vector<const char *> Args = {ClangPath, "-c", InputFile};
+    for (const auto &A : ExtraArgs)
----------------
nit: maybe put `InputFile` as the last entry in `Args` ?


================
Comment at: clang/unittests/Driver/SanitizerArgsTest.cpp:100
+              Contains(StrEq(std::string("-fsanitize-system-blacklist=") +
+                             ASanBlacklist)));
+  // User blacklists should also be added.
----------------
jkorous wrote:
> ilya-biryukov wrote:
> > I'm pretty sure this is going to fail on Windows, looking for ideas on how to unify this with minimal code...
> > Let me know if there's some cool trick for this that I should know...
> Would `llvm::sys::path::convert_to_slash()` help?
maybe make this explicit with smthng like `concat(ResourceDir, "share", "asan_blacklist.txt")` instead of `ASanBlackList`?


================
Comment at: clang/unittests/Driver/SanitizerArgsTest.cpp:100
+              Contains(StrEq(std::string("-fsanitize-system-blacklist=") +
+                             ASanBlacklist)));
+  // User blacklists should also be added.
----------------
kadircet wrote:
> jkorous wrote:
> > ilya-biryukov wrote:
> > > I'm pretty sure this is going to fail on Windows, looking for ideas on how to unify this with minimal code...
> > > Let me know if there's some cool trick for this that I should know...
> > Would `llvm::sys::path::convert_to_slash()` help?
> maybe make this explicit with smthng like `concat(ResourceDir, "share", "asan_blacklist.txt")` instead of `ASanBlackList`?
> Would llvm::sys::path::convert_to_slash() help?

+1 to this, maybe go over `Command.getArguments` and create a normalized version first, and then check over that one.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70440/new/

https://reviews.llvm.org/D70440