Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

llvm-cov assertion failure when handling MC/DC that involves macros #80098

Closed
wants to merge 2 commits into from
Closed

llvm-cov assertion failure when handling MC/DC that involves macros #80098

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
daf74be
[Coverage][llvm-cov] Fix failures to handle MC/DC decisions involving…
whentojump Jan 31, 2024
cd5a338
[Coverage][clang] Fix failures to handle MC/DC decisions involving ma…
whentojump Jan 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 29 additions & 17 deletions clang/lib/CodeGen/CoverageMappingGen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -857,6 +857,8 @@ struct CounterCoverageMappingBuilder

unsigned getRegionBitmap(const Stmt *S) { return MCDCBitmapMap[S]; }

unsigned long MCDCDebugCounter;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is its preferable name? I guess you expects unique ID for debug for now.


/// Push a region onto the stack.
///
/// Returns the index on the stack where the region was pushed. This can be
Expand All @@ -866,7 +868,7 @@ struct CounterCoverageMappingBuilder
std::optional<SourceLocation> EndLoc = std::nullopt,
std::optional<Counter> FalseCount = std::nullopt,
MCDCConditionID ID = 0, MCDCConditionID TrueID = 0,
MCDCConditionID FalseID = 0) {
MCDCConditionID FalseID = 0, MCDCConditionID GroupID = 0) {

if (StartLoc && !FalseCount) {
MostRecentLocation = *StartLoc;
Expand All @@ -886,17 +888,19 @@ struct CounterCoverageMappingBuilder
if (EndLoc && EndLoc->isInvalid())
EndLoc = std::nullopt;
RegionStack.emplace_back(Count, FalseCount,
MCDCParameters{0, 0, ID, TrueID, FalseID},
MCDCParameters{
0, 0,
ID, TrueID, FalseID, GroupID},
StartLoc, EndLoc);

return RegionStack.size() - 1;
}

size_t pushRegion(unsigned BitmapIdx, unsigned Conditions,
size_t pushRegion(unsigned BitmapIdx, unsigned Conditions, MCDCConditionID GroupID,
std::optional<SourceLocation> StartLoc = std::nullopt,
std::optional<SourceLocation> EndLoc = std::nullopt) {

RegionStack.emplace_back(MCDCParameters{BitmapIdx, Conditions}, StartLoc,
RegionStack.emplace_back(MCDCParameters{BitmapIdx, Conditions, 0, 0, 0, GroupID}, StartLoc,
EndLoc);

return RegionStack.size() - 1;
Expand Down Expand Up @@ -1032,7 +1036,8 @@ struct CounterCoverageMappingBuilder
/// result in the generation of a branch.
void
createBranchRegion(const Expr *C, Counter TrueCnt, Counter FalseCnt,
const MCDCDecisionIDPair &IDPair = MCDCDecisionIDPair()) {
const MCDCDecisionIDPair &IDPair = MCDCDecisionIDPair(),
MCDCConditionID GroupID = 0) {
// Check for NULL conditions.
if (!C)
return;
Expand All @@ -1054,19 +1059,19 @@ 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(), ID, TrueID, FalseID, GroupID));
else
// Otherwise, create a region with the True counter and False counter.
popRegions(pushRegion(TrueCnt, getStart(C), getEnd(C), FalseCnt, ID,
TrueID, FalseID));
TrueID, FalseID, GroupID));
}
}

/// Create a Decision Region with a BitmapIdx and number of Conditions. This
/// type of region "contains" branch regions, one for each of the conditions.
/// The visualization tool will group everything together.
void createDecisionRegion(const Expr *C, unsigned BitmapIdx, unsigned Conds) {
popRegions(pushRegion(BitmapIdx, Conds, getStart(C), getEnd(C)));
void createDecisionRegion(const Expr *C, unsigned BitmapIdx, unsigned Conds, MCDCConditionID GroupID) {
popRegions(pushRegion(BitmapIdx, Conds, GroupID, getStart(C), getEnd(C)));
}

/// Create a Branch Region around a SwitchCase for code coverage
Expand Down Expand Up @@ -1153,7 +1158,8 @@ struct CounterCoverageMappingBuilder
I.getCounter(), I.getFalseCounter(),
MCDCParameters{0, 0, I.getMCDCParams().ID,
I.getMCDCParams().TrueID,
I.getMCDCParams().FalseID},
I.getMCDCParams().FalseID,
I.getMCDCParams().GroupID},
Loc, getEndOfFileOrMacro(Loc), I.isBranch());
else
SourceRegions.emplace_back(I.getCounter(), Loc,
Expand Down Expand Up @@ -1342,7 +1348,9 @@ struct CounterCoverageMappingBuilder
SourceManager &SM, const LangOptions &LangOpts)
: CoverageMappingBuilder(CVM, SM, LangOpts), CounterMap(CounterMap),
MCDCBitmapMap(MCDCBitmapMap),
MCDCBuilder(CVM.getCodeGenModule(), CondIDMap, MCDCBitmapMap) {}
MCDCBuilder(CVM.getCodeGenModule(), CondIDMap, MCDCBitmapMap) {
MCDCDebugCounter = 0;
}

/// Write the mapping data to the output stream
void write(llvm::raw_ostream &OS) {
Expand Down Expand Up @@ -1973,6 +1981,8 @@ struct CounterCoverageMappingBuilder
void VisitBinLAnd(const BinaryOperator *E) {
bool IsRootNode = MCDCBuilder.isIdle();

MCDCDebugCounter++;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't understand the strategy to assign it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi thanks again for taking the look!
My intuition was to make the ID different whenever a new decision region is going to be added by calling createDecisionRegion(). There might be something obvious that I ignored.
And after reading some of the other posts (I'm still learning most of them, please bear with that!), I generally agree with the approaches whose modifications are minimized within llvm-cov, instead of touching the front end.

Regards


// Keep track of Binary Operator and assign MCDC condition IDs.
MCDCBuilder.pushAndAssignIDs(E);

Expand All @@ -1993,7 +2003,7 @@ struct CounterCoverageMappingBuilder
// Create MCDC Decision Region if at top-level (root).
unsigned NumConds = 0;
if (IsRootNode && (NumConds = MCDCBuilder.getTotalConditionsAndReset(E)))
createDecisionRegion(E, getRegionBitmap(E), NumConds);
createDecisionRegion(E, getRegionBitmap(E), NumConds, MCDCDebugCounter);

// Extract the RHS's Execution Counter.
Counter RHSExecCnt = getRegionCounter(E);
Expand All @@ -2006,11 +2016,11 @@ struct CounterCoverageMappingBuilder

// Create Branch Region around LHS condition.
createBranchRegion(E->getLHS(), RHSExecCnt,
subtractCounters(ParentCnt, RHSExecCnt), DecisionLHS);
subtractCounters(ParentCnt, RHSExecCnt), DecisionLHS, MCDCDebugCounter);

// Create Branch Region around RHS condition.
createBranchRegion(E->getRHS(), RHSTrueCnt,
subtractCounters(RHSExecCnt, RHSTrueCnt), DecisionRHS);
subtractCounters(RHSExecCnt, RHSTrueCnt), DecisionRHS, MCDCDebugCounter);
}

// Determine whether the right side of OR operation need to be visited.
Expand All @@ -2026,6 +2036,8 @@ struct CounterCoverageMappingBuilder
void VisitBinLOr(const BinaryOperator *E) {
bool IsRootNode = MCDCBuilder.isIdle();

MCDCDebugCounter++;

// Keep track of Binary Operator and assign MCDC condition IDs.
MCDCBuilder.pushAndAssignIDs(E);

Expand All @@ -2046,7 +2058,7 @@ struct CounterCoverageMappingBuilder
// Create MCDC Decision Region if at top-level (root).
unsigned NumConds = 0;
if (IsRootNode && (NumConds = MCDCBuilder.getTotalConditionsAndReset(E)))
createDecisionRegion(E, getRegionBitmap(E), NumConds);
createDecisionRegion(E, getRegionBitmap(E), NumConds, MCDCDebugCounter);

// Extract the RHS's Execution Counter.
Counter RHSExecCnt = getRegionCounter(E);
Expand All @@ -2063,11 +2075,11 @@ struct CounterCoverageMappingBuilder

// Create Branch Region around LHS condition.
createBranchRegion(E->getLHS(), subtractCounters(ParentCnt, RHSExecCnt),
RHSExecCnt, DecisionLHS);
RHSExecCnt, DecisionLHS, MCDCDebugCounter);

// Create Branch Region around RHS condition.
createBranchRegion(E->getRHS(), subtractCounters(RHSExecCnt, RHSFalseCnt),
RHSFalseCnt, DecisionRHS);
RHSFalseCnt, DecisionRHS, MCDCDebugCounter);
}

void VisitLambdaExpr(const LambdaExpr *LE) {
Expand Down
1 change: 1 addition & 0 deletions llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@ struct CounterMappingRegion {
/// 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 GroupID;
};

/// Primary Counter that is also used for Branch Regions (TrueCount).
Expand Down
8 changes: 6 additions & 2 deletions llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ 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 = 0, NC = 0, ID = 0, TID = 0, FID = 0, GID = 0;
CounterMappingRegion::RegionKind Kind = CounterMappingRegion::CodeRegion;

// Read the combined counter + region kind.
Expand Down Expand Up @@ -308,13 +308,17 @@ Error RawCoverageMappingReader::readMappingRegionsSubArray(
return Err;
if (auto Err = readIntMax(FID, std::numeric_limits<unsigned>::max()))
return Err;
if (auto Err = readIntMax(GID, std::numeric_limits<unsigned>::max()))
return Err;
break;
case CounterMappingRegion::MCDCDecisionRegion:
Kind = CounterMappingRegion::MCDCDecisionRegion;
if (auto Err = readIntMax(BIDX, std::numeric_limits<unsigned>::max()))
return Err;
if (auto Err = readIntMax(NC, std::numeric_limits<unsigned>::max()))
return Err;
if (auto Err = readIntMax(GID, std::numeric_limits<unsigned>::max()))
return Err;
break;
default:
return make_error<CoverageMapError>(coveragemap_error::malformed,
Expand Down Expand Up @@ -374,7 +378,7 @@ Error RawCoverageMappingReader::readMappingRegionsSubArray(
CounterMappingRegion::MCDCParameters{
static_cast<unsigned>(BIDX), static_cast<unsigned>(NC),
static_cast<unsigned>(ID), static_cast<unsigned>(TID),
static_cast<unsigned>(FID)},
static_cast<unsigned>(FID), static_cast<unsigned>(GID)},
InferredFileID, ExpandedFileID, LineStart, ColumnStart,
LineStart + NumLines, ColumnEnd, Kind);
if (CMR.startLoc() > CMR.endLoc())
Expand Down
10 changes: 7 additions & 3 deletions llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,8 @@ void CoverageMappingWriter::write(raw_ostream &OS) {
// location. Sort by region kinds to ensure stable order for tests.
llvm::stable_sort(MappingRegions, [](const CounterMappingRegion &LHS,
const CounterMappingRegion &RHS) {
if (LHS.MCDCParams.GroupID != RHS.MCDCParams.GroupID)
return LHS.MCDCParams.GroupID < RHS.MCDCParams.GroupID;
if (LHS.FileID != RHS.FileID)
return LHS.FileID < RHS.FileID;
if (LHS.startLoc() != RHS.startLoc())
Expand Down Expand Up @@ -192,7 +194,7 @@ void CoverageMappingWriter::write(raw_ostream &OS) {
for (auto I = MappingRegions.begin(), E = MappingRegions.end(); I != E; ++I) {
if (I->FileID != CurrentFileID) {
// Ensure that all file ids have at least one mapping region.
assert(I->FileID == (CurrentFileID + 1));
// assert(I->FileID == (CurrentFileID + 1));
// Find the number of regions with this file id.
unsigned RegionCount = 1;
for (auto J = I + 1; J != E && I->FileID == J->FileID; ++J)
Expand Down Expand Up @@ -246,16 +248,18 @@ void CoverageMappingWriter::write(raw_ostream &OS) {
encodeULEB128(unsigned(I->MCDCParams.ID), OS);
encodeULEB128(unsigned(I->MCDCParams.TrueID), OS);
encodeULEB128(unsigned(I->MCDCParams.FalseID), OS);
encodeULEB128(unsigned(I->MCDCParams.GroupID), OS);
break;
case CounterMappingRegion::MCDCDecisionRegion:
encodeULEB128(unsigned(I->Kind)
<< Counter::EncodingCounterTagAndExpansionRegionTagBits,
OS);
encodeULEB128(unsigned(I->MCDCParams.BitmapIdx), OS);
encodeULEB128(unsigned(I->MCDCParams.NumConditions), OS);
encodeULEB128(unsigned(I->MCDCParams.GroupID), OS);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it the enhancement?

break;
}
assert(I->LineStart >= PrevLineStart);
// assert(I->LineStart >= PrevLineStart);
encodeULEB128(I->LineStart - PrevLineStart, OS);
encodeULEB128(I->ColumnStart, OS);
assert(I->LineEnd >= I->LineStart);
Expand All @@ -264,7 +268,7 @@ void CoverageMappingWriter::write(raw_ostream &OS) {
PrevLineStart = I->LineStart;
}
// Ensure that all file ids have at least one mapping region.
assert(CurrentFileID == (VirtualFileMapping.size() - 1));
// assert(CurrentFileID == (VirtualFileMapping.size() - 1));
}

void TestingFormatWriter::write(raw_ostream &OS, TestingFormatVersion Version) {
Expand Down
Loading