llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-pgo Author: Fabian Meumertzheim (fmeum) <details> <summary>Changes</summary> The coverage mapping of a function that is unused in a TU (e.g. an inline function defined in a header) consists of a single zero region spanning the whole body. Since it doesn't contain skipped regions, comments, empty lines and code excluded by the preprocessor in the body are reported as uncovered lines if this mapping is used without the mapping of the same function from a TU that uses it. This is the case when generating baseline coverage with llvm-cov's -empty-profile for the object files of a library, where it results in uncovered lines that can never be covered. Emit skipped regions in mappings of unused functions just like for used functions and update the detection of such mappings in llvm-cov to allow for skipped regions, so that mappings of unused functions are still ignored in favor of those from TUs that use the function. Assisted-by: Claude Code --- Full diff: https://github.com/llvm/llvm-project/pull/232831.diff 5 Files Affected: - (modified) clang/lib/CodeGen/CoverageMappingGen.cpp (+6) - (added) clang/test/CoverageMapping/unused_function_skipped.cpp (+20) - (modified) llvm/lib/ProfileData/Coverage/CoverageMapping.cpp (+12-7) - (modified) llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp (+36-9) - (modified) llvm/unittests/ProfileData/CoverageMappingTest.cpp (+39) ``````````diff diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp index 970812f7b0f75..ccc7ac6debe52 100644 --- a/clang/lib/CodeGen/CoverageMappingGen.cpp +++ b/clang/lib/CodeGen/CoverageMappingGen.cpp @@ -648,6 +648,12 @@ struct EmptyCoverageMappingBuilder : public CoverageMappingBuilder { SmallVector<unsigned, 16> FileIDMapping; gatherFileIDs(FileIDMapping); emitSourceRegions(SourceRegionFilter()); + // As for used functions, mark comments, empty lines and code excluded by + // the preprocessor as skipped. Otherwise, the lines they are on would be + // reported as uncovered code when the mapping of an unused function is + // used without a mapping of the same function from another TU, e.g. to + // generate baseline coverage with llvm-cov's -empty-profile. + gatherSkippedRegions(); if (MappingRegions.empty()) return; diff --git a/clang/test/CoverageMapping/unused_function_skipped.cpp b/clang/test/CoverageMapping/unused_function_skipped.cpp new file mode 100644 index 0000000000000..eeac7c67e994c --- /dev/null +++ b/clang/test/CoverageMapping/unused_function_skipped.cpp @@ -0,0 +1,20 @@ +// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only %s | FileCheck %s + +// Comments and code excluded by the preprocessor in the body of an unused +// function are marked as skipped, just like in used functions. Otherwise, they +// would be reported as uncovered lines if the unused function's mapping is +// used on its own, e.g. by llvm-cov with -empty-profile. + +// CHECK-LABEL: {{_Z6unusedi|\?unused@@YAHH@Z}}: +// CHECK-NEXT: File 0, [[@LINE+4]]:26 -> [[@LINE+11]]:2 = 0 +// CHECK-NEXT: Skipped,File 0, [[@LINE+4]]:1 -> [[@LINE+4]]:16 = 0 +// CHECK-NEXT: Skipped,File 0, [[@LINE+5]]:1 -> [[@LINE+7]]:7 = 0 +// CHECK-NOT: Skipped +inline int unused(int x) { + // A comment. + ++x; +#if 0 + ++x; +#endif + return x; +} diff --git a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp index cbb048d15bca2..4e182eb382d9c 100644 --- a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp +++ b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp @@ -923,13 +923,18 @@ Error CoverageMapping::loadFunctionRecord( assert(!Record.MappingRegions.empty() && "Function has no regions"); - // This coverage record is a zero region for a function that's unused in - // some TU, but used in a different TU. Ignore it. The coverage maps from the - // the other TU will either be loaded (providing full region counts) or they - // won't (in which case we don't unintuitively report functions as uncovered - // when they have non-zero counts in the profile). - if (Record.MappingRegions.size() == 1 && - Record.MappingRegions[0].Count.isZero() && Counts[0] > 0) + // This coverage record is a zero region (possibly accompanied by skipped + // regions) for a function that's unused in some TU, but used in a different + // TU. Ignore it. The coverage maps from the the other TU will either be + // loaded (providing full region counts) or they won't (in which case we don't + // unintuitively report functions as uncovered when they have non-zero counts + // in the profile). + auto NonSkippedRegions = llvm::make_filter_range( + Record.MappingRegions, [](const CounterMappingRegion &Region) { + return Region.Kind != CounterMappingRegion::SkippedRegion; + }); + if (llvm::hasSingleElement(NonSkippedRegions) && + NonSkippedRegions.begin()->Count.isZero() && Counts[0] > 0) return Error::success(); FunctionRecord Function(OrigFuncName, Record.Filenames); diff --git a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp index e89efe09d49e8..0ba34c007870d 100644 --- a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp +++ b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp @@ -470,7 +470,8 @@ Error RawCoverageMappingReader::read() { } Expected<bool> RawCoverageMappingDummyChecker::isDummy() { - // A dummy coverage mapping data consists of just one region with zero count. + // A dummy coverage mapping data consists of just one region with zero count, + // possibly accompanied by skipped regions. uint64_t NumFileMappings; if (Error Err = readSize(NumFileMappings)) return std::move(Err); @@ -489,14 +490,40 @@ Expected<bool> RawCoverageMappingDummyChecker::isDummy() { uint64_t NumRegions; if (Error Err = readSize(NumRegions)) return std::move(Err); - if (NumRegions != 1) - return false; - uint64_t EncodedCounterAndRegion; - if (Error Err = readIntMax(EncodedCounterAndRegion, - std::numeric_limits<unsigned>::max())) - return std::move(Err); - unsigned Tag = EncodedCounterAndRegion & Counter::EncodingTagMask; - return Tag == Counter::Zero; + unsigned NumCodeRegions = 0; + for (uint64_t I = 0; I < NumRegions; ++I) { + uint64_t EncodedCounterAndRegion; + if (Error Err = readIntMax(EncodedCounterAndRegion, + std::numeric_limits<unsigned>::max())) + return std::move(Err); + unsigned Tag = EncodedCounterAndRegion & Counter::EncodingTagMask; + if (Tag != Counter::Zero || + (EncodedCounterAndRegion & EncodingExpansionRegionBit)) + return false; + switch (EncodedCounterAndRegion >> + Counter::EncodingCounterTagAndExpansionRegionTagBits) { + case CounterMappingRegion::CodeRegion: + if (++NumCodeRegions > 1) + return false; + break; + case CounterMappingRegion::SkippedRegion: + break; + default: + return false; + } + // Skip over the source range. + uint64_t LineStartDelta, ColumnStart, NumLines, ColumnEnd; + if (Error Err = + readIntMax(LineStartDelta, std::numeric_limits<unsigned>::max())) + return std::move(Err); + if (Error Err = readULEB128(ColumnStart)) + return std::move(Err); + if (Error Err = readIntMax(NumLines, std::numeric_limits<unsigned>::max())) + return std::move(Err); + if (Error Err = readIntMax(ColumnEnd, std::numeric_limits<unsigned>::max())) + return std::move(Err); + } + return NumCodeRegions == 1; } /// Determine if we should skip the first byte of the section content diff --git a/llvm/unittests/ProfileData/CoverageMappingTest.cpp b/llvm/unittests/ProfileData/CoverageMappingTest.cpp index b268aa7cdd057..a953bed8bacf6 100644 --- a/llvm/unittests/ProfileData/CoverageMappingTest.cpp +++ b/llvm/unittests/ProfileData/CoverageMappingTest.cpp @@ -1062,10 +1062,49 @@ TEST_P(CoverageMappingTest, skip_duplicate_function_record) { ASSERT_EQ(3U, NumFuncs); } +TEST_P(CoverageMappingTest, skip_unused_function_record_with_skipped_regions) { + ProfileWriter.addRecord({"func", 0x1234, {1}}, Err); + + // A record emitted for a function that's unused in some TU: a single zero + // region, possibly accompanied by skipped regions for comments. Since the + // function has a non-zero count in the profile, it should be skipped. + startFunction("func", 0x1234); + addCMR(Counter::getZero(), "file1", 1, 1, 9, 9); + addSkipped("file1", 2, 1, 2, 10); + addSkipped("file1", 4, 1, 6, 7); + + EXPECT_THAT_ERROR(loadCoverageMapping(), Succeeded()); + + auto Funcs = LoadedCoverage->getCoveredFunctions(); + EXPECT_EQ(0, std::distance(Funcs.begin(), Funcs.end())); +} + INSTANTIATE_TEST_SUITE_P(ParameterizedCovMapTest, CoverageMappingTest, ::testing::Combine(::testing::Bool(), ::testing::Bool())); +TEST(CoverageMappingTest, dummy_mapping_detection) { + auto IsDummy = [](std::vector<CounterMappingRegion> Regions) { + std::string Mapping; + raw_string_ostream OS(Mapping); + unsigned FileIDs[] = {0}; + CoverageMappingWriter(FileIDs, {}, Regions).write(OS); + return RawCoverageMappingDummyChecker(Mapping).isDummy(); + }; + auto ZeroRegion = + CounterMappingRegion::makeRegion(Counter::getZero(), 0, 1, 1, 9, 9); + auto CountedRegion = + CounterMappingRegion::makeRegion(Counter::getCounter(0), 0, 1, 1, 9, 9); + auto SkippedRegion = CounterMappingRegion::makeSkipped(0, 2, 1, 2, 10); + + EXPECT_THAT_EXPECTED(IsDummy({ZeroRegion}), HasValue(true)); + EXPECT_THAT_EXPECTED(IsDummy({ZeroRegion, SkippedRegion}), HasValue(true)); + EXPECT_THAT_EXPECTED(IsDummy({ZeroRegion, ZeroRegion}), HasValue(false)); + EXPECT_THAT_EXPECTED(IsDummy({CountedRegion}), HasValue(false)); + EXPECT_THAT_EXPECTED(IsDummy({CountedRegion, SkippedRegion}), + HasValue(false)); +} + TEST(CoverageMappingTest, filename_roundtrip) { std::vector<std::string> Paths({"dir", "a", "b", "c", "d", "e"}); `````````` </details> https://github.com/llvm/llvm-project/pull/232831 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
