From d976d43f23d67e18f097d72fd90923627f334c79 Mon Sep 17 00:00:00 2001 From: Alexey Samsonov Date: Mon, 12 Aug 2013 07:49:36 +0000 Subject: [PATCH] Introduce factory methods for SpecialCaseList Summary: 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. Reviewers: pcc Reviewed By: pcc CC: llvm-commits Differential Revision: http://llvm-reviews.chandlerc.com/D1327 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@188156 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../llvm/Transforms/Utils/SpecialCaseList.h | 18 ++++++- lib/Transforms/Utils/SpecialCaseList.cpp | 53 +++++++++++++++---- .../Transforms/Utils/SpecialCaseList.cpp | 34 +++++++++++- 3 files changed, 92 insertions(+), 13 deletions(-) diff --git a/include/llvm/Transforms/Utils/SpecialCaseList.h b/include/llvm/Transforms/Utils/SpecialCaseList.h index 787ddb0c7c4..36ee604a1a6 100644 --- a/include/llvm/Transforms/Utils/SpecialCaseList.h +++ b/include/llvm/Transforms/Utils/SpecialCaseList.h @@ -57,8 +57,18 @@ class StringRef; class SpecialCaseList { public: + // FIXME: Switch all users to factories and remove these constructors. SpecialCaseList(const StringRef Path); SpecialCaseList(const MemoryBuffer *MB); + + /// Parses the special case list from a file. If Path is empty, returns + /// an empty special case list. On failure, returns 0 and writes an error + /// message to string. + static SpecialCaseList *create(const StringRef Path, std::string &Error); + /// Parses the special case list from a memory buffer. On failure, returns + /// 0 and writes an error message to string. + 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 +99,16 @@ class SpecialCaseList { 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 > Entries; - void init(const MemoryBuffer *MB); + SpecialCaseList(); + /// Parses just-constructed SpecialCaseList entries from a memory buffer. + bool parse(const MemoryBuffer *MB, std::string &Error); + bool findCategory(const StringRef Section, const StringRef Query, StringRef &Category) const; bool inSectionCategory(const StringRef Section, const StringRef Query, diff --git a/lib/Transforms/Utils/SpecialCaseList.cpp b/lib/Transforms/Utils/SpecialCaseList.cpp index b98cb5b6894..5a3b192bf6a 100644 --- a/lib/Transforms/Utils/SpecialCaseList.cpp +++ b/lib/Transforms/Utils/SpecialCaseList.cpp @@ -49,29 +49,58 @@ struct SpecialCaseList::Entry { } }; +SpecialCaseList::SpecialCaseList() : Entries() {} + SpecialCaseList::SpecialCaseList(const StringRef Path) { // Validate and open blacklist file. if (Path.empty()) return; OwningPtr File; if (error_code EC = MemoryBuffer::getFile(Path, File)) { - report_fatal_error("Can't open blacklist file: " + Path + ": " + + report_fatal_error("Can't open file '" + Path + "': " + EC.message()); } - init(File.get()); + std::string Error; + if (!parse(File.get(), Error)) + report_fatal_error(Error); } SpecialCaseList::SpecialCaseList(const MemoryBuffer *MB) { - init(MB); + std::string Error; + if (!parse(MB, Error)) + report_fatal_error(Error); } -void SpecialCaseList::init(const MemoryBuffer *MB) { +SpecialCaseList *SpecialCaseList::create( + const StringRef Path, std::string &Error) { + if (Path.empty()) + return new SpecialCaseList(); + OwningPtr File; + if (error_code EC = MemoryBuffer::getFile(Path, File)) { + Error = (Twine("Can't open file '") + Path + "': " + EC.message()).str(); + return 0; + } + return create(File.get(), Error); +} + +SpecialCaseList *SpecialCaseList::create( + const MemoryBuffer *MB, std::string &Error) { + OwningPtr SCL(new SpecialCaseList()); + if (!SCL->parse(MB, Error)) + return 0; + return SCL.take(); +} + +bool SpecialCaseList::parse(const MemoryBuffer *MB, std::string &Error) { // Iterate through each line in the blacklist file. SmallVector Lines; SplitString(MB->getBuffer(), Lines, "\n\r"); StringMap > Regexps; + assert(Entries.empty() && + "parse() should be called on an empty SpecialCaseList"); + int LineNo = 1; for (SmallVectorImpl::iterator I = Lines.begin(), E = Lines.end(); - I != E; ++I) { + I != E; ++I, ++LineNo) { // Ignore empty lines and lines starting with "#" if (I->empty() || I->startswith("#")) continue; @@ -80,7 +109,9 @@ void SpecialCaseList::init(const MemoryBuffer *MB) { StringRef Prefix = SplitLine.first; if (SplitLine.second.empty()) { // Missing ':' in the line. - report_fatal_error("malformed blacklist line: " + SplitLine.first); + Error = (Twine("Malformed line ") + Twine(LineNo) + ": '" + + SplitLine.first + "'").str(); + return false; } std::pair SplitRegexp = SplitLine.second.split("="); @@ -113,10 +144,11 @@ void SpecialCaseList::init(const MemoryBuffer *MB) { // 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 regex in line ") + Twine(LineNo) + ": '" + + SplitLine.second + "': " + REError).str(); + return false; } // Add this regexp into the proper group by its prefix. @@ -135,6 +167,7 @@ void SpecialCaseList::init(const MemoryBuffer *MB) { Entries[I->getKey()][II->getKey()].RegEx = new Regex(II->getValue()); } } + return true; } SpecialCaseList::~SpecialCaseList() { diff --git a/unittests/Transforms/Utils/SpecialCaseList.cpp b/unittests/Transforms/Utils/SpecialCaseList.cpp index 9deee3c1e9d..aee412dcb5b 100644 --- a/unittests/Transforms/Utils/SpecialCaseList.cpp +++ b/unittests/Transforms/Utils/SpecialCaseList.cpp @@ -34,9 +34,17 @@ protected: M, ST, false, GlobalValue::ExternalLinkage, 0, Name); } - SpecialCaseList *makeSpecialCaseList(StringRef List) { + SpecialCaseList *makeSpecialCaseList(StringRef List, std::string &Error) { OwningPtr 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,26 @@ TEST_F(SpecialCaseListTest, Substring) { EXPECT_TRUE(SCL->isIn(*F)); } +TEST_F(SpecialCaseListTest, InvalidSpecialCaseList) { + std::string Error; + EXPECT_EQ(0, makeSpecialCaseList("badline", Error)); + EXPECT_EQ("Malformed line 1: 'badline'", Error); + EXPECT_EQ(0, makeSpecialCaseList("src:bad[a-", Error)); + EXPECT_EQ("Malformed regex in line 1: 'bad[a-': invalid character range", + Error); + EXPECT_EQ(0, makeSpecialCaseList("src:a.c\n" + "fun:fun(a\n", + Error)); + EXPECT_EQ("Malformed regex in line 2: 'fun(a': parentheses not balanced", + Error); + EXPECT_EQ(0, SpecialCaseList::create("unexisting", Error)); + EXPECT_EQ("Can't open file 'unexisting': No such file or directory", Error); +} + +TEST_F(SpecialCaseListTest, EmptySpecialCaseList) { + OwningPtr SCL(makeSpecialCaseList("")); + Module M("foo", Ctx); + EXPECT_FALSE(SCL->isIn(M)); +} + }