[PATCH] Introduce factory methods for SpecialCaseList

Alexey Samsonov samsonov at google.com
Thu Aug 8 10:53:56 PDT 2013


Hi pcc,

Doing work in constructors is bad: this change suggests to
call SpecialCaseList::create(Path, Error) instead of
"new SpecialCaseList(Path)". Currently the latter may crash with
report_fatal_error, which is undesirable - sometimes we want to report
the error to user gracefully - for example, if he provides an incorrect
file as an argument of Clang's -fsanitize-blacklist flag.

http://llvm-reviews.chandlerc.com/D1327

Files:
  unittests/Transforms/Utils/SpecialCaseList.cpp
  lib/Transforms/Utils/SpecialCaseList.cpp
  include/llvm/Transforms/Utils/SpecialCaseList.h

Index: unittests/Transforms/Utils/SpecialCaseList.cpp
===================================================================
--- unittests/Transforms/Utils/SpecialCaseList.cpp
+++ unittests/Transforms/Utils/SpecialCaseList.cpp
@@ -34,9 +34,17 @@
         M, ST, false, GlobalValue::ExternalLinkage, 0, Name);
   }
 
-  SpecialCaseList *makeSpecialCaseList(StringRef List) {
+  SpecialCaseList *makeSpecialCaseList(StringRef List, std::string &Error) {
     OwningPtr<MemoryBuffer> MB(MemoryBuffer::getMemBuffer(List));
-    return new SpecialCaseList(MB.get());
+    return SpecialCaseList::create(MB.get(), Error);
+  }
+
+  SpecialCaseList *makeSpecialCaseList(StringRef List) {
+    std::string Error;
+    SpecialCaseList *SCL = makeSpecialCaseList(List, Error);
+    assert(SCL);
+    assert(Error == "");
+    return SCL;
   }
 
   LLVMContext Ctx;
@@ -155,4 +163,24 @@
   EXPECT_TRUE(SCL->isIn(*F));
 }
 
+TEST_F(SpecialCaseListTest, InvalidSpecialCaseList) {
+  std::string Error;
+  EXPECT_EQ(0, makeSpecialCaseList("badline", Error));
+  EXPECT_EQ("malformed special case list line: badline", Error);
+  EXPECT_EQ(0, makeSpecialCaseList("src:bad[a-", Error));
+  EXPECT_EQ(
+      "malformed special case list regex: bad[a-: invalid character range",
+      Error);
+  EXPECT_EQ(0, makeSpecialCaseList("src:a.c\n"
+                                   "fun:fun(a\n", Error));
+  EXPECT_EQ(
+      "malformed special case list regex: fun(a: parentheses not balanced",
+      Error);
+  EXPECT_EQ(0, SpecialCaseList::create("", Error));
+  EXPECT_EQ("Empty path to blacklist file", Error);
+  EXPECT_EQ(0, SpecialCaseList::create("unexisting", Error));
+  EXPECT_EQ("Can't open blacklist file: unexisting: No such file or directory",
+            Error);
+}
+
 }
Index: lib/Transforms/Utils/SpecialCaseList.cpp
===================================================================
--- lib/Transforms/Utils/SpecialCaseList.cpp
+++ lib/Transforms/Utils/SpecialCaseList.cpp
@@ -49,6 +49,8 @@
   }
 };
 
+SpecialCaseList::SpecialCaseList() : Entries() {}
+
 SpecialCaseList::SpecialCaseList(const StringRef Path) {
   // Validate and open blacklist file.
   if (Path.empty()) return;
@@ -58,18 +60,47 @@
                        EC.message());
   }
 
-  init(File.get());
+  std::string Error;
+  if (!parse(File.get(), *this, Error))
+    report_fatal_error(Error);
 }
 
 SpecialCaseList::SpecialCaseList(const MemoryBuffer *MB) {
-  init(MB);
+  std::string Error;
+  if (!parse(MB, *this, Error))
+    report_fatal_error(Error);
+}
+
+SpecialCaseList *SpecialCaseList::create(
+    const StringRef Path, std::string &Error) {
+  if (Path.empty()) {
+    Error = "Empty path to blacklist file";
+    return 0;
+  }
+  OwningPtr<MemoryBuffer> File;
+  if (error_code EC = MemoryBuffer::getFile(Path, File)) {
+    Error = (Twine("Can't open blacklist file: ") + Path + ": " +
+        EC.message()).str();
+    return 0;
+  }
+  return create(File.get(), Error);
+}
+
+SpecialCaseList *SpecialCaseList::create(
+    const MemoryBuffer *MB, std::string &Error) {
+  OwningPtr<SpecialCaseList> SCL(new SpecialCaseList());
+  if (!parse(MB, *(SCL.get()), Error))
+    return 0;
+  return SCL.take();
 }
 
-void SpecialCaseList::init(const MemoryBuffer *MB) {
+bool SpecialCaseList::parse(const MemoryBuffer *MB, SpecialCaseList &SCL,
+                            std::string &Error) {
   // Iterate through each line in the blacklist file.
   SmallVector<StringRef, 16> Lines;
   SplitString(MB->getBuffer(), Lines, "\n\r");
   StringMap<StringMap<std::string> > Regexps;
+  SCL.Entries.clear();
   for (SmallVectorImpl<StringRef>::iterator I = Lines.begin(), E = Lines.end();
        I != E; ++I) {
     // Ignore empty lines and lines starting with "#"
@@ -80,7 +111,9 @@
     StringRef Prefix = SplitLine.first;
     if (SplitLine.second.empty()) {
       // Missing ':' in the line.
-      report_fatal_error("malformed blacklist line: " + SplitLine.first);
+      Error =
+          (Twine("malformed special case list line: ") + SplitLine.first).str();
+      return false;
     }
 
     std::pair<StringRef, StringRef> SplitRegexp = SplitLine.second.split("=");
@@ -101,7 +134,7 @@
 
     // See if we can store Regexp in Strings.
     if (Regex::isLiteralERE(Regexp)) {
-      Entries[Prefix][Category].Strings.insert(Regexp);
+      SCL.Entries[Prefix][Category].Strings.insert(Regexp);
       continue;
     }
 
@@ -113,10 +146,11 @@
 
     // Check that the regexp is valid.
     Regex CheckRE(Regexp);
-    std::string Error;
-    if (!CheckRE.isValid(Error)) {
-      report_fatal_error("malformed blacklist regex: " + SplitLine.second +
-          ": " + Error);
+    std::string REError;
+    if (!CheckRE.isValid(REError)) {
+      Error = (Twine("malformed special case list regex: ") +
+              SplitLine.second + ": " + REError).str();
+      return false;
     }
 
     // Add this regexp into the proper group by its prefix.
@@ -132,9 +166,10 @@
     for (StringMap<std::string>::const_iterator II = I->second.begin(),
                                                 IE = I->second.end();
          II != IE; ++II) {
-      Entries[I->getKey()][II->getKey()].RegEx = new Regex(II->getValue());
+      SCL.Entries[I->getKey()][II->getKey()].RegEx = new Regex(II->getValue());
     }
   }
+  return true;
 }
 
 SpecialCaseList::~SpecialCaseList() {
Index: include/llvm/Transforms/Utils/SpecialCaseList.h
===================================================================
--- include/llvm/Transforms/Utils/SpecialCaseList.h
+++ include/llvm/Transforms/Utils/SpecialCaseList.h
@@ -57,8 +57,15 @@
 
 class SpecialCaseList {
  public:
+  // FIXME: Switch all users to factories and remove these constructors.
   SpecialCaseList(const StringRef Path);
   SpecialCaseList(const MemoryBuffer *MB);
+
+  // Factory methods. On failure, return 0 and write an error
+  // message to string.
+  static SpecialCaseList *create(const StringRef Path, std::string &Error);
+  static SpecialCaseList *create(const MemoryBuffer *MB, std::string &Error);
+
   ~SpecialCaseList();
 
   /// Returns whether either this function or its source file are listed in the
@@ -89,10 +96,16 @@
   bool findCategory(const Module &M, StringRef &Category) const;
 
  private:
+  SpecialCaseList(SpecialCaseList const &) LLVM_DELETED_FUNCTION;
+  SpecialCaseList &operator=(SpecialCaseList const &) LLVM_DELETED_FUNCTION;
+
   struct Entry;
   StringMap<StringMap<Entry> > Entries;
 
-  void init(const MemoryBuffer *MB);
+  SpecialCaseList();
+  static bool parse(const MemoryBuffer *MB, SpecialCaseList &SCL,
+                    std::string &Error);
+
   bool findCategory(const StringRef Section, const StringRef Query,
                     StringRef &Category) const;
   bool inSectionCategory(const StringRef Section, const StringRef Query,
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D1327.1.patch
Type: text/x-patch
Size: 6835 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130808/f9bbec8a/attachment.bin>


More information about the llvm-commits mailing list