Ignore object files that lack coverage information. Before this change, if multiple binary files were presented, all of them must have been instrumented or the load would fail with coverage_map_error::no_data_found.

[vsk]

Thanks for the patch. I'd like for the test to be minimized as far as possible, e.g.:

obj1.o: not instrumented, no functions
obj2.o: instrumented, just contains 'void f1() {}'

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

317 ↗

(On Diff #217211)

Could you simply check whether 'Readers' is empty after the loop, and return data_not_found if so? Then there's no need to update an Error object.

llvm/lib/ProfileData/InstrProfReader.cpp

45 ↗

(On Diff #217211)

Perhaps this could be a follow-up change? I'm not sure what it has to do with the main change.

[deansturtevant]

We can simplify by not propagating data_not_found, but rather manufacturing one if desired.
However, that would result in any information in the original payload being lost.
Let me know if that is ok.

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

317 ↗

(On Diff #217211)

That would change the logic so that passing in an empty ObjectFilenames would result in a data_not_found error. I'm not convinced that's the desired behavior, but would be willing to implement it if you think it is.

Please let me know what your preference is.

llvm/lib/ProfileData/InstrProfReader.cpp

45 ↗

(On Diff #217211)

I will remove it.

[vsk]

Thanks, it should be fine to ignore the payload of the data_not_found error.

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

317 ↗

(On Diff #217211)

Sure, this should just create an error if ObjectFilenames is non-empty.

[deansturtevant]

Thanks. Please take another look.

[vsk]

In D66763#1647073, @deansturtevant wrote:

Thanks. Please take another look.

Hm, perhaps the diff update was dropped somehow? I don’t see a change yet.

[deansturtevant]

Responded to review comments. Simplified the code and the tests.

[deansturtevant]

Sorry. User error. Should be up now.

[vsk]

Looks great. Thank you!

[deansturtevant]

Thanks for the review including some great suggestions.
I do not have a commit account. Could you commit this for me? Thanks again.

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

public:
 
   /// Load the coverage mapping using the given readers.
   static Expected<std::unique_ptr<CoverageMapping>>
   load(ArrayRef<std::unique_ptr<CoverageMappingReader>> CoverageReaders,
        IndexedInstrProfReader &ProfileReader);
 
   /// Load the coverage mapping from the given object files and profile. If
   /// \p Arches is non-empty, it must specify an architecture for each object.
+  /// Ignores non-instrumented object files unless all are not instrumented.
   static Expected<std::unique_ptr<CoverageMapping>>
   load(ArrayRef<StringRef> ObjectFilenames, StringRef ProfileFilename,
        ArrayRef<StringRef> Arches = None);
 
   /// The number of functions that couldn't have their profiles mapped.
   ///
   /// This is a count of functions whose profile is out of date or otherwise
   /// can't be associated with any coverage information.

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

for (auto RecordOrErr : *CoverageReader) {
       if (Error E = Coverage->loadFunctionRecord(Record, ProfileReader))
         return std::move(E);
     }
   }
 
   return std::move(Coverage);
 }
 
+// If E is a no_data_found error, returns success. Otherwise returns E.
+static Error handleMaybeNoDataFoundError(Error E) {
+  return handleErrors(
+      std::move(E), [](const CoverageMapError &CME) {
+        if (CME.get() == coveragemap_error::no_data_found)
+          return static_cast<Error>(Error::success());
+        return make_error<CoverageMapError>(CME.get());
+      });
+}
+
 Expected<std::unique_ptr<CoverageMapping>>
 CoverageMapping::load(ArrayRef<StringRef> ObjectFilenames,
                       StringRef ProfileFilename, ArrayRef<StringRef> Arches) {
   auto ProfileReaderOrErr = IndexedInstrProfReader::create(ProfileFilename);
   if (Error E = ProfileReaderOrErr.takeError())
     return std::move(E);
   auto ProfileReader = std::move(ProfileReaderOrErr.get());
 
   SmallVector<std::unique_ptr<CoverageMappingReader>, 4> Readers;
   SmallVector<std::unique_ptr<MemoryBuffer>, 4> Buffers;
   for (const auto &File : llvm::enumerate(ObjectFilenames)) {
     auto CovMappingBufOrErr = MemoryBuffer::getFileOrSTDIN(File.value());
     if (std::error_code EC = CovMappingBufOrErr.getError())
       return errorCodeToError(EC);
     StringRef Arch = Arches.empty() ? StringRef() : Arches[File.index()];
     MemoryBufferRef CovMappingBufRef =
         CovMappingBufOrErr.get()->getMemBufferRef();
     auto CoverageReadersOrErr =
         BinaryCoverageReader::create(CovMappingBufRef, Arch, Buffers);
-    if (Error E = CoverageReadersOrErr.takeError())
+    if (Error E = CoverageReadersOrErr.takeError()) {
+      E = handleMaybeNoDataFoundError(std::move(E));
+      if (E)
         return std::move(E);
+      // E == success (originally a no_data_found error).
+      continue;
+    }
     for (auto &Reader : CoverageReadersOrErr.get())
       Readers.push_back(std::move(Reader));
     Buffers.push_back(std::move(CovMappingBufOrErr.get()));
   }
+  // If no readers were created, either no objects were provided or none of them
+  // had coverage data. Return an error in the latter case.
+  if (Readers.empty() && !ObjectFilenames.empty())
+    return make_error<CoverageMapError>(coveragemap_error::no_data_found);
   return load(Readers, *ProfileReader);
 }
 
 namespace {
 
 /// Distributes functions into instantiation sets.
 ///
 /// An instantiation set is a collection of functions that have the same source

llvm/trunk/test/tools/llvm-cov/Inputs/multiple_objects_not_all_instrumented/instrumented.cc

+void f1() {}
+
+int main(int argc, char** argv) {
+  f1();
+}

llvm/trunk/test/tools/llvm-cov/Inputs/multiple_objects_not_all_instrumented/not_instrumented.cc

+int main(int argc, char** argv) {}

llvm/trunk/test/tools/llvm-cov/multiple-objects-not-all-instrumented.test

+RUN: llvm-cov export --format=lcov --instr-profile=%S/Inputs/multiple_objects_not_all_instrumented/instrumented.profdata \
+RUN:   -object %S/Inputs/multiple_objects_not_all_instrumented/not_instrumented \
+RUN:   -object %S/Inputs/multiple_objects_not_all_instrumented/instrumented | FileCheck -check-prefix=FN %s
+
+FN:1,_Z2f1v
+
+Instructions for regenerating the test:
+
+clang -std=c++11 not_instrumented.cc -o not_instrumented
+clang -std=c++11 -mllvm -enable-name-compression=false -fprofile-instr-generate -fcoverage-mapping instrumented.cc -o instrumented
+LLVM_PROFILE_FILE="instrumented.raw" ./instrumented
+llvm-profdata merge instrumented.raw -o instrumented.profdata