From c2b49a5317bf5b8af419cba814f95cc9305bec21 Mon Sep 17 00:00:00 2001 From: NAKAMURA Takumi Date: Thu, 8 Feb 2024 23:12:54 +0900 Subject: [PATCH 1/3] [MC/DC] Refactor: Make `MCDCParams` as `std::variant` Introduce `MCDCDecisionParameters` and `MCDCBranchParameters` and make sure them not initialized as zero. FIXME: Could we make `CoverageMappingRegion` as a smart tagged union? --- clang/lib/CodeGen/CoverageMappingGen.cpp | 69 +++++++++-------- .../ProfileData/Coverage/CoverageMapping.h | 77 ++++++++++++------- .../ProfileData/Coverage/CoverageMapping.cpp | 53 +++++++------ .../Coverage/CoverageMappingReader.cpp | 17 ++-- .../Coverage/CoverageMappingWriter.cpp | 23 ++++-- .../ProfileData/CoverageMappingTest.cpp | 8 +- 6 files changed, 149 insertions(+), 98 deletions(-) diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp index 0c43317642bca4..da5d43cda91209 100644 --- a/clang/lib/CodeGen/CoverageMappingGen.cpp +++ b/clang/lib/CodeGen/CoverageMappingGen.cpp @@ -97,6 +97,8 @@ void CoverageSourceInfo::updateNextTokLoc(SourceLocation Loc) { namespace { using MCDCConditionID = CounterMappingRegion::MCDCConditionID; using MCDCParameters = CounterMappingRegion::MCDCParameters; +using MCDCBranchParameters = CounterMappingRegion::MCDCBranchParameters; +using MCDCDecisionParameters = CounterMappingRegion::MCDCDecisionParameters; /// A region of source code that can be mapped to a counter. class SourceMappingRegion { @@ -185,7 +187,17 @@ class SourceMappingRegion { bool isBranch() const { return FalseCount.has_value(); } - bool isMCDCDecision() const { return MCDCParams.NumConditions != 0; } + bool isMCDCDecision() const { + const auto *DecisionParams = + std::get_if(&MCDCParams); + assert(!DecisionParams || DecisionParams->NumConditions > 0); + return DecisionParams; + } + + const auto &getMCDCDecisionParams() const { + return CounterMappingRegion::getParams( + MCDCParams); + } const MCDCParameters &getMCDCParams() const { return MCDCParams; } }; @@ -483,13 +495,13 @@ class CoverageMappingBuilder { SR.ColumnEnd)); } else if (Region.isBranch()) { MappingRegions.push_back(CounterMappingRegion::makeBranchRegion( - Region.getCounter(), Region.getFalseCounter(), - Region.getMCDCParams(), *CovFileID, SR.LineStart, SR.ColumnStart, - SR.LineEnd, SR.ColumnEnd)); + Region.getCounter(), Region.getFalseCounter(), *CovFileID, + SR.LineStart, SR.ColumnStart, SR.LineEnd, SR.ColumnEnd, + Region.getMCDCParams())); } else if (Region.isMCDCDecision()) { MappingRegions.push_back(CounterMappingRegion::makeDecisionRegion( - Region.getMCDCParams(), *CovFileID, SR.LineStart, SR.ColumnStart, - SR.LineEnd, SR.ColumnEnd)); + Region.getMCDCDecisionParams(), *CovFileID, SR.LineStart, + SR.ColumnStart, SR.LineEnd, SR.ColumnEnd)); } else { MappingRegions.push_back(CounterMappingRegion::makeRegion( Region.getCounter(), *CovFileID, SR.LineStart, SR.ColumnStart, @@ -865,8 +877,7 @@ struct CounterCoverageMappingBuilder std::optional StartLoc = std::nullopt, std::optional EndLoc = std::nullopt, std::optional FalseCount = std::nullopt, - MCDCConditionID ID = 0, MCDCConditionID TrueID = 0, - MCDCConditionID FalseID = 0) { + const MCDCParameters &BranchParams = std::monostate()) { if (StartLoc && !FalseCount) { MostRecentLocation = *StartLoc; @@ -885,9 +896,7 @@ struct CounterCoverageMappingBuilder StartLoc = std::nullopt; if (EndLoc && EndLoc->isInvalid()) EndLoc = std::nullopt; - RegionStack.emplace_back(Count, FalseCount, - MCDCParameters{0, 0, ID, TrueID, FalseID}, - StartLoc, EndLoc); + RegionStack.emplace_back(Count, FalseCount, BranchParams, StartLoc, EndLoc); return RegionStack.size() - 1; } @@ -896,8 +905,8 @@ struct CounterCoverageMappingBuilder std::optional StartLoc = std::nullopt, std::optional EndLoc = std::nullopt) { - RegionStack.emplace_back(MCDCParameters{BitmapIdx, Conditions}, StartLoc, - EndLoc); + RegionStack.emplace_back(MCDCDecisionParameters{BitmapIdx, Conditions}, + StartLoc, EndLoc); return RegionStack.size() - 1; } @@ -1042,9 +1051,10 @@ struct CounterCoverageMappingBuilder // function's SourceRegions) because it doesn't apply to any other source // code other than the Condition. if (CodeGenFunction::isInstrumentedCondition(C)) { + MCDCParameters BranchParams; MCDCConditionID ID = MCDCBuilder.getCondID(C); - MCDCConditionID TrueID = IDPair.TrueID; - MCDCConditionID FalseID = IDPair.FalseID; + if (ID > 0) + BranchParams = MCDCBranchParameters{ID, IDPair.TrueID, IDPair.FalseID}; // If a condition can fold to true or false, the corresponding branch // will be removed. Create a region with both counters hard-coded to @@ -1054,11 +1064,11 @@ struct CounterCoverageMappingBuilder // CodeGenFunction.c always returns false, but that is very heavy-handed. if (ConditionFoldsToBool(C)) popRegions(pushRegion(Counter::getZero(), getStart(C), getEnd(C), - Counter::getZero(), ID, TrueID, FalseID)); + Counter::getZero(), BranchParams)); else // Otherwise, create a region with the True counter and False counter. - popRegions(pushRegion(TrueCnt, getStart(C), getEnd(C), FalseCnt, ID, - TrueID, FalseID)); + popRegions(pushRegion(TrueCnt, getStart(C), getEnd(C), FalseCnt, + BranchParams)); } } @@ -1149,12 +1159,9 @@ struct CounterCoverageMappingBuilder // we've seen this region. if (StartLocs.insert(Loc).second) { if (I.isBranch()) - SourceRegions.emplace_back( - I.getCounter(), I.getFalseCounter(), - MCDCParameters{0, 0, I.getMCDCParams().ID, - I.getMCDCParams().TrueID, - I.getMCDCParams().FalseID}, - Loc, getEndOfFileOrMacro(Loc), I.isBranch()); + SourceRegions.emplace_back(I.getCounter(), I.getFalseCounter(), + I.getMCDCParams(), Loc, + getEndOfFileOrMacro(Loc), I.isBranch()); else SourceRegions.emplace_back(I.getCounter(), Loc, getEndOfFileOrMacro(Loc)); @@ -2120,9 +2127,10 @@ static void dump(llvm::raw_ostream &OS, StringRef FunctionName, OS << "File " << R.FileID << ", " << R.LineStart << ":" << R.ColumnStart << " -> " << R.LineEnd << ":" << R.ColumnEnd << " = "; - if (R.Kind == CounterMappingRegion::MCDCDecisionRegion) { - OS << "M:" << R.MCDCParams.BitmapIdx; - OS << ", C:" << R.MCDCParams.NumConditions; + if (const auto *DecisionParams = + std::get_if(&R.MCDCParams)) { + OS << "M:" << DecisionParams->BitmapIdx; + OS << ", C:" << DecisionParams->NumConditions; } else { Ctx.dump(R.Count, OS); @@ -2133,9 +2141,10 @@ static void dump(llvm::raw_ostream &OS, StringRef FunctionName, } } - if (R.Kind == CounterMappingRegion::MCDCBranchRegion) { - OS << " [" << R.MCDCParams.ID << "," << R.MCDCParams.TrueID; - OS << "," << R.MCDCParams.FalseID << "] "; + if (const auto *BranchParams = + std::get_if(&R.MCDCParams)) { + OS << " [" << BranchParams->ID << "," << BranchParams->TrueID; + OS << "," << BranchParams->FalseID << "] "; } if (R.Kind == CounterMappingRegion::ExpansionRegion) diff --git a/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h b/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h index 88ec60c7aa33c6..0ad6d48a0eb798 100644 --- a/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h +++ b/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h @@ -39,6 +39,7 @@ #include #include #include +#include #include namespace llvm { @@ -250,18 +251,27 @@ struct CounterMappingRegion { }; using MCDCConditionID = unsigned int; - struct MCDCParameters { + struct MCDCDecisionParameters { /// Byte Index of Bitmap Coverage Object for a Decision Region. - unsigned BitmapIdx = 0; + unsigned BitmapIdx; /// Number of Conditions used for a Decision Region. - unsigned NumConditions = 0; + unsigned NumConditions; + MCDCDecisionParameters() = delete; + }; + + struct MCDCBranchParameters { /// IDs used to represent a branch region and other branch regions /// evaluated based on True and False branches. - MCDCConditionID ID = 0, TrueID = 0, FalseID = 0; + MCDCConditionID ID, TrueID, FalseID; + + MCDCBranchParameters() = delete; }; + using MCDCParameters = std::variant; + /// Primary Counter that is also used for Branch Regions (TrueCount). Counter Count; @@ -271,6 +281,24 @@ struct CounterMappingRegion { /// Parameters used for Modified Condition/Decision Coverage MCDCParameters MCDCParams; + template + static auto &getParams(MaybeConstMCDCParameters &MCDCParams) { + using InnerParameters = + typename std::remove_const::type; + MaybeConstInnerParameters *Params = + std::get_if(&MCDCParams); + assert(Params && "InnerParameters unavailable"); + return *Params; + } + + const auto &getDecisionParams() const { + return getParams(MCDCParams); + } + + const auto &getBranchParams() const { + return getParams(MCDCParams); + } + unsigned FileID = 0; unsigned ExpandedFileID = 0; unsigned LineStart, ColumnStart, LineEnd, ColumnEnd; @@ -284,19 +312,20 @@ struct CounterMappingRegion { LineStart(LineStart), ColumnStart(ColumnStart), LineEnd(LineEnd), ColumnEnd(ColumnEnd), Kind(Kind) {} - CounterMappingRegion(Counter Count, Counter FalseCount, - MCDCParameters MCDCParams, unsigned FileID, + CounterMappingRegion(Counter Count, Counter FalseCount, unsigned FileID, unsigned ExpandedFileID, unsigned LineStart, unsigned ColumnStart, unsigned LineEnd, - unsigned ColumnEnd, RegionKind Kind) + unsigned ColumnEnd, RegionKind Kind, + const MCDCParameters &MCDCParams = std::monostate()) : Count(Count), FalseCount(FalseCount), MCDCParams(MCDCParams), FileID(FileID), ExpandedFileID(ExpandedFileID), LineStart(LineStart), ColumnStart(ColumnStart), LineEnd(LineEnd), ColumnEnd(ColumnEnd), Kind(Kind) {} - CounterMappingRegion(MCDCParameters MCDCParams, unsigned FileID, - unsigned LineStart, unsigned ColumnStart, - unsigned LineEnd, unsigned ColumnEnd, RegionKind Kind) + CounterMappingRegion(const MCDCDecisionParameters &MCDCParams, + unsigned FileID, unsigned LineStart, + unsigned ColumnStart, unsigned LineEnd, + unsigned ColumnEnd, RegionKind Kind) : MCDCParams(MCDCParams), FileID(FileID), LineStart(LineStart), ColumnStart(ColumnStart), LineEnd(LineEnd), ColumnEnd(ColumnEnd), Kind(Kind) {} @@ -333,24 +362,18 @@ struct CounterMappingRegion { static CounterMappingRegion makeBranchRegion(Counter Count, Counter FalseCount, unsigned FileID, unsigned LineStart, unsigned ColumnStart, unsigned LineEnd, - unsigned ColumnEnd) { - return CounterMappingRegion(Count, FalseCount, MCDCParameters(), FileID, 0, - LineStart, ColumnStart, LineEnd, ColumnEnd, - BranchRegion); - } - - static CounterMappingRegion - makeBranchRegion(Counter Count, Counter FalseCount, MCDCParameters MCDCParams, - unsigned FileID, unsigned LineStart, unsigned ColumnStart, - unsigned LineEnd, unsigned ColumnEnd) { - return CounterMappingRegion(Count, FalseCount, MCDCParams, FileID, 0, - LineStart, ColumnStart, LineEnd, ColumnEnd, - MCDCParams.ID == 0 ? BranchRegion - : MCDCBranchRegion); + unsigned ColumnEnd, + const MCDCParameters &MCDCParams = std::monostate()) { + return CounterMappingRegion(Count, FalseCount, FileID, 0, LineStart, + ColumnStart, LineEnd, ColumnEnd, + (std::get_if(&MCDCParams) + ? MCDCBranchRegion + : BranchRegion), + MCDCParams); } static CounterMappingRegion - makeDecisionRegion(MCDCParameters MCDCParams, unsigned FileID, + makeDecisionRegion(const MCDCDecisionParameters &MCDCParams, unsigned FileID, unsigned LineStart, unsigned ColumnStart, unsigned LineEnd, unsigned ColumnEnd) { return CounterMappingRegion(MCDCParams, FileID, LineStart, ColumnStart, @@ -415,9 +438,7 @@ struct MCDCRecord { CounterMappingRegion getDecisionRegion() const { return Region; } unsigned getNumConditions() const { - assert(Region.MCDCParams.NumConditions != 0 && - "In MC/DC, NumConditions should never be zero!"); - return Region.MCDCParams.NumConditions; + return Region.getDecisionParams().NumConditions; } unsigned getNumTestVectors() const { return TV.size(); } bool isCondFolded(unsigned Condition) const { return Folded[Condition]; } diff --git a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp index 6b189c31463283..646b9fb077ef07 100644 --- a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp +++ b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp @@ -234,6 +234,7 @@ class MCDCRecordProcessor { /// Decision Region to which the ExecutedTestVectorBitmap applies. const CounterMappingRegion &Region; + const CounterMappingRegion::MCDCDecisionParameters &DecisionParams; /// Array of branch regions corresponding each conditions in the boolean /// expression. @@ -244,8 +245,9 @@ class MCDCRecordProcessor { unsigned BitmapIdx; - /// Mapping of a condition ID to its corresponding branch region. - llvm::DenseMap Map; + /// Mapping of a condition ID to its corresponding branch params. + llvm::DenseMap + BranchParamsMap; /// Vector used to track whether a condition is constant folded. MCDCRecord::BoolVector Folded; @@ -264,9 +266,10 @@ class MCDCRecordProcessor { MCDCRecordProcessor(const BitVector &Bitmap, const CounterMappingRegion &Region, ArrayRef Branches) - : Bitmap(Bitmap), Region(Region), Branches(Branches), - NumConditions(Region.MCDCParams.NumConditions), - BitmapIdx(Region.MCDCParams.BitmapIdx * CHAR_BIT), + : Bitmap(Bitmap), Region(Region), + DecisionParams(Region.getDecisionParams()), Branches(Branches), + NumConditions(DecisionParams.NumConditions), + BitmapIdx(DecisionParams.BitmapIdx * CHAR_BIT), Folded(NumConditions, false), IndependencePairs(NumConditions), TestVectors((size_t)1 << NumConditions) {} @@ -286,18 +289,18 @@ class MCDCRecordProcessor { // the truth table. void buildTestVector(MCDCRecord::TestVector &TV, unsigned ID, unsigned Index) { - const CounterMappingRegion *Branch = Map[ID]; + auto [UnusedID, TrueID, FalseID] = *BranchParamsMap[ID]; TV[ID - 1] = MCDCRecord::MCDC_False; - if (Branch->MCDCParams.FalseID > 0) - buildTestVector(TV, Branch->MCDCParams.FalseID, Index); + if (FalseID > 0) + buildTestVector(TV, FalseID, Index); else recordTestVector(TV, Index, MCDCRecord::MCDC_False); Index |= 1 << (ID - 1); TV[ID - 1] = MCDCRecord::MCDC_True; - if (Branch->MCDCParams.TrueID > 0) - buildTestVector(TV, Branch->MCDCParams.TrueID, Index); + if (TrueID > 0) + buildTestVector(TV, TrueID, Index); else recordTestVector(TV, Index, MCDCRecord::MCDC_True); @@ -374,8 +377,9 @@ class MCDCRecordProcessor { // - Record whether the condition is constant folded so that we exclude it // from being measured. for (const auto *B : Branches) { - Map[B->MCDCParams.ID] = B; - PosToID[I] = B->MCDCParams.ID - 1; + const auto &BranchParams = B->getBranchParams(); + BranchParamsMap[BranchParams.ID] = &BranchParams; + PosToID[I] = BranchParams.ID - 1; CondLoc[I] = B->startLoc(); Folded[I++] = (B->Count.isZero() && B->FalseCount.isZero()); } @@ -501,10 +505,12 @@ static unsigned getMaxBitmapSize(const CounterMappingContext &Ctx, // Note that `<=` is used insted of `<`, because `BitmapIdx == 0` is valid // and `MaxBitmapIdx is `unsigned`. `BitmapIdx` is unique in the record. for (const auto &Region : reverse(Record.MappingRegions)) { - if (Region.Kind == CounterMappingRegion::MCDCDecisionRegion && - MaxBitmapIdx <= Region.MCDCParams.BitmapIdx) { - MaxBitmapIdx = Region.MCDCParams.BitmapIdx; - NumConditions = Region.MCDCParams.NumConditions; + if (Region.Kind != CounterMappingRegion::MCDCDecisionRegion) + continue; + const auto &DecisionParams = Region.getDecisionParams(); + if (MaxBitmapIdx <= DecisionParams.BitmapIdx) { + MaxBitmapIdx = DecisionParams.BitmapIdx; + NumConditions = DecisionParams.NumConditions; } } unsigned SizeInBits = llvm::alignTo(uint64_t(1) << NumConditions, CHAR_BIT); @@ -526,6 +532,7 @@ class MCDCDecisionRecorder { /// They are reflected from DecisionRegion for convenience. LineColPair DecisionStartLoc; LineColPair DecisionEndLoc; + CounterMappingRegion::MCDCDecisionParameters DecisionParams; /// This is passed to `MCDCRecordProcessor`, so this should be compatible /// to`ArrayRef`. @@ -543,7 +550,8 @@ class MCDCDecisionRecorder { DecisionRecord(const CounterMappingRegion &Decision) : DecisionRegion(&Decision), DecisionStartLoc(Decision.startLoc()), - DecisionEndLoc(Decision.endLoc()) { + DecisionEndLoc(Decision.endLoc()), + DecisionParams(Decision.getDecisionParams()) { assert(Decision.Kind == CounterMappingRegion::MCDCDecisionRegion); } @@ -570,17 +578,17 @@ class MCDCDecisionRecorder { Result addBranch(const CounterMappingRegion &Branch) { assert(Branch.Kind == CounterMappingRegion::MCDCBranchRegion); - auto ConditionID = Branch.MCDCParams.ID; + auto ConditionID = Branch.getBranchParams().ID; assert(ConditionID > 0 && "ConditionID should begin with 1"); if (ConditionIDs.contains(ConditionID) || - ConditionID > DecisionRegion->MCDCParams.NumConditions) + ConditionID > DecisionParams.NumConditions) return NotProcessed; if (!this->dominates(Branch)) return NotProcessed; - assert(MCDCBranches.size() < DecisionRegion->MCDCParams.NumConditions); + assert(MCDCBranches.size() < DecisionParams.NumConditions); // Put `ID=1` in front of `MCDCBranches` for convenience // even if `MCDCBranches` is not topological. @@ -593,9 +601,8 @@ class MCDCDecisionRecorder { ConditionIDs.insert(ConditionID); // `Completed` when `MCDCBranches` is full - return (MCDCBranches.size() == DecisionRegion->MCDCParams.NumConditions - ? Completed - : Processed); + return (MCDCBranches.size() == DecisionParams.NumConditions ? Completed + : Processed); } /// Record Expansion if it is relevant to this Decision. diff --git a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp index ac8e6b56379f21..bd2ca2afe31d6a 100644 --- a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp +++ b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp @@ -244,7 +244,8 @@ Error RawCoverageMappingReader::readMappingRegionsSubArray( unsigned LineStart = 0; for (size_t I = 0; I < NumRegions; ++I) { Counter C, C2; - uint64_t BIDX = 0, NC = 0, ID = 0, TID = 0, FID = 0; + uint64_t BIDX, NC, ID, TID, FID; + CounterMappingRegion::MCDCParameters Params; CounterMappingRegion::RegionKind Kind = CounterMappingRegion::CodeRegion; // Read the combined counter + region kind. @@ -308,6 +309,9 @@ Error RawCoverageMappingReader::readMappingRegionsSubArray( return Err; if (auto Err = readIntMax(FID, std::numeric_limits::max())) return Err; + assert(ID > 0); + Params = CounterMappingRegion::MCDCBranchParameters{ + (unsigned)ID, (unsigned)TID, (unsigned)FID}; break; case CounterMappingRegion::MCDCDecisionRegion: Kind = CounterMappingRegion::MCDCDecisionRegion; @@ -315,6 +319,8 @@ Error RawCoverageMappingReader::readMappingRegionsSubArray( return Err; if (auto Err = readIntMax(NC, std::numeric_limits::max())) return Err; + Params = CounterMappingRegion::MCDCDecisionParameters{(unsigned)BIDX, + (unsigned)NC}; break; default: return make_error(coveragemap_error::malformed, @@ -370,13 +376,8 @@ Error RawCoverageMappingReader::readMappingRegionsSubArray( }); auto CMR = CounterMappingRegion( - C, C2, - CounterMappingRegion::MCDCParameters{ - static_cast(BIDX), static_cast(NC), - static_cast(ID), static_cast(TID), - static_cast(FID)}, - InferredFileID, ExpandedFileID, LineStart, ColumnStart, - LineStart + NumLines, ColumnEnd, Kind); + C, C2, InferredFileID, ExpandedFileID, LineStart, ColumnStart, + LineStart + NumLines, ColumnEnd, Kind, Params); if (CMR.startLoc() > CMR.endLoc()) return make_error( coveragemap_error::malformed, diff --git a/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp b/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp index 27727f216b0513..8706f8e677bce6 100644 --- a/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp +++ b/llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp @@ -213,6 +213,7 @@ void CoverageMappingWriter::write(raw_ostream &OS) { } Counter Count = Minimizer.adjust(I->Count); Counter FalseCount = Minimizer.adjust(I->FalseCount); + bool ParamsShouldBeNull = true; switch (I->Kind) { case CounterMappingRegion::CodeRegion: case CounterMappingRegion::GapRegion: @@ -251,16 +252,25 @@ void CoverageMappingWriter::write(raw_ostream &OS) { OS); writeCounter(MinExpressions, Count, OS); writeCounter(MinExpressions, FalseCount, OS); - encodeULEB128(unsigned(I->MCDCParams.ID), OS); - encodeULEB128(unsigned(I->MCDCParams.TrueID), OS); - encodeULEB128(unsigned(I->MCDCParams.FalseID), OS); + { + const auto &BranchParams = I->getBranchParams(); + ParamsShouldBeNull = false; + assert(BranchParams.ID > 0); + encodeULEB128(unsigned(BranchParams.ID), OS); + encodeULEB128(unsigned(BranchParams.TrueID), OS); + encodeULEB128(unsigned(BranchParams.FalseID), OS); + } break; case CounterMappingRegion::MCDCDecisionRegion: encodeULEB128(unsigned(I->Kind) << Counter::EncodingCounterTagAndExpansionRegionTagBits, OS); - encodeULEB128(unsigned(I->MCDCParams.BitmapIdx), OS); - encodeULEB128(unsigned(I->MCDCParams.NumConditions), OS); + { + const auto &DecisionParams = I->getDecisionParams(); + ParamsShouldBeNull = false; + encodeULEB128(unsigned(DecisionParams.BitmapIdx), OS); + encodeULEB128(unsigned(DecisionParams.NumConditions), OS); + } break; } assert(I->LineStart >= PrevLineStart); @@ -270,6 +280,9 @@ void CoverageMappingWriter::write(raw_ostream &OS) { encodeULEB128(I->LineEnd - I->LineStart, OS); encodeULEB128(I->ColumnEnd, OS); PrevLineStart = I->LineStart; + assert((!ParamsShouldBeNull || std::get_if<0>(&I->MCDCParams)) && + "MCDCParams should be empty"); + (void)ParamsShouldBeNull; } // Ensure that all file ids have at least one mapping region. assert(CurrentFileID == (VirtualFileMapping.size() - 1)); diff --git a/llvm/unittests/ProfileData/CoverageMappingTest.cpp b/llvm/unittests/ProfileData/CoverageMappingTest.cpp index 2849781a9dc43b..836afa7bd1b129 100644 --- a/llvm/unittests/ProfileData/CoverageMappingTest.cpp +++ b/llvm/unittests/ProfileData/CoverageMappingTest.cpp @@ -197,8 +197,8 @@ struct CoverageMappingTest : ::testing::TestWithParam> { auto &Regions = InputFunctions.back().Regions; unsigned FileID = getFileIndexForFunction(File); Regions.push_back(CounterMappingRegion::makeDecisionRegion( - CounterMappingRegion::MCDCParameters{Mask, NC}, FileID, LS, CS, LE, - CE)); + CounterMappingRegion::MCDCDecisionParameters{Mask, NC}, FileID, LS, CS, + LE, CE)); } void addMCDCBranchCMR(Counter C1, Counter C2, unsigned ID, unsigned TrueID, @@ -207,8 +207,8 @@ struct CoverageMappingTest : ::testing::TestWithParam> { auto &Regions = InputFunctions.back().Regions; unsigned FileID = getFileIndexForFunction(File); Regions.push_back(CounterMappingRegion::makeBranchRegion( - C1, C2, CounterMappingRegion::MCDCParameters{0, 0, ID, TrueID, FalseID}, - FileID, LS, CS, LE, CE)); + C1, C2, FileID, LS, CS, LE, CE, + CounterMappingRegion::MCDCBranchParameters{ID, TrueID, FalseID})); } void addExpansionCMR(StringRef File, StringRef ExpandedFile, unsigned LS, From 7f4af8de53089cbe4c7e62ba6f2cd6199d048c77 Mon Sep 17 00:00:00 2001 From: NAKAMURA Takumi Date: Sun, 11 Feb 2024 01:43:46 +0900 Subject: [PATCH 2/3] CoverageMappingReader: MCDCConditionID shouldn't be zero --- llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp index bd2ca2afe31d6a..598590db7c39ae 100644 --- a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp +++ b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp @@ -309,7 +309,10 @@ Error RawCoverageMappingReader::readMappingRegionsSubArray( return Err; if (auto Err = readIntMax(FID, std::numeric_limits::max())) return Err; - assert(ID > 0); + if (ID == 0) + return make_error( + coveragemap_error::malformed, + "MCDCConditionID shouldn't be zero"); Params = CounterMappingRegion::MCDCBranchParameters{ (unsigned)ID, (unsigned)TID, (unsigned)FID}; break; From 109d273fe5b06683c20e902844600b81871441a3 Mon Sep 17 00:00:00 2001 From: NAKAMURA Takumi Date: Mon, 12 Feb 2024 18:08:23 +0900 Subject: [PATCH 3/3] static_cast --- llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp index 598590db7c39ae..670fa4a7aeda55 100644 --- a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp +++ b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp @@ -314,7 +314,8 @@ Error RawCoverageMappingReader::readMappingRegionsSubArray( coveragemap_error::malformed, "MCDCConditionID shouldn't be zero"); Params = CounterMappingRegion::MCDCBranchParameters{ - (unsigned)ID, (unsigned)TID, (unsigned)FID}; + static_cast(ID), static_cast(TID), + static_cast(FID)}; break; case CounterMappingRegion::MCDCDecisionRegion: Kind = CounterMappingRegion::MCDCDecisionRegion; @@ -322,8 +323,8 @@ Error RawCoverageMappingReader::readMappingRegionsSubArray( return Err; if (auto Err = readIntMax(NC, std::numeric_limits::max())) return Err; - Params = CounterMappingRegion::MCDCDecisionParameters{(unsigned)BIDX, - (unsigned)NC}; + Params = CounterMappingRegion::MCDCDecisionParameters{ + static_cast(BIDX), static_cast(NC)}; break; default: return make_error(coveragemap_error::malformed,