[llvm] r305765 - [ProfileData] PR33517: Check for failure of symtab creation

Vedant Kumar via llvm-commits llvm-commits at lists.llvm.org
Mon Jun 19 18:38:56 PDT 2017


Author: vedantk
Date: Mon Jun 19 20:38:56 2017
New Revision: 305765

URL: http://llvm.org/viewvc/llvm-project?rev=305765&view=rev
Log:
[ProfileData] PR33517: Check for failure of symtab creation

With PR33517, it became apparent that symbol table creation can fail
when presented with malformed inputs. This patch makes that sort of
error detectable, so llvm-cov etc. can fail more gracefully.

Specifically, we now check that function names within the symbol table
aren't empty.

Testing: check-{llvm,clang,profile}, some unit test updates.

Modified:
    llvm/trunk/include/llvm/ProfileData/InstrProf.h
    llvm/trunk/include/llvm/ProfileData/InstrProfReader.h
    llvm/trunk/include/llvm/ProfileData/InstrProfWriter.h
    llvm/trunk/lib/ProfileData/Coverage/CoverageMappingReader.cpp
    llvm/trunk/lib/ProfileData/InstrProf.cpp
    llvm/trunk/lib/ProfileData/InstrProfReader.cpp
    llvm/trunk/lib/ProfileData/InstrProfWriter.cpp
    llvm/trunk/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp
    llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp
    llvm/trunk/unittests/ProfileData/InstrProfTest.cpp

Modified: llvm/trunk/include/llvm/ProfileData/InstrProf.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/InstrProf.h?rev=305765&r1=305764&r2=305765&view=diff
==============================================================================
--- llvm/trunk/include/llvm/ProfileData/InstrProf.h (original)
+++ llvm/trunk/include/llvm/ProfileData/InstrProf.h Mon Jun 19 20:38:56 2017
@@ -450,11 +450,11 @@ public:
   /// decls from module \c M. This interface is used by transformation
   /// passes such as indirect function call promotion. Variable \c InLTO
   /// indicates if this is called from LTO optimization passes.
-  void create(Module &M, bool InLTO = false);
+  Error create(Module &M, bool InLTO = false);
 
   /// Create InstrProfSymtab from a set of names iteratable from
   /// \p IterRange. This interface is used by IndexedProfReader.
-  template <typename NameIterRange> void create(const NameIterRange &IterRange);
+  template <typename NameIterRange> Error create(const NameIterRange &IterRange);
 
   // If the symtab is created by a series of calls to \c addFuncName, \c
   // finalizeSymtab needs to be called before looking up function names.
@@ -464,11 +464,14 @@ public:
 
   /// Update the symtab by adding \p FuncName to the table. This interface
   /// is used by the raw and text profile readers.
-  void addFuncName(StringRef FuncName) {
+  Error addFuncName(StringRef FuncName) {
+    if (FuncName.empty())
+      return make_error<InstrProfError>(instrprof_error::malformed);
     auto Ins = NameTab.insert(FuncName);
     if (Ins.second)
       MD5NameMap.push_back(std::make_pair(
           IndexedInstrProf::ComputeHash(FuncName), Ins.first->getKey()));
+    return Error::success();
   }
 
   /// Map a function address to its name's MD5 hash. This interface
@@ -511,11 +514,13 @@ Error InstrProfSymtab::create(StringRef
 }
 
 template <typename NameIterRange>
-void InstrProfSymtab::create(const NameIterRange &IterRange) {
+Error InstrProfSymtab::create(const NameIterRange &IterRange) {
   for (auto Name : IterRange)
-    addFuncName(Name);
+    if (Error E = addFuncName(Name))
+      return E;
 
   finalizeSymtab();
+  return Error::success();
 }
 
 void InstrProfSymtab::finalizeSymtab() {

Modified: llvm/trunk/include/llvm/ProfileData/InstrProfReader.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/InstrProfReader.h?rev=305765&r1=305764&r2=305765&view=diff
==============================================================================
--- llvm/trunk/include/llvm/ProfileData/InstrProfReader.h (original)
+++ llvm/trunk/include/llvm/ProfileData/InstrProfReader.h Mon Jun 19 20:38:56 2017
@@ -343,7 +343,7 @@ struct InstrProfReaderIndexBase {
   virtual void setValueProfDataEndianness(support::endianness Endianness) = 0;
   virtual uint64_t getVersion() const = 0;
   virtual bool isIRLevelProfile() const = 0;
-  virtual void populateSymtab(InstrProfSymtab &) = 0;
+  virtual Error populateSymtab(InstrProfSymtab &) = 0;
 };
 
 typedef OnDiskIterableChainedHashTable<InstrProfLookupTrait>
@@ -383,8 +383,8 @@ public:
     return (FormatVersion & VARIANT_MASK_IR_PROF) != 0;
   }
 
-  void populateSymtab(InstrProfSymtab &Symtab) override {
-    Symtab.create(HashTable->keys());
+  Error populateSymtab(InstrProfSymtab &Symtab) override {
+    return Symtab.create(HashTable->keys());
   }
 };
 

Modified: llvm/trunk/include/llvm/ProfileData/InstrProfWriter.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/InstrProfWriter.h?rev=305765&r1=305764&r2=305765&view=diff
==============================================================================
--- llvm/trunk/include/llvm/ProfileData/InstrProfWriter.h (original)
+++ llvm/trunk/include/llvm/ProfileData/InstrProfWriter.h Mon Jun 19 20:38:56 2017
@@ -58,7 +58,7 @@ public:
   void write(raw_fd_ostream &OS);
 
   /// Write the profile in text format to \c OS
-  void writeText(raw_fd_ostream &OS);
+  Error writeText(raw_fd_ostream &OS);
 
   /// Write \c Record in text format to \c OS
   static void writeRecordInText(const InstrProfRecord &Record,

Modified: llvm/trunk/lib/ProfileData/Coverage/CoverageMappingReader.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/Coverage/CoverageMappingReader.cpp?rev=305765&r1=305764&r2=305765&view=diff
==============================================================================
--- llvm/trunk/lib/ProfileData/Coverage/CoverageMappingReader.cpp (original)
+++ llvm/trunk/lib/ProfileData/Coverage/CoverageMappingReader.cpp Mon Jun 19 20:38:56 2017
@@ -419,6 +419,8 @@ class VersionedCovMapFuncRecordReader :
       StringRef FuncName;
       if (Error Err = CFR->template getFuncName<Endian>(ProfileNames, FuncName))
         return Err;
+      if (FuncName.empty())
+        return make_error<InstrProfError>(instrprof_error::malformed);
       Records.emplace_back(Version, FuncName, FuncHash, Mapping, FilenamesBegin,
                            Filenames.size() - FilenamesBegin);
       return Error::success();

Modified: llvm/trunk/lib/ProfileData/InstrProf.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProf.cpp?rev=305765&r1=305764&r2=305765&view=diff
==============================================================================
--- llvm/trunk/lib/ProfileData/InstrProf.cpp (original)
+++ llvm/trunk/lib/ProfileData/InstrProf.cpp Mon Jun 19 20:38:56 2017
@@ -330,14 +330,15 @@ GlobalVariable *createPGOFuncNameVar(Fun
   return createPGOFuncNameVar(*F.getParent(), F.getLinkage(), PGOFuncName);
 }
 
-void InstrProfSymtab::create(Module &M, bool InLTO) {
+Error InstrProfSymtab::create(Module &M, bool InLTO) {
   for (Function &F : M) {
     // Function may not have a name: like using asm("") to overwrite the name.
     // Ignore in this case.
     if (!F.hasName())
       continue;
     const std::string &PGOFuncName = getPGOFuncName(F, InLTO);
-    addFuncName(PGOFuncName);
+    if (Error E = addFuncName(PGOFuncName))
+      return E;
     MD5FuncMap.emplace_back(Function::getGUID(PGOFuncName), &F);
     // In ThinLTO, local function may have been promoted to global and have
     // suffix added to the function name. We need to add the stripped function
@@ -346,13 +347,15 @@ void InstrProfSymtab::create(Module &M,
       auto pos = PGOFuncName.find('.');
       if (pos != std::string::npos) {
         const std::string &OtherFuncName = PGOFuncName.substr(0, pos);
-        addFuncName(OtherFuncName);
+        if (Error E = addFuncName(OtherFuncName))
+          return E;
         MD5FuncMap.emplace_back(Function::getGUID(OtherFuncName), &F);
       }
     }
   }
 
   finalizeSymtab();
+  return Error::success();
 }
 
 Error collectPGOFuncNameStrings(ArrayRef<std::string> NameStrs,
@@ -447,7 +450,8 @@ Error readPGOFuncNameStrings(StringRef N
     SmallVector<StringRef, 0> Names;
     NameStrings.split(Names, getInstrProfNameSeparator());
     for (StringRef &Name : Names)
-      Symtab.addFuncName(Name);
+      if (Error E = Symtab.addFuncName(Name))
+        return E;
 
     while (P < EndP && *P == 0)
       P++;

Modified: llvm/trunk/lib/ProfileData/InstrProfReader.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProfReader.cpp?rev=305765&r1=305764&r2=305765&view=diff
==============================================================================
--- llvm/trunk/lib/ProfileData/InstrProfReader.cpp (original)
+++ llvm/trunk/lib/ProfileData/InstrProfReader.cpp Mon Jun 19 20:38:56 2017
@@ -200,7 +200,8 @@ TextInstrProfReader::readValueProfileDat
         std::pair<StringRef, StringRef> VD = Line->rsplit(':');
         uint64_t TakenCount, Value;
         if (ValueKind == IPVK_IndirectCallTarget) {
-          Symtab->addFuncName(VD.first);
+          if (Error E = Symtab->addFuncName(VD.first))
+            return E;
           Value = IndexedInstrProf::ComputeHash(VD.first);
         } else {
           READ_NUM(VD.first, Value);
@@ -232,7 +233,8 @@ Error TextInstrProfReader::readNextRecor
 
   // Read the function name.
   Record.Name = *Line++;
-  Symtab->addFuncName(Record.Name);
+  if (Error E = Symtab->addFuncName(Record.Name))
+    return E;
 
   // Read the function hash.
   if (Line.is_at_end())
@@ -694,7 +696,9 @@ InstrProfSymtab &IndexedInstrProfReader:
     return *Symtab.get();
 
   std::unique_ptr<InstrProfSymtab> NewSymtab = make_unique<InstrProfSymtab>();
-  Index->populateSymtab(*NewSymtab.get());
+  if (Error E = Index->populateSymtab(*NewSymtab.get())) {
+    consumeError(error(InstrProfError::take(std::move(E))));
+  }
 
   Symtab = std::move(NewSymtab);
   return *Symtab.get();

Modified: llvm/trunk/lib/ProfileData/InstrProfWriter.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProfWriter.cpp?rev=305765&r1=305764&r2=305765&view=diff
==============================================================================
--- llvm/trunk/lib/ProfileData/InstrProfWriter.cpp (original)
+++ llvm/trunk/lib/ProfileData/InstrProfWriter.cpp Mon Jun 19 20:38:56 2017
@@ -363,17 +363,19 @@ void InstrProfWriter::writeRecordInText(
   OS << "\n";
 }
 
-void InstrProfWriter::writeText(raw_fd_ostream &OS) {
+Error InstrProfWriter::writeText(raw_fd_ostream &OS) {
   if (ProfileKind == PF_IRLevel)
     OS << "# IR level Instrumentation Flag\n:ir\n";
   InstrProfSymtab Symtab;
   for (const auto &I : FunctionData)
     if (shouldEncodeData(I.getValue()))
-      Symtab.addFuncName(I.getKey());
+      if (Error E = Symtab.addFuncName(I.getKey()))
+        return E;
   Symtab.finalizeSymtab();
 
   for (const auto &I : FunctionData)
     if (shouldEncodeData(I.getValue()))
       for (const auto &Func : I.getValue())
         writeRecordInText(Func.second, Symtab, OS);
+  return Error::success();
 }

Modified: llvm/trunk/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp?rev=305765&r1=305764&r2=305765&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp (original)
+++ llvm/trunk/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp Mon Jun 19 20:38:56 2017
@@ -642,7 +642,12 @@ static bool promoteIndirectCalls(Module
   if (DisableICP)
     return false;
   InstrProfSymtab Symtab;
-  Symtab.create(M, InLTO);
+  if (Error E = Symtab.create(M, InLTO)) {
+    std::string SymtabFailure = toString(std::move(E));
+    DEBUG(dbgs() << "Failed to create symtab: " << SymtabFailure << "\n");
+    (void)SymtabFailure;
+    return false;
+  }
   bool Changed = false;
   for (auto &F : M) {
     if (F.isDeclaration())

Modified: llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp?rev=305765&r1=305764&r2=305765&view=diff
==============================================================================
--- llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp (original)
+++ llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp Mon Jun 19 20:38:56 2017
@@ -246,10 +246,12 @@ static void mergeInstrProfile(const Weig
       exitWithError(std::move(WC->Err), WC->ErrWhence);
 
   InstrProfWriter &Writer = Contexts[0]->Writer;
-  if (OutputFormat == PF_Text)
-    Writer.writeText(Output);
-  else
+  if (OutputFormat == PF_Text) {
+    if (Error E = Writer.writeText(Output))
+      exitWithError(std::move(E));
+  } else {
     Writer.write(Output);
+  }
 }
 
 static sampleprof::SampleProfileFormat FormatMap[] = {

Modified: llvm/trunk/unittests/ProfileData/InstrProfTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ProfileData/InstrProfTest.cpp?rev=305765&r1=305764&r2=305765&view=diff
==============================================================================
--- llvm/trunk/unittests/ProfileData/InstrProfTest.cpp (original)
+++ llvm/trunk/unittests/ProfileData/InstrProfTest.cpp Mon Jun 19 20:38:56 2017
@@ -859,7 +859,7 @@ TEST_P(MaybeSparseInstrProfTest, instr_p
   FuncNames.push_back("bar2");
   FuncNames.push_back("bar3");
   InstrProfSymtab Symtab;
-  Symtab.create(FuncNames);
+  NoError(Symtab.create(FuncNames));
   StringRef R = Symtab.getFuncName(IndexedInstrProf::ComputeHash("func1"));
   ASSERT_EQ(StringRef("func1"), R);
   R = Symtab.getFuncName(IndexedInstrProf::ComputeHash("func2"));
@@ -880,9 +880,9 @@ TEST_P(MaybeSparseInstrProfTest, instr_p
   ASSERT_EQ(StringRef(), R);
 
   // Now incrementally update the symtab
-  Symtab.addFuncName("blah_1");
-  Symtab.addFuncName("blah_2");
-  Symtab.addFuncName("blah_3");
+  NoError(Symtab.addFuncName("blah_1"));
+  NoError(Symtab.addFuncName("blah_2"));
+  NoError(Symtab.addFuncName("blah_3"));
   // Finalize it
   Symtab.finalizeSymtab();
 
@@ -907,6 +907,12 @@ TEST_P(MaybeSparseInstrProfTest, instr_p
   ASSERT_EQ(StringRef("bar3"), R);
 }
 
+// Test that we get an error when creating a bogus symtab.
+TEST_P(MaybeSparseInstrProfTest, instr_prof_bogus_symtab_empty_func_name) {
+  InstrProfSymtab Symtab;
+  ErrorEquals(instrprof_error::malformed, Symtab.addFuncName(""));
+}
+
 // Testing symtab creator interface used by value profile transformer.
 TEST_P(MaybeSparseInstrProfTest, instr_prof_symtab_module_test) {
   LLVMContext Ctx;
@@ -927,7 +933,7 @@ TEST_P(MaybeSparseInstrProfTest, instr_p
   Function::Create(FTy, Function::WeakODRLinkage, "Wbar", M.get());
 
   InstrProfSymtab ProfSymtab;
-  ProfSymtab.create(*M);
+  NoError(ProfSymtab.create(*M));
 
   StringRef Funcs[] = {"Gfoo", "Gblah", "Gbar", "Ifoo", "Iblah", "Ibar",
                        "Pfoo", "Pblah", "Pbar", "Wfoo", "Wblah", "Wbar"};




More information about the llvm-commits mailing list