[ProfileData] Add option to ignore invalid regions

[modocache]

Summary

Clang's source location information reporting is always improving, but
llvm-cov and LLVM's profiling data reporters are "all-or-nothing": if
just a single region of code coverage data is invalid or malformed, the
entire coverage data file is scrapped, and llvm-cov reports "Failed to
load coverage: Malformed coverage data".

The C++ developers I support rely heavily on coverage data from their
unit tests, and this all-or-nothing approach to reporting has caused that
data to be unreliable. I fixed a recent incident of "Failed to load
coverage" by patching D65428 into an older internal release of Clang
that we use, to fix location information for a single function that had
caused thousands of other functions to not be reported. More recently,
I've discovered that Clang generates invalid coverage data for C++20
coroutines, and this prevents coverage data from being reported for the
entire unit.

Before I can send up a fix for C++20 coroutines, I thought I'd provide a
stop-gap solution: allow developers to invoke
llvm-cov -ignore-malformed-coverage-mapping-regions, which will simply
drop mapping regions with invalid source location information. Using
this option would have allowed the developers I support to get coverage
data for most of their source code, even when Clang could not generate
valid information for specific regions (such as prior to D65428, or prior
to whatever I send up for coroutines).

[vsk]

I'm not sure this is the right thing to do. Do you see the 'malformed coverage data' error reported when there isn't a compiler bug? I worry that if this option were made available, frontend support for new language features would lag behind.

Mechanically I think this patch is ok, although I suspect there are more places the coverage reader detects malformed data and subsequently bails out.

[modocache]

I see. No, I think whenever we have malformed coverage data, it points to an underlying bug in the compiler. Still, I think llvm-cov's treatment of invalid data is too severe. Is there a good reason it should bail entirely? If it's able to process all but a single coverage region, it seems like it ought to report what it was able to process, no? I think that at least having the option for llvm-cov to behave this way is helpful, although I can understand if others disagree.

For the time being, I'm working to fix the coverage data flaw in Clang's support of C++20 coroutines. But just to reiterate: it's not just coroutines and new language features that have caused invalid regions to appear in coverage data, there's also been cases such as D65428, which as far as I can tell is a bug in a long-existing language feature. In a sense, it's nice that llvm-cov "fails fast" and reports invalid data, but pragmatically it's also nice to have the capability to make it resilient to frontend bugs. At least, that's what I think -- let me know if you disagree :)

[vsk]

In D76016#1932039, @modocache wrote:

I see. No, I think whenever we have malformed coverage data, it points to an underlying bug in the compiler. Still, I think llvm-cov's treatment of invalid data is too severe. Is there a good reason it should bail entirely? If it's able to process all but a single coverage region, it seems like it ought to report what it was able to process, no? I think that at least having the option for llvm-cov to behave this way is helpful, although I can understand if others disagree.

For the time being, I'm working to fix the coverage data flaw in Clang's support of C++20 coroutines. But just to reiterate: it's not just coroutines and new language features that have caused invalid regions to appear in coverage data, there's also been cases such as D65428, which as far as I can tell is a bug in a long-existing language feature. In a sense, it's nice that llvm-cov "fails fast" and reports invalid data, but pragmatically it's also nice to have the capability to make it resilient to frontend bugs. At least, that's what I think -- let me know if you disagree :)

I definitely see your point of view. Actually I had a similar perspective when I first started working on this feature. The "fail hard, early" approach created.. quite a bit of up-front work. But the upshot was that we had more confidence in the coverage reports. And we didn't have to add tricky workarounds to hide partially incorrect data (incidentally, re: this patch, if a CounterMappingRegion got messed up, could it be that other CMRs in the function are not quite right?). If compiler bugs which break the coverage workflow are relatively rare (and afaict they now are), istm it's feasible/preferable to fix them early, instead of fleshing out and maintaining a separate alpha-quality coverage reporting mode.

[modocache]

Yup, makes sense. In the codebase I maintain, I think there are a few distinct coverage issues, so I'll keep this patch applied to my fork of LLVM for the time being, until I send patches to fix those issues. But it sounds like this isn't something that we'd want in LLVM trunk, so I'll close this out. Thanks!

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

public:
   RawCoverageFilenamesReader(const RawCoverageFilenamesReader &) = delete;
   RawCoverageFilenamesReader &
   operator=(const RawCoverageFilenamesReader &) = delete;
 
   Error read(CovMapVersion Version,
              BinaryCoverageReader::DecompressedData &Decompressed);
 };
 
+// Whether to ignore coverage mapping regions that are invalid, for example
+// regions for which the frontend has indicated the start location comes after
+// the end location. These are typically bugs in the frontend generating source
+// location information, but by enabling this option users can have coverage
+// information for regions such as these silently ignored.
+extern cl::opt<bool> IgnoreMalformedCoverageMappingRegions;
+
 } // end namespace coverage
 } // end namespace llvm
 
 #endif // LLVM_PROFILEDATA_COVERAGE_COVERAGEMAPPINGREADER_H

llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp

 using namespace coverage;
 using namespace object;
 
 #define DEBUG_TYPE "coverage-mapping"
 
 STATISTIC(CovMapNumRecords, "The # of coverage function records");
 STATISTIC(CovMapNumUsedRecords, "The # of used coverage function records");
 
+cl::opt<bool> coverage::IgnoreMalformedCoverageMappingRegions(
+    "ignore-malformed-coverage-mapping-regions",
+    cl::desc("Ignore malformed code coverage mapping regions"),
+    cl::init(false));
+
 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))
     handleAllErrors(std::move(E), [&](const CoverageMapError &CME) {
LLVM_DEBUG({
       else
         CounterMappingContext(Expressions).dump(C, dbgs());
       dbgs() << "\n";
     });
 
     auto CMR = CounterMappingRegion(C, InferredFileID, ExpandedFileID,
                                     LineStart, ColumnStart,
                                     LineStart + NumLines, ColumnEnd, Kind);
-    if (CMR.startLoc() > CMR.endLoc())
+    if (CMR.startLoc() > CMR.endLoc()) {
+      LLVM_DEBUG(dbgs() << "Malformed coverage mapping region: "
+                        << CMR.startLoc().first << ":" << CMR.startLoc().second
+                        << " -> " << CMR.endLoc().first << ":"
+                        << CMR.endLoc().second << "\n");
+      if (IgnoreMalformedCoverageMappingRegions)
+        continue;
+      else
         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;

llvm/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
+// RUN: llvm-cov report -ignore-malformed-coverage-mapping-regions %S/Inputs/malformedRegions.covmapping -instr-profile %S/Inputs/elf_binary_comdat.profdata 2>&1 | FileCheck %s -check-prefix=IGNORE-MALFORMED-REGION
+// IGNORE-MALFORMED-REGION: warning: 1 functions have mismatched data
+// IGNORE-MALFORMED-REGION-NOT: Failed to load coverage