InstrProf: Simplify the construction of BinaryCoverageReader

Creating BinaryCoverageReader is a strange and complicated dance where
the constructor sets error codes that member functions will later
read, and the object is in an invalid state if readHeader isn't
immediately called after construction.

Instead, make the constructor private and add a static create method
to do the construction properly. This also has the benefit of removing
readHeader completely and simplifying the interface of the object.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@230676 91177308-0d34-0410-b5e6-96231b3b80d8

include/llvm/ProfileData/CoverageMappingReader.h

@@ -161,8 +161,6 @@ public:
   };
 
 private:
-  std::error_code LastError;
-  object::OwningBinary<object::ObjectFile> Object;
   std::vector<StringRef> Filenames;
   std::vector<ProfileMappingRecord> MappingRecords;
   size_t CurrentRecord;
@@ -173,28 +171,13 @@ private:
   BinaryCoverageReader(const BinaryCoverageReader &) = delete;
   BinaryCoverageReader &operator=(const BinaryCoverageReader &) = delete;
 
-  /// \brief Set the current error_code and return same.
+  BinaryCoverageReader() : CurrentRecord(0) {}
-  std::error_code error(std::error_code EC) {
-    LastError = EC;
-    return EC;
-  }
-
-  /// \brief Clear the current error code and return a successful one.
-  std::error_code success() { return error(instrprof_error::success); }
 
 public:
-  BinaryCoverageReader(std::unique_ptr<MemoryBuffer> &ObjectBuffer);
+  static ErrorOr<std::unique_ptr<BinaryCoverageReader>>
+  create(std::unique_ptr<MemoryBuffer> &ObjectBuffer);
 
-  std::error_code readHeader();
   std::error_code readNextRecord(CoverageMappingRecord &Record) override;
-
-  /// \brief Return true if the reader has finished reading the profile data.
-  bool isEOF() { return LastError == instrprof_error::eof; }
-  /// \brief Return true if the reader encountered an error reading profiling
-  /// data.
-  bool hasError() { return LastError && !isEOF(); }
-  /// \brief Get the current error code.
-  std::error_code getError() { return LastError; }
 };
 
 } // end namespace coverage

lib/ProfileData/CoverageMapping.cpp

@@ -219,16 +219,18 @@ CoverageMapping::load(CoverageMappingReader &CoverageReader,
 ErrorOr<std::unique_ptr<CoverageMapping>>
 CoverageMapping::load(StringRef ObjectFilename, StringRef ProfileFilename) {
   auto CounterMappingBuff = MemoryBuffer::getFileOrSTDIN(ObjectFilename);
-  if (auto EC = CounterMappingBuff.getError())
+  if (std::error_code EC = CounterMappingBuff.getError())
     return EC;
-  BinaryCoverageReader CoverageReader(CounterMappingBuff.get());
+  auto CoverageReaderOrErr =
-  if (auto EC = CoverageReader.readHeader())
+      BinaryCoverageReader::create(CounterMappingBuff.get());
+  if (std::error_code EC = CoverageReaderOrErr.getError())
     return EC;
+  auto CoverageReader = std::move(CoverageReaderOrErr.get());
   auto ProfileReaderOrErr = IndexedInstrProfReader::create(ProfileFilename);
   if (auto EC = ProfileReaderOrErr.getError())
     return EC;
   auto ProfileReader = std::move(ProfileReaderOrErr.get());
-  return load(CoverageReader, *ProfileReader);
+  return load(*CoverageReader, *ProfileReader);
 }
 
 namespace {

lib/ProfileData/CoverageMappingReader.cpp

@@ -411,9 +411,12 @@ std::error_code readCoverageMappingData(
 
 static const char *TestingFormatMagic = "llvmcovmtestdata";
 
-static std::error_code decodeTestingFormat(StringRef Data,
+static std::error_code loadTestingFormat(StringRef Data,
-                                           SectionData &ProfileNames,
+                                         SectionData &ProfileNames,
-                                           StringRef &CoverageMapping) {
+                                         StringRef &CoverageMapping,
+                                         uint8_t &BytesInAddress) {
+  BytesInAddress = 8;
+
   Data = Data.substr(StringRef(TestingFormatMagic).size());
   if (Data.size() < 1)
     return instrprof_error::truncated;
@@ -438,86 +441,78 @@ static std::error_code decodeTestingFormat(StringRef Data,
   return instrprof_error::success;
 }
 
-BinaryCoverageReader::BinaryCoverageReader(
+static std::error_code loadBinaryFormat(MemoryBufferRef ObjectBuffer,
-    std::unique_ptr<MemoryBuffer> &ObjectBuffer)
+                                        SectionData &ProfileNames,
-    : CurrentRecord(0) {
+                                        StringRef &CoverageMapping,
-  if (ObjectBuffer->getBuffer().startswith(TestingFormatMagic)) {
+                                        uint8_t &BytesInAddress) {
-    // This is a special format used for testing.
+  auto ObjectFileOrErr = object::ObjectFile::createObjectFile(ObjectBuffer);
-    SectionData ProfileNames;
+  if (std::error_code EC = ObjectFileOrErr.getError())
-    StringRef CoverageMapping;
+    return EC;
-    if (auto Err = decodeTestingFormat(ObjectBuffer->getBuffer(), ProfileNames,
+  auto OF = std::move(ObjectFileOrErr.get());
-                                       CoverageMapping)) {
+  BytesInAddress = OF->getBytesInAddress();
-      error(Err);
-      return;
-    }
-    error(readCoverageMappingData<uint64_t>(ProfileNames, CoverageMapping,
-                                            MappingRecords, Filenames));
-    Object = OwningBinary<ObjectFile>(std::unique_ptr<ObjectFile>(),
-                                      std::move(ObjectBuffer));
-    return;
-  }
-
-  auto File =
-      object::ObjectFile::createObjectFile(ObjectBuffer->getMemBufferRef());
-  if (!File)
-    error(File.getError());
-  else
-    Object = OwningBinary<ObjectFile>(std::move(File.get()),
-                                      std::move(ObjectBuffer));
-}
-
-std::error_code BinaryCoverageReader::readHeader() {
-  const ObjectFile *OF = Object.getBinary();
-  if (!OF)
-    return getError();
-  auto BytesInAddress = OF->getBytesInAddress();
-  if (BytesInAddress != 4 && BytesInAddress != 8)
-    return error(instrprof_error::malformed);
 
   // Look for the sections that we are interested in.
   int FoundSectionCount = 0;
-  SectionRef ProfileNames, CoverageMapping;
+  SectionRef NamesSection, CoverageSection;
   for (const auto &Section : OF->sections()) {
     StringRef Name;
     if (auto Err = Section.getName(Name))
       return Err;
     if (Name == "__llvm_prf_names") {
-      ProfileNames = Section;
+      NamesSection = Section;
     } else if (Name == "__llvm_covmap") {
-      CoverageMapping = Section;
+      CoverageSection = Section;
     } else
       continue;
     ++FoundSectionCount;
   }
   if (FoundSectionCount != 2)
-    return error(instrprof_error::bad_header);
+    return instrprof_error::bad_header;
 
   // Get the contents of the given sections.
-  StringRef Data;
+  if (std::error_code EC = CoverageSection.getContents(CoverageMapping))
-  if (auto Err = CoverageMapping.getContents(Data))
+    return EC;
-    return Err;
+  if (std::error_code EC = ProfileNames.load(NamesSection))
-  SectionData ProfileNamesData;
+    return EC;
-  if (auto Err = ProfileNamesData.load(ProfileNames))
-    return Err;
 
-  // Load the data from the found sections.
+  return std::error_code();
-  std::error_code Err;
+}
-  if (BytesInAddress == 4)
+
-    Err = readCoverageMappingData<uint32_t>(ProfileNamesData, Data,
+ErrorOr<std::unique_ptr<BinaryCoverageReader>>
-                                            MappingRecords, Filenames);
+BinaryCoverageReader::create(std::unique_ptr<MemoryBuffer> &ObjectBuffer) {
+  std::unique_ptr<BinaryCoverageReader> Reader(new BinaryCoverageReader());
+
+  SectionData Profile;
+  StringRef Coverage;
+  uint8_t BytesInAddress;
+  std::error_code EC;
+  if (ObjectBuffer->getBuffer().startswith(TestingFormatMagic))
+    // This is a special format used for testing.
+    EC = loadTestingFormat(ObjectBuffer->getBuffer(), Profile, Coverage,
+                           BytesInAddress);
   else
-    Err = readCoverageMappingData<uint64_t>(ProfileNamesData, Data,
+    EC = loadBinaryFormat(ObjectBuffer->getMemBufferRef(), Profile, Coverage,
-                                            MappingRecords, Filenames);
+                          BytesInAddress);
-  if (Err)
+  if (EC)
-    return error(Err);
+    return EC;
 
-  return success();
+  if (BytesInAddress == 4)
+    EC = readCoverageMappingData<uint32_t>(
+        Profile, Coverage, Reader->MappingRecords, Reader->Filenames);
+  else if (BytesInAddress == 8)
+    EC = readCoverageMappingData<uint64_t>(
+        Profile, Coverage, Reader->MappingRecords, Reader->Filenames);
+  else
+    return instrprof_error::malformed;
+  if (EC)
+    return EC;
+  return std::move(Reader);
 }
 
 std::error_code
 BinaryCoverageReader::readNextRecord(CoverageMappingRecord &Record) {
   if (CurrentRecord >= MappingRecords.size())
-    return error(instrprof_error::eof);
+    return instrprof_error::eof;
 
   FunctionsFilenames.clear();
   Expressions.clear();
@@ -537,5 +532,5 @@ BinaryCoverageReader::readNextRecord(CoverageMappingRecord &Record) {
   Record.MappingRegions = MappingRegions;
 
   ++CurrentRecord;
-  return success();
+  return std::error_code();
 }