Add VFS support for sanitizers' blacklist' 2

[jkorous]

Summary

This is another approach to using VFS for blacklists.

The previous one got reverted as it didn't work with in-memory file systems.
https://reviews.llvm.org/D67742

Adding original reviewers + people who got broken by the previous version.

[jkorous]

llvm/lib/Support/SpecialCaseList.cpp

120

Technically, this isn't really necessary and could be split in a separate patch - I just kept it here so the motivation is obvious.

[ilya-biryukov]

clang/include/clang/Basic/SanitizerSpecialCaseList.h

18

NIT: #include "llvm/Support/VirtualFileSystem.h" should be enough

clang/lib/Basic/SanitizerSpecialCaseList.cpp

26

Why not pass VFS to the SpecialCaseList::createInternal instead?
We're duplicating the code of createInternal here.

27

NIT: maybe use auto. The type is quite long, spelling it out seems to hurt readability

29

NIT: it's probably more idiomatic to use a boolean conversion here:

if (!MaybeBuffer) {
  Error = (Twine("can't open file '") + Path + "': " + MaybeBuffer.getError().message()).str();
  return;
}

But up to you, obviously

[jkorous]

clang/lib/Basic/SanitizerSpecialCaseList.cpp

26

I intended to keep the scope of this patch small but open to discussion. Seems like the only other place where SpecialCaseList is used is XRayFunctionFilter. The options I see here are:

1. Get rid of anything filesystem-related in SpecialCaseList and just require buffers in the interface.
2. Require VFS in SpecialCaseList interface and pass it from XRayFunctionFilter (seems like VFS would be used just in the constructor and not in the actual methods?).
3. Add VFS to only some method/s in SpecialCaseList. I'd consider this a regression in interface "quality".

I'd prefer #1 and unless someone can confirm that the XRay change is trivial I'd prefer to not do #2.

27

I feel that spelling the type out here makes it a bit easier to understand what's going on later (MaybeBuffer.get().get()).

29

Thanks! I copied this from SpecialCaseList. Let's discuss eventual changes to SpecialCaseList interface first, I'd change this after.

[ilya-biryukov]

clang/lib/Basic/SanitizerSpecialCaseList.cpp

26

I would argue the minimal change is actually #2.

createInternal(std::vector<string> Paths)

gets turned into

createInternal(std::vector<string> Paths, vfs::FileSystem *FS = getRealFileSystem());

The implementation is updated accordingly, callsites in XRay don't need an update.

[jkorous]

I see what you mean - I didn't realize I could use the real fs as the default arg.
I still feel that handling files is out of scope for SpecialCaseList but I don't think those couple lines are a big deal either way - updating the patch now.

[ilya-biryukov]

llvm/lib/Support/SpecialCaseList.cpp

102

Should this call into VFS?

[ilya-biryukov]

Do the tests pass with the current revision?
Since we're not actually calling into the VFS, is there a chance we're not actually properly testing this?

[jkorous]

llvm/lib/Support/SpecialCaseList.cpp

102

Good catch! Thanks.

[jkorous]

Actually use the VFS that got passed.

[jkorous]

The test seems to be fine (fails on master, passes with the patch). I most probably just forgot to run it.

[ilya-biryukov]

LGTM

[ilya-biryukov]

Just wanted to give early warning that this might still break our integrate and buildcops might be keen to revert it. The change itself LG, but our infrastructure might need time to pick it up and get broken for some time.
We're discussing how to properly land this internally, so hopefully this shouldn't happen, but still wanted to make sure that does not come as a surprise.

[jkorous]

I am in no particular rush and happy to help you as much as I can but at some point not too far in the future I just want this to land and not get reverted.

Is there anything I can do to help you with this? Can't you guys just cherry-pick the patch internally, do whatever needs to be done on your side and ping me when it's safe to land?

[ilya-biryukov]

It seems we should be fine, now that the file accesses actually go through the VFS.
Feel free to submit the patch and sorry for the disturbance!

[ilya-biryukov]

It turned out there are more places that attempt to construct SpecialCaseList and access corresponding file via the usual filesystem APIs.
D70440 should take care of those.

[jkorous]

Ahh, right. I originally wanted to support -ivfsoverlay in Driver as it seemed reasonable to check the existence of blacklist in Driver and open it in cc1 using the same fs. I got talked out of it and I didn't touch the Driver but seems like I should've replaced the calls to native fs with VFS to keep things consistent. Thanks for picking this up!

clang/include/clang/Basic/SanitizerSpecialCaseList.h

 // SanitizerMask.
 //
 //===----------------------------------------------------------------------===//
 #ifndef LLVM_CLANG_BASIC_SANITIZERSPECIALCASELIST_H
 #define LLVM_CLANG_BASIC_SANITIZERSPECIALCASELIST_H
 
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/Sanitizers.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/SpecialCaseList.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include <memory>
 
 namespace clang {
 
 class SanitizerSpecialCaseList : public llvm::SpecialCaseList {
 public:
   static std::unique_ptr<SanitizerSpecialCaseList>
-  create(const std::vector<std::string> &Paths, std::string &Error);
+  create(const std::vector<std::string> &Paths, llvm::vfs::FileSystem &VFS,
+         std::string &Error);
 
   static std::unique_ptr<SanitizerSpecialCaseList>
-  createOrDie(const std::vector<std::string> &Paths);
+  createOrDie(const std::vector<std::string> &Paths,
+              llvm::vfs::FileSystem &VFS);
 
   // Query blacklisted entries if any bit in Mask matches the entry's section.
   bool inSection(SanitizerMask Mask, StringRef Prefix, StringRef Query,
                  StringRef Category = StringRef()) const;
 
 protected:
   // Initialize SanitizerSections.
   void createSanitizerSections();

clang/lib/Basic/SanitizerBlacklist.cpp

 //
 //===----------------------------------------------------------------------===//
 #include "clang/Basic/SanitizerBlacklist.h"
 
 using namespace clang;
 
 SanitizerBlacklist::SanitizerBlacklist(
     const std::vector<std::string> &BlacklistPaths, SourceManager &SM)
-    : SSCL(SanitizerSpecialCaseList::createOrDie(BlacklistPaths)), SM(SM) {}
+    : SSCL(SanitizerSpecialCaseList::createOrDie(
+          BlacklistPaths, SM.getFileManager().getVirtualFileSystem())),
+      SM(SM) {}
 
 bool SanitizerBlacklist::isBlacklistedGlobal(SanitizerMask Mask,
                                              StringRef GlobalName,
                                              StringRef Category) const {
   return SSCL->inSection(Mask, "global", GlobalName, Category);
 }
 
 bool SanitizerBlacklist::isBlacklistedType(SanitizerMask Mask,

clang/lib/Basic/SanitizerSpecialCaseList.cpp

 //
 //===----------------------------------------------------------------------===//
 #include "clang/Basic/SanitizerSpecialCaseList.h"
 
 using namespace clang;
 
 std::unique_ptr<SanitizerSpecialCaseList>
 SanitizerSpecialCaseList::create(const std::vector<std::string> &Paths,
+                                 llvm::vfs::FileSystem &VFS,
                                  std::string &Error) {
   std::unique_ptr<clang::SanitizerSpecialCaseList> SSCL(
       new SanitizerSpecialCaseList());
   if (SSCL->createInternal(Paths, Error)) {
     SSCL->createSanitizerSections();
     return SSCL;
   }
   return nullptr;
 }
 
 std::unique_ptr<SanitizerSpecialCaseList>
-SanitizerSpecialCaseList::createOrDie(const std::vector<std::string> &Paths) {
+SanitizerSpecialCaseList::createOrDie(const std::vector<std::string> &Paths,
+                                      llvm::vfs::FileSystem &VFS) {
   std::string Error;
-  if (auto SSCL = create(Paths, Error))
+  if (auto SSCL = create(Paths, VFS, Error))
     return SSCL;
   llvm::report_fatal_error(Error);
 }
 
 void SanitizerSpecialCaseList::createSanitizerSections() {
   for (auto &S : Sections) {
     SanitizerMask Mask;
 

clang/test/CodeGen/Inputs/sanitizer-blacklist-vfsoverlay.yaml

+{
+  'version': 0,
+  'roots': [
+    { 'name': '@DIR@', 'type': 'directory',
+      'contents': [
+        { 'name': 'only-virtual-file.blacklist', 'type': 'file',
+          'external-contents': '@REAL_FILE@'
+        },
+        { 'name': 'invalid-virtual-file.blacklist', 'type': 'file',
+          'external-contents': '@NONEXISTENT_FILE@'
+        }
+      ]
+    }
+  ]
+}

clang/test/CodeGen/ubsan-blacklist-vfs.c

+// Verify ubsan doesn't emit checks for blacklisted functions and files
+// RUN: echo "fun:hash" > %t-func.blacklist
+// RUN: echo "src:%s" | sed -e 's/\\/\\\\/g' > %t-file.blacklist
+
+// RUN: rm -f %t-vfsoverlay.yaml
+// RUN: rm -f %t-nonexistent.blacklist
+// RUN: sed -e "s|@DIR@|%/T|g" %S/Inputs/sanitizer-blacklist-vfsoverlay.yaml | sed -e "s|@REAL_FILE@|%/t-func.blacklist|g" | sed -e "s|@NONEXISTENT_FILE@|%/t-nonexistent.blacklist|g" > %t-vfsoverlay.yaml
+// RUN: %clang_cc1 -fsanitize=unsigned-integer-overflow -ivfsoverlay %t-vfsoverlay.yaml -fsanitize-blacklist=%/T/only-virtual-file.blacklist -emit-llvm %s -o - | FileCheck %s --check-prefix=FUNC
+
+// RUN: not %clang_cc1 -fsanitize=unsigned-integer-overflow -ivfsoverlay %t-vfsoverlay.yaml -fsanitize-blacklist=%/T/invalid-virtual-file.blacklist -emit-llvm %s -o - 2>&1 | FileCheck %s --check-prefix=INVALID-MAPPED-FILE
+// INVALID-MAPPED-FILE: invalid-virtual-file.blacklist': {{[Nn]}}o such file or directory
+
+// RUN: not %clang_cc1 -fsanitize=unsigned-integer-overflow -ivfsoverlay %t-vfsoverlay.yaml -fsanitize-blacklist=%t-nonexistent.blacklist -emit-llvm %s -o - 2>&1 | FileCheck %s --check-prefix=INVALID
+// INVALID: nonexistent.blacklist': {{[Nn]}}o such file or directory
+
+unsigned i;
+
+// DEFAULT: @hash
+// FUNC: @hash
+// FILE: @hash
+unsigned hash() {
+// DEFAULT: call {{.*}}void @__ubsan
+// FUNC-NOT: call {{.*}}void @__ubsan
+// FILE-NOT: call {{.*}}void @__ubsan
+  return i * 37;
+}
+
+// DEFAULT: @add
+// FUNC: @add
+// FILE: @add
+unsigned add() {
+// DEFAULT: call {{.*}}void @__ubsan
+// FUNC: call {{.*}}void @__ubsan
+// FILE-NOT: call {{.*}}void @__ubsan
+  return i + 1;
+}

llvm/include/llvm/Support/SpecialCaseList.h

 
 #ifndef LLVM_SUPPORT_SPECIALCASELIST_H
 #define LLVM_SUPPORT_SPECIALCASELIST_H
 
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Support/TrigramIndex.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include <string>
 #include <vector>
 
 namespace llvm {
 class MemoryBuffer;
 class Regex;
 class StringRef;
 
public:
   /// Returns zero if there is no blacklist entry corresponding to this
   /// expression.
   unsigned inSectionBlame(StringRef Section, StringRef Prefix, StringRef Query,
                           StringRef Category = StringRef()) const;
 
 protected:
   // Implementations of the create*() functions that can also be used by derived
   // classes.
-  bool createInternal(const std::vector<std::string> &Paths,
-                      std::string &Error);
+  bool createInternal(const std::vector<std::string> &Paths, std::string &Error,
+                      vfs::FileSystem &VFS = *vfs::getRealFileSystem());
   bool createInternal(const MemoryBuffer *MB, std::string &Error);
 
   SpecialCaseList() = default;
   SpecialCaseList(SpecialCaseList const &) = delete;
   SpecialCaseList &operator=(SpecialCaseList const &) = delete;
 
   /// Represents a set of regular expressions.  Regular expressions which are
   /// "literal" (i.e. no regex metacharacters) are stored in Strings.  The

llvm/lib/Support/SpecialCaseList.cpp

 SpecialCaseList::createOrDie(const std::vector<std::string> &Paths) {
   std::string Error;
   if (auto SCL = create(Paths, Error))
     return SCL;
   report_fatal_error(Error);
 }
 
 bool SpecialCaseList::createInternal(const std::vector<std::string> &Paths,
-                                     std::string &Error) {
+                                     std::string &Error, vfs::FileSystem &VFS) {
   StringMap<size_t> Sections;
   for (const auto &Path : Paths) {
     ErrorOr<std::unique_ptr<MemoryBuffer>> FileOrErr =
         MemoryBuffer::getFile(Path);
     if (std::error_code EC = FileOrErr.getError()) {
       Error = (Twine("can't open file '") + Path + "': " + EC.message()).str();
       return false;
     }
     std::string ParseError;
     if (!parse(FileOrErr.get().get(), Sections, ParseError)) {
       Error = (Twine("error parsing file '") + Path + "': " + ParseError).str();
       return false;
     }
   }
   return true;
 }
 
 bool SpecialCaseList::createInternal(const MemoryBuffer *MB,
                                      std::string &Error) {
   StringMap<size_t> Sections;
   if (!parse(MB, Sections, Error))
     return false;
   return true;
 }
 
 bool SpecialCaseList::parse(const MemoryBuffer *MB,
                             StringMap<size_t> &SectionsMap,
                             std::string &Error) {
   // Iterate through each line in the blacklist file.
   SmallVector<StringRef, 16> Lines;