Skip to content

Commit

Permalink
[clang][CoverageMapping] Refactor setting MC/DC True/False Condition …
Browse files Browse the repository at this point in the history 
…IDs (#78202)

Clean-up of the algorithm that assigns MC/DC True/False control-flow
condition IDs when constructing an MC/DC decision region. This patch
creates a common API for setting/getting the condition IDs, making the
binary logical operator visitor functions much cleaner.

This patch also fixes issue
#77873 in which a record's
control flow map can be malformed due to an incorrect calculation of the
True/False condition IDs.
  • Loading branch information
@evodius96
evodius96 committed Jan 18, 2024
1 parent 0fc5f4b commit 2286789
Show file tree
Hide file tree
Showing 2 changed files with 179 additions and 119 deletions.
221 changes: 102 additions & 119 deletions clang/lib/CodeGen/CoverageMappingGen.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,11 @@ struct EmptyCoverageMappingBuilder : public CoverageMappingBuilder {
/// creation.
struct MCDCCoverageBuilder {

struct DecisionIDPair {
MCDCConditionID TrueID = 0;
MCDCConditionID FalseID = 0;
};

/// The AST walk recursively visits nested logical-AND or logical-OR binary
/// operator nodes and then visits their LHS and RHS children nodes. As this
/// happens, the algorithm will assign IDs to each operator's LHS and RHS side
Expand Down Expand Up @@ -616,14 +621,14 @@ struct MCDCCoverageBuilder {
///
/// A node ID of '0' always means MC/DC isn't being tracked.
///
/// As the AST walk proceeds recursively, the algorithm will also use stacks
/// As the AST walk proceeds recursively, the algorithm will also use a stack
/// to track the IDs of logical-AND and logical-OR operations on the RHS so
/// that it can be determined which nodes are executed next, depending on how
/// a LHS or RHS of a logical-AND or logical-OR is evaluated. This
/// information relies on the assigned IDs and are embedded within the
/// coverage region IDs of each branch region associated with a leaf-level
/// condition. This information helps the visualization tool reconstruct all
/// possible test vectors for the purposes of MC/DC analysis. if a "next" node
/// possible test vectors for the purposes of MC/DC analysis. If a "next" node
/// ID is '0', it means it's the end of the test vector. The following rules
/// are used:
///
Expand Down Expand Up @@ -663,54 +668,40 @@ struct MCDCCoverageBuilder {
private:
CodeGenModule &CGM;

llvm::SmallVector<MCDCConditionID> AndRHS;
llvm::SmallVector<MCDCConditionID> OrRHS;
llvm::SmallVector<const BinaryOperator *> NestLevel;
llvm::SmallVector<DecisionIDPair> DecisionStack;
llvm::DenseMap<const Stmt *, MCDCConditionID> &CondIDs;
llvm::DenseMap<const Stmt *, unsigned> &MCDCBitmapMap;
MCDCConditionID NextID = 1;
bool NotMapped = false;

/// Represent a sentinel value of [0,0] for the bottom of DecisionStack.
static constexpr DecisionIDPair DecisionStackSentinel{0, 0};

/// Is this a logical-AND operation?
bool isLAnd(const BinaryOperator *E) const {
return E->getOpcode() == BO_LAnd;
}

/// Push an ID onto the corresponding RHS stack.
void pushRHS(const BinaryOperator *E) {
llvm::SmallVector<MCDCConditionID> &rhs = isLAnd(E) ? AndRHS : OrRHS;
rhs.push_back(CondIDs[CodeGenFunction::stripCond(E->getRHS())]);
}

/// Pop an ID from the corresponding RHS stack.
void popRHS(const BinaryOperator *E) {
llvm::SmallVector<MCDCConditionID> &rhs = isLAnd(E) ? AndRHS : OrRHS;
if (!rhs.empty())
rhs.pop_back();
}

/// If the expected ID is on top, pop it off the corresponding RHS stack.
void popRHSifTop(const BinaryOperator *E) {
if (!OrRHS.empty() && CondIDs[E] == OrRHS.back())
OrRHS.pop_back();
else if (!AndRHS.empty() && CondIDs[E] == AndRHS.back())
AndRHS.pop_back();
}

public:
MCDCCoverageBuilder(CodeGenModule &CGM,
llvm::DenseMap<const Stmt *, MCDCConditionID> &CondIDMap,
llvm::DenseMap<const Stmt *, unsigned> &MCDCBitmapMap)
: CGM(CGM), CondIDs(CondIDMap), MCDCBitmapMap(MCDCBitmapMap) {}
: CGM(CGM), DecisionStack(1, DecisionStackSentinel), CondIDs(CondIDMap),
MCDCBitmapMap(MCDCBitmapMap) {}

/// Return the ID of the RHS of the next, upper nest-level logical-OR.
MCDCConditionID getNextLOrCondID() const {
return OrRHS.empty() ? 0 : OrRHS.back();
}
/// Return whether the build of the control flow map is at the top-level
/// (root) of a logical operator nest in a boolean expression prior to the
/// assignment of condition IDs.
bool isIdle() const { return (NextID == 1 && !NotMapped); }

/// Return whether any IDs have been assigned in the build of the control
/// flow map, indicating that the map is being generated for this boolean
/// expression.
bool isBuilding() const { return (NextID > 1); }

/// Return the ID of the RHS of the next, upper nest-level logical-AND.
MCDCConditionID getNextLAndCondID() const {
return AndRHS.empty() ? 0 : AndRHS.back();
/// Set the given condition's ID.
void setCondID(const Expr *Cond, MCDCConditionID ID) {
CondIDs[CodeGenFunction::stripCond(Cond)] = ID;
}

/// Return the ID of a given condition.
Expand All @@ -722,6 +713,9 @@ struct MCDCCoverageBuilder {
return I->second;
}

/// Return the LHS Decision ([0,0] if not set).
const DecisionIDPair &back() const { return DecisionStack.back(); }

/// Push the binary operator statement to track the nest level and assign IDs
/// to the operator's LHS and RHS. The RHS may be a larger subtree that is
/// broken up on successive levels.
Expand All @@ -730,68 +724,67 @@ struct MCDCCoverageBuilder {
return;

// If binary expression is disqualified, don't do mapping.
if (NestLevel.empty() &&
!MCDCBitmapMap.contains(CodeGenFunction::stripCond(E)))
if (!isBuilding() && !MCDCBitmapMap.contains(CodeGenFunction::stripCond(E)))
NotMapped = true;

// Push Stmt on 'NestLevel' stack to keep track of nest location.
NestLevel.push_back(E);

// Don't go any further if we don't need to map condition IDs.
if (NotMapped)
return;

const DecisionIDPair &ParentDecision = DecisionStack.back();

// If the operator itself has an assigned ID, this means it represents a
// larger subtree. In this case, pop its ID out of the RHS stack and
// assign that ID to its LHS node. Its RHS will receive a new ID.
if (CondIDs.contains(CodeGenFunction::stripCond(E))) {
// If Stmt has an ID, assign its ID to LHS
CondIDs[CodeGenFunction::stripCond(E->getLHS())] = CondIDs[E];

// Since the operator's LHS assumes the operator's same ID, pop the
// operator from the RHS stack so that if LHS short-circuits, it won't be
// incorrectly re-used as the node executed next.
popRHSifTop(E);
} else {
// Otherwise, assign ID+1 to LHS.
CondIDs[CodeGenFunction::stripCond(E->getLHS())] = NextID++;
}
// larger subtree. In this case, assign that ID to its LHS node. Its RHS
// will receive a new ID below. Otherwise, assign ID+1 to LHS.
if (CondIDs.contains(CodeGenFunction::stripCond(E)))
setCondID(E->getLHS(), getCondID(E));
else
setCondID(E->getLHS(), NextID++);

// Assign ID+1 to RHS.
CondIDs[CodeGenFunction::stripCond(E->getRHS())] = NextID++;
// Assign a ID+1 for the RHS.
MCDCConditionID RHSid = NextID++;
setCondID(E->getRHS(), RHSid);

// Push ID of Stmt's RHS so that LHS nodes know about it
pushRHS(E);
// Push the LHS decision IDs onto the DecisionStack.
if (isLAnd(E))
DecisionStack.push_back({RHSid, ParentDecision.FalseID});
else
DecisionStack.push_back({ParentDecision.TrueID, RHSid});
}

/// Pop and return the LHS Decision ([0,0] if not set).
DecisionIDPair pop() {
if (!CGM.getCodeGenOpts().MCDCCoverage || NotMapped)
return DecisionStack.front();

assert(DecisionStack.size() > 1);
DecisionIDPair D = DecisionStack.back();
DecisionStack.pop_back();
return D;
}

/// Pop the binary operator from the next level. If the walk is at the top of
/// the next, assign the total number of conditions.
unsigned popAndReturnCondCount(const BinaryOperator *E) {
/// Return the total number of conditions and reset the state. The number of
/// conditions is zero if the expression isn't mapped.
unsigned getTotalConditionsAndReset(const BinaryOperator *E) {
if (!CGM.getCodeGenOpts().MCDCCoverage)
return 0;

unsigned TotalConds = 0;

// Pop Stmt from 'NestLevel' stack.
assert(NestLevel.back() == E);
NestLevel.pop_back();
assert(!isIdle());
assert(DecisionStack.size() == 1);

// Reset state if not doing mapping.
if (NestLevel.empty() && NotMapped) {
if (NotMapped) {
NotMapped = false;
assert(NextID == 1);
return 0;
}

// Pop RHS ID.
popRHS(E);
// Set number of conditions and reset.
unsigned TotalConds = NextID - 1;

// If at the parent (NestLevel=0), set conds and reset.
if (NestLevel.empty()) {
TotalConds = NextID - 1;
// Reset ID back to beginning.
NextID = 1;

// Reset ID back to beginning.
NextID = 1;
}
return TotalConds;
}
};
Expand Down Expand Up @@ -1018,13 +1011,15 @@ struct CounterCoverageMappingBuilder
return (Cond->EvaluateAsInt(Result, CVM.getCodeGenModule().getContext()));
}

using MCDCDecisionIDPair = MCDCCoverageBuilder::DecisionIDPair;

/// Create a Branch Region around an instrumentable condition for coverage
/// and add it to the function's SourceRegions. A branch region tracks a
/// "True" counter and a "False" counter for boolean expressions that
/// result in the generation of a branch.
void createBranchRegion(const Expr *C, Counter TrueCnt, Counter FalseCnt,
MCDCConditionID ID = 0, MCDCConditionID TrueID = 0,
MCDCConditionID FalseID = 0) {
void
createBranchRegion(const Expr *C, Counter TrueCnt, Counter FalseCnt,
const MCDCDecisionIDPair &IDPair = MCDCDecisionIDPair()) {
// Check for NULL conditions.
if (!C)
return;
Expand All @@ -1034,6 +1029,10 @@ struct CounterCoverageMappingBuilder
// function's SourceRegions) because it doesn't apply to any other source
// code other than the Condition.
if (CodeGenFunction::isInstrumentedCondition(C)) {
MCDCConditionID ID = MCDCBuilder.getCondID(C);
MCDCConditionID TrueID = IDPair.TrueID;
MCDCConditionID FalseID = 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
// zero. This allows us to visualize them in a special way.
Expand Down Expand Up @@ -1822,20 +1821,28 @@ struct CounterCoverageMappingBuilder
}

void VisitBinLAnd(const BinaryOperator *E) {
// Keep track of Binary Operator and assign MCDC condition IDs
bool IsRootNode = MCDCBuilder.isIdle();

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

extendRegion(E->getLHS());
propagateCounts(getRegion().getCounter(), E->getLHS());
handleFileExit(getEnd(E->getLHS()));

// Track LHS True/False Decision.
const auto DecisionLHS = MCDCBuilder.pop();

// Counter tracks the right hand side of a logical and operator.
extendRegion(E->getRHS());
propagateCounts(getRegionCounter(E), E->getRHS());

// Process Binary Operator and create MCDC Decision Region if top-level
// Track RHS True/False Decision.
const auto DecisionRHS = MCDCBuilder.back();

// Create MCDC Decision Region if at top-level (root).
unsigned NumConds = 0;
if ((NumConds = MCDCBuilder.popAndReturnCondCount(E)))
if (IsRootNode && (NumConds = MCDCBuilder.getTotalConditionsAndReset(E)))
createDecisionRegion(E, getRegionBitmap(E), NumConds);

// Extract the RHS's Execution Counter.
Expand All @@ -1847,30 +1854,13 @@ struct CounterCoverageMappingBuilder
// Extract the Parent Region Counter.
Counter ParentCnt = getRegion().getCounter();

// Extract the MCDC condition IDs (returns 0 if not needed).
MCDCConditionID NextOrID = MCDCBuilder.getNextLOrCondID();
MCDCConditionID NextAndID = MCDCBuilder.getNextLAndCondID();
MCDCConditionID LHSid = MCDCBuilder.getCondID(E->getLHS());
MCDCConditionID RHSid = MCDCBuilder.getCondID(E->getRHS());

// Create Branch Region around LHS condition.
// MC/DC: For "LHS && RHS"
// - If LHS is TRUE, execution goes to the RHS.
// - If LHS is FALSE, execution goes to the LHS of the next logical-OR.
// If that does not exist, execution exits (ID == 0).
createBranchRegion(E->getLHS(), RHSExecCnt,
subtractCounters(ParentCnt, RHSExecCnt), LHSid, RHSid,
NextOrID);
subtractCounters(ParentCnt, RHSExecCnt), DecisionLHS);

// Create Branch Region around RHS condition.
// MC/DC: For "LHS && RHS"
// - If RHS is TRUE, execution goes to LHS of the next logical-AND.
// If that does not exist, execution exits (ID == 0).
// - If RHS is FALSE, execution goes to the LHS of the next logical-OR.
// If that does not exist, execution exits (ID == 0).
createBranchRegion(E->getRHS(), RHSTrueCnt,
subtractCounters(RHSExecCnt, RHSTrueCnt), RHSid,
NextAndID, NextOrID);
subtractCounters(RHSExecCnt, RHSTrueCnt), DecisionRHS);
}

// Determine whether the right side of OR operation need to be visited.
Expand All @@ -1884,20 +1874,28 @@ struct CounterCoverageMappingBuilder
}

void VisitBinLOr(const BinaryOperator *E) {
// Keep track of Binary Operator and assign MCDC condition IDs
bool IsRootNode = MCDCBuilder.isIdle();

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

extendRegion(E->getLHS());
Counter OutCount = propagateCounts(getRegion().getCounter(), E->getLHS());
handleFileExit(getEnd(E->getLHS()));

// Track LHS True/False Decision.
const auto DecisionLHS = MCDCBuilder.pop();

// Counter tracks the right hand side of a logical or operator.
extendRegion(E->getRHS());
propagateCounts(getRegionCounter(E), E->getRHS());

// Process Binary Operator and create MCDC Decision Region if top-level
// Track RHS True/False Decision.
const auto DecisionRHS = MCDCBuilder.back();

// Create MCDC Decision Region if at top-level (root).
unsigned NumConds = 0;
if ((NumConds = MCDCBuilder.popAndReturnCondCount(E)))
if (IsRootNode && (NumConds = MCDCBuilder.getTotalConditionsAndReset(E)))
createDecisionRegion(E, getRegionBitmap(E), NumConds);

// Extract the RHS's Execution Counter.
Expand All @@ -1913,28 +1911,13 @@ struct CounterCoverageMappingBuilder
// Extract the Parent Region Counter.
Counter ParentCnt = getRegion().getCounter();

// Extract the MCDC condition IDs (returns 0 if not needed).
MCDCConditionID NextOrID = MCDCBuilder.getNextLOrCondID();
MCDCConditionID NextAndID = MCDCBuilder.getNextLAndCondID();
MCDCConditionID LHSid = MCDCBuilder.getCondID(E->getLHS());
MCDCConditionID RHSid = MCDCBuilder.getCondID(E->getRHS());

// Create Branch Region around LHS condition.
// MC/DC: For "LHS || RHS"
// - If LHS is TRUE, execution goes to the LHS of the next logical-AND.
// If that does not exist, execution exits (ID == 0).
// - If LHS is FALSE, execution goes to the RHS.
createBranchRegion(E->getLHS(), subtractCounters(ParentCnt, RHSExecCnt),
RHSExecCnt, LHSid, NextAndID, RHSid);
RHSExecCnt, DecisionLHS);

// Create Branch Region around RHS condition.
// MC/DC: For "LHS || RHS"
// - If RHS is TRUE, execution goes to LHS of the next logical-AND.
// If that does not exist, execution exits (ID == 0).
// - If RHS is FALSE, execution goes to the LHS of the next logical-OR.
// If that does not exist, execution exits (ID == 0).
createBranchRegion(E->getRHS(), subtractCounters(RHSExecCnt, RHSFalseCnt),
RHSFalseCnt, RHSid, NextAndID, NextOrID);
RHSFalseCnt, DecisionRHS);
}

void VisitLambdaExpr(const LambdaExpr *LE) {
Expand Down

0 comments on commit 2286789

Please sign in to comment.