Revert "[MC/DC] Make covmap tolerant of nested Decisions (#125407)"#181069
Merged
Conversation
This reverts commit 8f690ec.
gulfemsavrun
temporarily deployed
to
main-branch-only
GitHub Actions
Inactive
llvmbot
temporarily deployed
to
main-branch-only
GitHub Actions
Inactive
Member
|
@llvm/pr-subscribers-pgo Author: None (gulfemsavrun) ChangesThis reverts commit 8f690ec. Full diff: https://github.com/llvm/llvm-project/pull/181069.diff 2 Files Affected:
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
index b63f105e93513..40bbb9b194520 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -685,166 +685,154 @@ static unsigned getMaxBitmapSize(const CoverageMappingRecord &Record,
namespace {
-/// Walk MappingRegions along Expansions and emit CountedRegions.
-struct CountedRegionEmitter {
- /// A nestable Decision.
+/// Collect Decisions, Branchs, and Expansions and associate them.
+class MCDCDecisionRecorder {
+private:
+ /// This holds the DecisionRegion and MCDCBranches under it.
+ /// Also traverses Expansion(s).
+ /// The Decision has the number of MCDCBranches and will complete
+ /// when it is filled with unique ConditionID of MCDCBranches.
struct DecisionRecord {
const CounterMappingRegion *DecisionRegion;
- unsigned NumConditions; ///< Copy of DecisionRegion.NumConditions
- /// Pushed by traversal order.
+
+ /// They are reflected from DecisionRegion for convenience.
+ mcdc::DecisionParameters DecisionParams;
+ LineColPair DecisionStartLoc;
+ LineColPair DecisionEndLoc;
+
+ /// This is passed to `MCDCRecordProcessor`, so this should be compatible
+ /// to`ArrayRef<const CounterMappingRegion *>`.
SmallVector<const CounterMappingRegion *> MCDCBranches;
-#ifndef NDEBUG
+
+ /// IDs that are stored in MCDCBranches
+ /// Complete when all IDs (1 to NumConditions) are met.
DenseSet<mcdc::ConditionID> ConditionIDs;
-#endif
+
+ /// Set of IDs of Expansion(s) that are relevant to DecisionRegion
+ /// and its children (via expansions).
+ /// FileID pointed by ExpandedFileID is dedicated to the expansion, so
+ /// the location in the expansion doesn't matter.
+ DenseSet<unsigned> ExpandedFileIDs;
DecisionRecord(const CounterMappingRegion &Decision)
: DecisionRegion(&Decision),
- NumConditions(Decision.getDecisionParams().NumConditions) {
+ DecisionParams(Decision.getDecisionParams()),
+ DecisionStartLoc(Decision.startLoc()),
+ DecisionEndLoc(Decision.endLoc()) {
assert(Decision.Kind == CounterMappingRegion::MCDCDecisionRegion);
}
- bool pushBranch(const CounterMappingRegion &B) {
- assert(B.Kind == CounterMappingRegion::MCDCBranchRegion);
- assert(ConditionIDs.insert(B.getBranchParams().ID).second &&
- "Duplicate CondID");
- MCDCBranches.push_back(&B);
- assert(MCDCBranches.size() <= NumConditions &&
- "MCDCBranch exceeds NumConds");
- return (MCDCBranches.size() == NumConditions);
+ /// Determine whether DecisionRecord dominates `R`.
+ bool dominates(const CounterMappingRegion &R) const {
+ // Determine whether `R` is included in `DecisionRegion`.
+ if (R.FileID == DecisionRegion->FileID &&
+ R.startLoc() >= DecisionStartLoc && R.endLoc() <= DecisionEndLoc)
+ return true;
+
+ // Determine whether `R` is pointed by any of Expansions.
+ return ExpandedFileIDs.contains(R.FileID);
}
- };
- const CoverageMappingRecord &Record;
- CounterMappingContext &Ctx;
- FunctionRecord &Function;
- bool IsVersion11;
+ enum Result {
+ NotProcessed = 0, /// Irrelevant to this Decision
+ Processed, /// Added to this Decision
+ Completed, /// Added and filled this Decision
+ };
- /// Evaluated Counters.
- std::map<Counter, uint64_t> CounterValues;
+ /// Add Branch into the Decision
+ /// \param Branch expects MCDCBranchRegion
+ /// \returns NotProcessed/Processed/Completed
+ Result addBranch(const CounterMappingRegion &Branch) {
+ assert(Branch.Kind == CounterMappingRegion::MCDCBranchRegion);
- /// Decisions are nestable.
- SmallVector<DecisionRecord, 1> DecisionStack;
+ auto ConditionID = Branch.getBranchParams().ID;
- /// A File pointed by Expansion
- struct FileInfo {
- /// The last index(+1) for each FileID in MappingRegions.
- unsigned LastIndex = 0;
- /// Mark Files pointed by Expansions.
- /// Non-marked Files are root Files.
- bool IsExpanded = false;
- };
+ if (ConditionIDs.contains(ConditionID) ||
+ ConditionID >= DecisionParams.NumConditions)
+ return NotProcessed;
- /// The last element is a sentinel with Index=NumRegions.
- std::vector<FileInfo> Files;
-#ifndef NDEBUG
- DenseSet<unsigned> Visited;
-#endif
+ if (!this->dominates(Branch))
+ return NotProcessed;
- CountedRegionEmitter(const CoverageMappingRecord &Record,
- CounterMappingContext &Ctx, FunctionRecord &Function,
- bool IsVersion11)
- : Record(Record), Ctx(Ctx), Function(Function), IsVersion11(IsVersion11),
- Files(Record.Filenames.size()) {
- // Scan MappingRegions and mark each last index by FileID.
- for (auto [I, Region] : enumerate(Record.MappingRegions)) {
- if (Region.FileID >= Files.size()) {
- // Extend (only possible in CoverageMappingTests)
- Files.resize(Region.FileID + 1);
- }
- Files[Region.FileID].LastIndex = I + 1;
- if (Region.Kind == CounterMappingRegion::ExpansionRegion) {
- if (Region.ExpandedFileID >= Files.size()) {
- // Extend (only possible in CoverageMappingTests)
- Files.resize(Region.ExpandedFileID + 1);
- }
- Files[Region.ExpandedFileID].IsExpanded = true;
- }
- }
- }
+ assert(MCDCBranches.size() < DecisionParams.NumConditions);
- /// Evaluate C and store its evaluated Value into CounterValues.
- Error evaluateAndCacheCounter(Counter C) {
- if (CounterValues.count(C) > 0)
- return Error::success();
+ // Put `ID=0` in front of `MCDCBranches` for convenience
+ // even if `MCDCBranches` is not topological.
+ if (ConditionID == 0)
+ MCDCBranches.insert(MCDCBranches.begin(), &Branch);
+ else
+ MCDCBranches.push_back(&Branch);
- auto ValueOrErr = Ctx.evaluate(C);
- if (!ValueOrErr)
- return ValueOrErr.takeError();
- CounterValues[C] = *ValueOrErr;
- return Error::success();
- }
+ // Mark `ID` as `assigned`.
+ ConditionIDs.insert(ConditionID);
- Error walk(unsigned Idx) {
- assert(Idx < Files.size());
- unsigned B = (Idx == 0 ? 0 : Files[Idx - 1].LastIndex);
- unsigned E = Files[Idx].LastIndex;
- assert(B != E && "Empty FileID");
- assert(Visited.insert(Idx).second && "Duplicate Expansions");
- for (unsigned I = B; I != E; ++I) {
- const auto &Region = Record.MappingRegions[I];
- if (Region.FileID != Idx)
- break;
+ // `Completed` when `MCDCBranches` is full
+ return (MCDCBranches.size() == DecisionParams.NumConditions ? Completed
+ : Processed);
+ }
- if (Region.Kind == CounterMappingRegion::ExpansionRegion)
- if (auto E = walk(Region.ExpandedFileID))
- return E;
+ /// Record Expansion if it is relevant to this Decision.
+ /// Each `Expansion` may nest.
+ /// \returns true if recorded.
+ bool recordExpansion(const CounterMappingRegion &Expansion) {
+ if (!this->dominates(Expansion))
+ return false;
- if (auto E = evaluateAndCacheCounter(Region.Count))
- return E;
+ ExpandedFileIDs.insert(Expansion.ExpandedFileID);
+ return true;
+ }
+ };
- if (Region.Kind == CounterMappingRegion::MCDCDecisionRegion) {
- // Start the new Decision on the stack.
- DecisionStack.emplace_back(Region);
- } else if (Region.Kind == CounterMappingRegion::MCDCBranchRegion) {
- assert(!DecisionStack.empty() && "Orphan MCDCBranch");
- auto &D = DecisionStack.back();
-
- if (D.pushBranch(Region)) {
- // All Branches have been found in the Decision.
- auto RecordOrErr = Ctx.evaluateMCDCRegion(
- *D.DecisionRegion, D.MCDCBranches, IsVersion11);
- if (!RecordOrErr)
- return RecordOrErr.takeError();
-
- // Finish the stack.
- Function.pushMCDCRecord(std::move(*RecordOrErr));
- DecisionStack.pop_back();
- }
- }
+private:
+ /// Decisions in progress
+ /// DecisionRecord is added for each MCDCDecisionRegion.
+ /// DecisionRecord is removed when Decision is completed.
+ SmallVector<DecisionRecord> Decisions;
- // Evaluate FalseCount
- // It may have the Counter in Branches, or Zero.
- if (auto E = evaluateAndCacheCounter(Region.FalseCount))
- return E;
- }
+public:
+ ~MCDCDecisionRecorder() {
+ assert(Decisions.empty() && "All Decisions have not been resolved");
+ }
- assert((Idx != 0 || DecisionStack.empty()) && "Decision wasn't closed");
+ /// Register Region and start recording.
+ void registerDecision(const CounterMappingRegion &Decision) {
+ Decisions.emplace_back(Decision);
+ }
- return Error::success();
+ void recordExpansion(const CounterMappingRegion &Expansion) {
+ any_of(Decisions, [&Expansion](auto &Decision) {
+ return Decision.recordExpansion(Expansion);
+ });
}
- Error emitCountedRegions() {
- // Walk MappingRegions along Expansions.
- // - Evaluate Counters
- // - Emit MCDCRecords
- for (auto [I, F] : enumerate(Files)) {
- if (!F.IsExpanded)
- if (auto E = walk(I))
- return E;
- }
- assert(Visited.size() == Files.size() && "Dangling FileID");
-
- // Emit CountedRegions in the same order as MappingRegions.
- for (const auto &Region : Record.MappingRegions) {
- if (Region.Kind == CounterMappingRegion::MCDCDecisionRegion)
- continue; // Don't emit.
- // Adopt values from the CounterValues.
- // FalseCount may be Zero unless Branches.
- Function.pushRegion(Region, CounterValues[Region.Count],
- CounterValues[Region.FalseCount]);
- }
+ using DecisionAndBranches =
+ std::pair<const CounterMappingRegion *, /// Decision
+ SmallVector<const CounterMappingRegion *> /// Branches
+ >;
+
+ /// Add MCDCBranchRegion to DecisionRecord.
+ /// \param Branch to be processed
+ /// \returns DecisionsAndBranches if DecisionRecord completed.
+ /// Or returns nullopt.
+ std::optional<DecisionAndBranches>
+ processBranch(const CounterMappingRegion &Branch) {
+ // Seek each Decision and apply Region to it.
+ for (auto DecisionIter = Decisions.begin(), DecisionEnd = Decisions.end();
+ DecisionIter != DecisionEnd; ++DecisionIter)
+ switch (DecisionIter->addBranch(Branch)) {
+ case DecisionRecord::NotProcessed:
+ continue;
+ case DecisionRecord::Processed:
+ return std::nullopt;
+ case DecisionRecord::Completed:
+ DecisionAndBranches Result =
+ std::make_pair(DecisionIter->DecisionRegion,
+ std::move(DecisionIter->MCDCBranches));
+ Decisions.erase(DecisionIter); // No longer used.
+ return Result;
+ }
- return Error::success();
+ llvm_unreachable("Branch not found in Decisions");
}
};
@@ -919,12 +907,57 @@ Error CoverageMapping::loadFunctionRecord(
Record.MappingRegions[0].Count.isZero() && Counts[0] > 0)
return Error::success();
+ MCDCDecisionRecorder MCDCDecisions;
FunctionRecord Function(OrigFuncName, Record.Filenames);
+ for (const auto &Region : Record.MappingRegions) {
+ // MCDCDecisionRegion should be handled first since it overlaps with
+ // others inside.
+ if (Region.Kind == CounterMappingRegion::MCDCDecisionRegion) {
+ MCDCDecisions.registerDecision(Region);
+ continue;
+ }
+ Expected<int64_t> ExecutionCount = Ctx.evaluate(Region.Count);
+ if (auto E = ExecutionCount.takeError()) {
+ consumeError(std::move(E));
+ return Error::success();
+ }
+ Expected<int64_t> AltExecutionCount = Ctx.evaluate(Region.FalseCount);
+ if (auto E = AltExecutionCount.takeError()) {
+ consumeError(std::move(E));
+ return Error::success();
+ }
+ Function.pushRegion(Region, *ExecutionCount, *AltExecutionCount);
- // Emit CountedRegions into FunctionRecord.
- if (auto E = CountedRegionEmitter(Record, Ctx, Function, IsVersion11)
- .emitCountedRegions())
- return E;
+ // Record ExpansionRegion.
+ if (Region.Kind == CounterMappingRegion::ExpansionRegion) {
+ MCDCDecisions.recordExpansion(Region);
+ continue;
+ }
+
+ // Do nothing unless MCDCBranchRegion.
+ if (Region.Kind != CounterMappingRegion::MCDCBranchRegion)
+ continue;
+
+ auto Result = MCDCDecisions.processBranch(Region);
+ if (!Result) // Any Decision doesn't complete.
+ continue;
+
+ auto MCDCDecision = Result->first;
+ auto &MCDCBranches = Result->second;
+
+ // Since the bitmap identifies the executed test vectors for an MC/DC
+ // DecisionRegion, all of the information is now available to process.
+ // This is where the bulk of the MC/DC progressing takes place.
+ Expected<MCDCRecord> Record =
+ Ctx.evaluateMCDCRegion(*MCDCDecision, MCDCBranches, IsVersion11);
+ if (auto E = Record.takeError()) {
+ consumeError(std::move(E));
+ return Error::success();
+ }
+
+ // Save the MC/DC Record so that it can be visualized later.
+ Function.pushMCDCRecord(std::move(*Record));
+ }
// Don't create records for (filenames, function) pairs we've already seen.
auto FilenamesHash = hash_combine_range(Record.Filenames);
diff --git a/llvm/test/tools/llvm-cov/mcdc-macro.test b/llvm/test/tools/llvm-cov/mcdc-macro.test
index d68f4aeb67bcb..14dd5ebd68eb1 100644
--- a/llvm/test/tools/llvm-cov/mcdc-macro.test
+++ b/llvm/test/tools/llvm-cov/mcdc-macro.test
@@ -13,19 +13,19 @@
// CHECK-NEXT: |
// CHECK-NEXT: | Number of Conditions: 5
// CHECK-NEXT: | Condition C1 --> (9:7)
-// CHECK-NEXT: | Condition C2 --> (2:11)
-// CHECK-NEXT: | Condition C3 --> (3:11)
-// CHECK-NEXT: | Condition C4 --> (3:23)
-// CHECK-NEXT: | Condition C5 --> (9:22)
+// CHECK-NEXT: | Condition C2 --> (9:22)
+// CHECK-NEXT: | Condition C3 --> (2:11)
+// CHECK-NEXT: | Condition C4 --> (3:11)
+// CHECK-NEXT: | Condition C5 --> (3:23)
// CHECK-NEXT: |
// CHECK-NEXT: | Executed MC/DC Test Vectors:
// CHECK-NEXT: |
// CHECK-NEXT: | C1, C2, C3, C4, C5 Result
-// CHECK-NEXT: | 1 { T, C, T, T, - = T }
+// CHECK-NEXT: | 1 { T, -, C, T, T = T }
// CHECK-NEXT: |
// CHECK-NEXT: | C1-Pair: not covered
-// CHECK-NEXT: | C2-Pair: constant folded
-// CHECK-NEXT: | C3-Pair: not covered
+// CHECK-NEXT: | C2-Pair: not covered
+// CHECK-NEXT: | C3-Pair: constant folded
// CHECK-NEXT: | C4-Pair: not covered
// CHECK-NEXT: | C5-Pair: not covered
// CHECK-NEXT: | MC/DC Coverage for Decision: 0.00%
|
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/31683 Here is the relevant piece of the build log for the reference |
Contributor
|
@gulfemsavrun |
Contributor
Author
PR #180277 is not relevant; the issue we are seeing is a specific regression introduced by PR #125407. In Fuchsia, we perform end-to-end coverage collection across a large codebase (C++ and Rust). This makes us sensitive to edge cases in coverage mapping logic. The failure is triggered by this line in CoverageMapping.cpp. I think we should only evaluate |
chapuni
reviewed
Feb 13, 2026
Contributor
chapuni
left a comment
chapuni
left a comment
There was a problem hiding this comment.
@gulfemsavrun
Let me confirm. Does llvm-cov process non-clang-generated covmaps as well? It'd be helpful if you could provide the actual covmap that triggers the issue.
I haven't encountered this issue while testing llvm-cov with the LLVM tree.
Comment on lines
+924
to
+929
| Expected<int64_t> AltExecutionCount = Ctx.evaluate(Region.FalseCount); | ||
| if (auto E = AltExecutionCount.takeError()) { | ||
| consumeError(std::move(E)); | ||
| return Error::success(); | ||
| } | ||
| Function.pushRegion(Region, *ExecutionCount, *AltExecutionCount); |
Contributor
There was a problem hiding this comment.
This is the corresponding impl. This discards the error and terminates the evaluation of Function coverage at that point.
Is this the desired behavior? I can consider relaxing error handling in evaluate() as a workaround if preferred.
Contributor
Author
The reproducer files have been uploaded to: Please use the following command to reproduce the failure: Just a heads-up that I'll be OOO all next week. |
kevinwkt
pushed a commit
to kevinwkt/llvm-project
that referenced
this pull request
Feb 16, 2026
…" (llvm#181069) This reverts commit 8f690ec because it caused errors in collecting coverage.
Contributor
|
@gulfemsavrun I cannot download two files in the URL (500). Seems small files could be downloaded. I can wait, for a few days, for the content server would get back better but I'd be happy if you (or anyone else) would send me files via other paths. This revert has broken functionality of "MC/DC nested expressions". |
Contributor
Author
I have attached the file in split chunks, as the individual file size exceeds the 25MB upload limit. Run the following command using the provided files: The command fails with the following error: merged.profdata.tar.gz.part_aa.gz |
Contributor
|
@gulfemsavrun Thanks for files to be provided. I can see what happens but I could not investigate what would actually be happening. Just guess, "profile correlation". I have created #183073. It loosens errors. |
Contributor
|
I have restored in #183073. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This reverts commit 8f690ec because it caused errors in collecting coverage.