[Coverage] Report errors when reading malformed source regions

[vsk]

Summary

Each source region has a start and end location. Report an error when
the end location does not precede the begin location.

The old lineExecutionCounts.covmapping test actually had a buggy source
region in it. This commit introduces a regenerated copy of the coverage
and moves the old copy to malformedRegions.covmapping, for a test.

This is needed to unblock D36813, since the proposed segment builder
changes assert-fail when given bogus input.

[ikudrin]

It looks like Phabricator doesn't let me download binary files. But overall the patch looks good.

llvm/trunk/include/llvm/ProfileData/Coverage/CoverageMappingReader.h

struct CoverageMappingRecord {
   ArrayRef<StringRef> Filenames;
   ArrayRef<CounterExpression> Expressions;
   ArrayRef<CounterMappingRegion> MappingRegions;
 };
 
 /// \brief A file format agnostic iterator over coverage mapping data.
 class CoverageMappingIterator
     : public std::iterator<std::input_iterator_tag, CoverageMappingRecord> {
-  CoverageMappingReader *Reader = nullptr;
+  CoverageMappingReader *Reader;
   CoverageMappingRecord Record;
+  coveragemap_error ReadErr;
 
   void increment();
 
 public:
-  CoverageMappingIterator() = default;
+  CoverageMappingIterator()
+      : Reader(nullptr), Record(), ReadErr(coveragemap_error::success) {}
 
-  CoverageMappingIterator(CoverageMappingReader *Reader) : Reader(Reader) {
+  CoverageMappingIterator(CoverageMappingReader *Reader)
+      : Reader(Reader), Record(), ReadErr(coveragemap_error::success) {
     increment();
   }
 
+  ~CoverageMappingIterator() {
+    if (ReadErr != coveragemap_error::success)
+      llvm_unreachable("Unexpected error in coverage mapping iterator");
+  }
+
   CoverageMappingIterator &operator++() {
     increment();
     return *this;
   }
   bool operator==(const CoverageMappingIterator &RHS) {
     return Reader == RHS.Reader;
   }
   bool operator!=(const CoverageMappingIterator &RHS) {
     return Reader != RHS.Reader;
   }
-  CoverageMappingRecord &operator*() { return Record; }
-  CoverageMappingRecord *operator->() { return &Record; }
+  Expected<CoverageMappingRecord &> operator*() {
+    if (ReadErr != coveragemap_error::success) {
+      auto E = make_error<CoverageMapError>(ReadErr);
+      ReadErr = coveragemap_error::success;
+      return std::move(E);
+    }
+    return Record;
+  }
+  Expected<CoverageMappingRecord *> operator->() {
+    if (ReadErr != coveragemap_error::success) {
+      auto E = make_error<CoverageMapError>(ReadErr);
+      ReadErr = coveragemap_error::success;
+      return std::move(E);
+    }
+    return &Record;
+  }
 };
 
 class CoverageMappingReader {
 public:
   virtual ~CoverageMappingReader() = default;
 
   virtual Error readNextRecord(CoverageMappingRecord &Record) = 0;
   CoverageMappingIterator begin() { return CoverageMappingIterator(this); }

llvm/trunk/lib/ProfileData/Coverage/CoverageMapping.cpp

Error CoverageMapping::loadFunctionRecord(
   return Error::success();
 }
 
 Expected<std::unique_ptr<CoverageMapping>> CoverageMapping::load(
     ArrayRef<std::unique_ptr<CoverageMappingReader>> CoverageReaders,
     IndexedInstrProfReader &ProfileReader) {
   auto Coverage = std::unique_ptr<CoverageMapping>(new CoverageMapping());
 
-  for (const auto &CoverageReader : CoverageReaders)
-    for (const auto &Record : *CoverageReader)
+  for (const auto &CoverageReader : CoverageReaders) {
+    for (auto RecordOrErr : *CoverageReader) {
+      if (Error E = RecordOrErr.takeError())
+        return std::move(E);
+      const auto &Record = *RecordOrErr;
       if (Error E = Coverage->loadFunctionRecord(Record, ProfileReader))
         return std::move(E);
+    }
+  }
 
   return std::move(Coverage);
 }
 
 Expected<std::unique_ptr<CoverageMapping>>
 CoverageMapping::load(ArrayRef<StringRef> ObjectFilenames,
                       StringRef ProfileFilename, ArrayRef<StringRef> Arches) {
   auto ProfileReaderOrErr = IndexedInstrProfReader::create(ProfileFilename);

llvm/trunk/lib/ProfileData/Coverage/CoverageMappingReader.cpp

 
 using namespace llvm;
 using namespace coverage;
 using namespace object;
 
 #define DEBUG_TYPE "coverage-mapping"
 
 void CoverageMappingIterator::increment() {
+  if (ReadErr != coveragemap_error::success)
+    return;
+
   // Check if all the records were read or if an error occurred while reading
   // the next record.
-  if (auto E = Reader->readNextRecord(Record)) {
+  if (auto E = Reader->readNextRecord(Record))
     handleAllErrors(std::move(E), [&](const CoverageMapError &CME) {
       if (CME.get() == coveragemap_error::eof)
         *this = CoverageMappingIterator();
       else
-        llvm_unreachable("Unexpected error in coverage mapping iterator");
+        ReadErr = CME.get();
     });
-  }
-}
 
 Error RawCoverageReader::readULEB128(uint64_t &Result) {
   if (Data.empty())
     return make_error<CoverageMapError>(coveragemap_error::truncated);
   unsigned N = 0;
   Result = decodeULEB128(reinterpret_cast<const uint8_t *>(Data.data()), &N);
   if (N > Data.size())
     return make_error<CoverageMapError>(coveragemap_error::malformed);
DEBUG({
              << ColumnEnd << ", ";
       if (Kind == CounterMappingRegion::ExpansionRegion)
         dbgs() << "Expands to file " << ExpandedFileID;
       else
         CounterMappingContext(Expressions).dump(C, dbgs());
       dbgs() << "\n";
     });
 
-    MappingRegions.push_back(CounterMappingRegion(
-        C, InferredFileID, ExpandedFileID, LineStart, ColumnStart,
-        LineStart + NumLines, ColumnEnd, Kind));
+    auto CMR = CounterMappingRegion(C, InferredFileID, ExpandedFileID,
+                                    LineStart, ColumnStart,
+                                    LineStart + NumLines, ColumnEnd, Kind);
+    if (CMR.startLoc() > CMR.endLoc())
+      return make_error<CoverageMapError>(coveragemap_error::malformed);
+    MappingRegions.push_back(CMR);
   }
   return Error::success();
 }
 
 Error RawCoverageMappingReader::read() {
   // Read the virtual file mapping.
   SmallVector<unsigned, 8> VirtualFileMapping;
   uint64_t NumFileMappings;

llvm/trunk/lib/ProfileData/Coverage/CoverageMappingWriter.cpp

 }
 
 static void writeCounter(ArrayRef<CounterExpression> Expressions, Counter C,
                          raw_ostream &OS) {
   encodeULEB128(encodeCounter(Expressions, C), OS);
 }
 
 void CoverageMappingWriter::write(raw_ostream &OS) {
+  // Check that we don't have any bogus regions.
+  assert(all_of(MappingRegions,
+                [](const CounterMappingRegion &CMR) {
+                  return CMR.startLoc() <= CMR.endLoc();
+                }) &&
+         "Source region does not begin before it ends");
+
   // Sort the regions in an ascending order by the file id and the starting
   // location. Sort by region kinds to ensure stable order for tests.
   std::stable_sort(
       MappingRegions.begin(), MappingRegions.end(),
       [](const CounterMappingRegion &LHS, const CounterMappingRegion &RHS) {
         if (LHS.FileID != RHS.FileID)
           return LHS.FileID < RHS.FileID;
         if (LHS.startLoc() != RHS.startLoc())

llvm/trunk/test/tools/llvm-cov/Inputs/lineExecutionCounts.json

 // Metadata section
 // CHECK: {"version":"{{[0-9]\.[0-9]\.[0-9]}}","type":"llvm.coverage.json.export","data":[
 
 // Open Export
 // CHECK-SAME: {"files":[
 
 // File Object
 // CHECK-SAME: {"filename":"{{[^"]+}}showLineExecutionCounts.cpp",
 // CHECK-SAME: "segments":[
 // CHECK-SAME: {{(\[[0-9]+,[0-9]+,[0-9]+,[0-9]+,[0-9]+\],?)+}}],
 // CHECK-SAME: "expansions":[],
 
 // Verify the Summary Section for the first file
 // CHECK-SAME: "summary":{
 // CHECK-SAME: "lines":{"count":20,"covered":16,"percent":80},
 // CHECK-SAME: "functions":{"count":1,"covered":1,"percent":100},
-// CHECK-SAME: "regions":{"count":10,"covered":7,"notcovered":3,"percent":70}}}
+// CHECK-SAME: "regions":{"count":11,"covered":8,"notcovered":3,"percent":72}}}
 
 // Close Files Array
 // CHECK-SAME: ],
 
 // Functions List
 // CHECK-SAME: "functions":[
 // CHECK-SAME: {"name":"main","count":161,"regions":[
 // CHECK-SAME: {{(\[[0-9]+,[0-9]+,[0-9]+,[0-9]+,[0-9]+,[0-9]+,[0-9]+,[0-9]+\],?)+}}
 // CHECK-SAME: ],
 // CHECK-SAME: "filenames":["{{[^"]+}}showLineExecutionCounts.cpp"]
 // CHECK-SAME: }],
 
 
 // Full Export Summary
 // CHECK-SAME: "totals":{
 // CHECK-SAME: "lines":{"count":20,"covered":16,"percent":80},
 // CHECK-SAME: "functions":{"count":1,"covered":1,"percent":100},
 // CHECK-SAME: "instantiations":{"count":1,"covered":1,"percent":100},
-// CHECK-SAME: "regions":{"count":10,"covered":7,"notcovered":3,"percent":70}}
+// CHECK-SAME: "regions":{"count":11,"covered":8,"notcovered":3,"percent":72}}
 
 // Close the export object, data array, and root object
 // CHECK-SAME: }]}

llvm/trunk/test/tools/llvm-cov/showLineExecutionCounts.cpp

 // RUN: FileCheck -check-prefixes=HTML,HTML-WHOLE-FILE -input-file %t.html.dir/coverage/tmp/showLineExecutionCounts.cpp.html %s
 // RUN: FileCheck -check-prefixes=HTML,HTML-FILTER -input-file %t.html.dir/functions.html %s
 //
 // HTML-WHOLE-FILE: <td class='line-number'><a name='L[[@LINE-44]]' href='#L[[@LINE-44]]'><pre>[[@LINE-44]]</pre></a></td><td class='uncovered-line'></td><td class='code'><pre>// before
 // HTML-FILTER-NOT: <td class='line-number'><a name='L[[@LINE-45]]' href='#L[[@LINE-45]]'><pre>[[@LINE-45]]</pre></a></td><td class='uncovered-line'></td><td class='code'><pre>// before
 // HTML: <td class='line-number'><a name='L[[@LINE-44]]' href='#L[[@LINE-44]]'><pre>[[@LINE-44]]</pre></a></td><td class='covered-line'><pre>161</pre></td><td class='code'><pre>int main() {
 // HTML: <td class='line-number'><a name='L[[@LINE-44]]' href='#L[[@LINE-44]]'><pre>[[@LINE-44]]</pre></a></td><td class='covered-line'><pre>161</pre></td><td class='code'><pre>  int x = 0
 // HTML: <td class='line-number'><a name='L[[@LINE-44]]' href='#L[[@LINE-44]]'><pre>[[@LINE-44]]</pre></a></td><td class='covered-line'><pre>161</pre></td><td class='code'><pre>
-// HTML: <td class='line-number'><a name='L[[@LINE-44]]' href='#L[[@LINE-44]]'><pre>[[@LINE-44]]</pre></a></td><td class='covered-line'><pre>161</pre></td><td class='code'><pre>  if (x) {
+// HTML: <td class='line-number'><a name='L[[@LINE-44]]' href='#L[[@LINE-44]]'><pre>[[@LINE-44]]</pre></a></td><td class='covered-line'><pre>161</pre></td><td class='code'><pre>  if (x)
 // HTML: <td class='line-number'><a name='L[[@LINE-44]]' href='#L[[@LINE-44]]'><pre>[[@LINE-44]]</pre></a></td><td class='uncovered-line'><pre>0</pre></td><td class='code'><pre>
 // HTML: <td class='line-number'><a name='L[[@LINE-44]]' href='#L[[@LINE-44]]'><pre>[[@LINE-44]]</pre></a></td><td class='covered-line'><pre>161</pre></td><td class='code'><pre><span class='red'>  }</span>
 // HTML: <td class='line-number'><a name='L[[@LINE-44]]' href='#L[[@LINE-44]]'><pre>[[@LINE-44]]</pre></a></td><td class='covered-line'><pre>161</pre></td><td class='code'><pre>    x = 1;
 // HTML: <td class='line-number'><a name='L[[@LINE-44]]' href='#L[[@LINE-44]]'><pre>[[@LINE-44]]</pre></a></td><td class='covered-line'><pre>161</pre></td><td class='code'><pre>  }
 // HTML: <td class='line-number'><a name='L[[@LINE-44]]' href='#L[[@LINE-44]]'><pre>[[@LINE-44]]</pre></a></td><td class='covered-line'><pre>161</pre></td><td class='code'><pre>
 // HTML: <td class='line-number'><a name='L[[@LINE-44]]' href='#L[[@LINE-44]]'><pre>[[@LINE-44]]</pre></a></td><td class='covered-line'><pre>16.2k</pre></td><td class='code'><pre>  for (int i = 0; i &lt; 100; ++i)
 // HTML: <td class='line-number'><a name='L[[@LINE-44]]' href='#L[[@LINE-44]]'><pre>[[@LINE-44]]</pre></a></td><td class='covered-line'><pre>16.1k</pre></td><td class='code'><pre>    x = 1;
 // HTML: <td class='line-number'><a name='L[[@LINE-44]]' href='#L[[@LINE-44]]'><pre>[[@LINE-44]]</pre></a></td><td class='covered-line'><pre>16.1k</pre></td><td class='code'><pre>  }
 // HTML: <td class='line-number'><a name='L[[@LINE-44]]' href='#L[[@LINE-44]]'><pre>[[@LINE-44]]</pre></a></td><td class='covered-line'><pre>161</pre></td><td class='code'><pre>
 // HTML: <td class='line-number'><a name='L[[@LINE-44]]' href='#L[[@LINE-44]]'><pre>[[@LINE-44]]</pre></a></td><td class='covered-line'><pre>161</pre></td><td class='code'><pre>  x = x &lt; 10
 // HTML: <td class='line-number'><a name='L[[@LINE-44]]' href='#L[[@LINE-44]]'><pre>[[@LINE-44]]</pre></a></td><td class='covered-line'><pre>161</pre></td><td class='code'><pre>  x = x &gt; 10
-// HTML: <td class='line-number'><a name='L[[@LINE-44]]' href='#L[[@LINE-44]]'><pre>[[@LINE-44]]</pre></a></td><td class='uncovered-line'><pre>0</pre></td><td class='code'><pre>        x - 1:
+// HTML: <td class='line-number'><a name='L[[@LINE-44]]' href='#L[[@LINE-44]]'><pre>[[@LINE-44]]</pre></a></td><td class='uncovered-line'><pre>0</pre></td><td class='code'><pre> <span class='red'>x - 1</span>:
 // HTML: <td class='line-number'><a name='L[[@LINE-44]]' href='#L[[@LINE-44]]'><pre>[[@LINE-44]]</pre></a></td><td class='covered-line'><pre>161</pre></td><td class='code'><pre>        x + 1;
 // HTML: <td class='line-number'><a name='L[[@LINE-44]]' href='#L[[@LINE-44]]'><pre>[[@LINE-44]]</pre></a></td><td class='covered-line'><pre>161</pre></td><td class='code'><pre>
 // HTML: <td class='line-number'><a name='L[[@LINE-44]]' href='#L[[@LINE-44]]'><pre>[[@LINE-44]]</pre></a></td><td class='covered-line'><pre>161</pre></td><td class='code'><pre>  return 0;
 // HTML: <td class='line-number'><a name='L[[@LINE-44]]' href='#L[[@LINE-44]]'><pre>[[@LINE-44]]</pre></a></td><td class='covered-line'><pre>161</pre></td><td class='code'><pre>}
 // HTML-WHOLE-FILE: <td class='line-number'><a name='L[[@LINE-44]]' href='#L[[@LINE-44]]'><pre>[[@LINE-44]]</pre></a></td><td class='uncovered-line'></td><td class='code'><pre>// after
 // HTML-FILTER-NOT: <td class='line-number'><a name='L[[@LINE-45]]' href='#L[[@LINE-45]]'><pre>[[@LINE-45]]</pre></a></td><td class='uncovered-line'></td><td class='code'><pre>// after
 //
 // Test index creation.

 // HTML-INDEX: <a href='coverage{{.*}}showLineExecutionCounts.cpp.html'{{.*}}showLineExecutionCounts.cpp</a>
 // HTML-INDEX: <td class='column-entry-green'>
 // HTML-INDEX: 100.00% (1/1)
 // HTML-INDEX: <td class='column-entry-green'>
 // HTML-INDEX: 100.00% (1/1)
 // HTML-INDEX: <td class='column-entry-yellow'>
 // HTML-INDEX: 80.00% (16/20)
 // HTML-INDEX: <td class='column-entry-red'>
-// HTML-INDEX: 70.00% (7/10)
+// HTML-INDEX: 72.73% (8/11)
 // HTML-INDEX: TOTALS

llvm/trunk/test/tools/llvm-cov/warnings.h

 // RUN: llvm-cov show %S/Inputs/prevent_false_instantiations.covmapping -instr-profile %S/Inputs/elf_binary_comdat.profdata -path-equivalence=/tmp,%S /dev/null | FileCheck %s -allow-empty -check-prefix=FAKE-FILE-STDOUT
 // RUN: llvm-cov show %S/Inputs/prevent_false_instantiations.covmapping -instr-profile %S/Inputs/elf_binary_comdat.profdata -path-equivalence=/tmp,%S /dev/null 2>&1 | FileCheck %s -check-prefix=FAKE-FILE-STDERR
 // RUN: not llvm-cov report %S/Inputs/prevent_false_instantiations.covmapping -instr-profile %S/Inputs/elf_binary_comdat.profdata -format=html
 // RUN: not llvm-cov export %S/Inputs/prevent_false_instantiations.covmapping -instr-profile %S/Inputs/elf_binary_comdat.profdata -format=html
 
 // FAKE-FILE-STDOUT-NOT: warning: The file '{{.*}}' isn't covered.
 // FAKE-FILE-STDERR: warning: The file '{{.*}}' isn't covered.
 
 // RUN: llvm-cov show %S/Inputs/prevent_false_instantiations.covmapping -instr-profile %S/Inputs/elf_binary_comdat.profdata -path-equivalence=/tmp,%S -name ".*" /dev/null | FileCheck %s -allow-empty -check-prefix=FAKE-FUNC-STDOUT
 // RUN: llvm-cov show %S/Inputs/prevent_false_instantiations.covmapping -instr-profile %S/Inputs/elf_binary_comdat.profdata -path-equivalence=/tmp,%S -name-regex ".*" /dev/null 2>&1 | FileCheck %s -check-prefix=FAKE-FUNC-STDERR
 
 // FAKE-FUNC-STDOUT-NOT: warning: Could not read coverage for '{{.*}}'.
 // FAKE-FUNC-STDERR: Could not read coverage for '{{.*}}'.
+
+// RUN: not llvm-cov report %S/Inputs/malformedRegions.covmapping -instr-profile %S/Inputs/elf_binary_comdat.profdata 2>&1 | FileCheck %s -check-prefix=MALFORMED-REGION
+// MALFORMED-REGION: malformedRegions.covmapping: Failed to load coverage: Malformed coverage data