Skip to content

Commit

Permalink
Revert "[MC/DC][Coverage] Loosen the limit of NumConds from 6 (#82448)"
Browse files Browse the repository at this point in the history 
This broke the lit tests on Mac:
https://green.lab.llvm.org/job/llvm.org/job/clang-stage1-RA/1096/

> By storing possible test vectors instead of combinations of conditions,
> the restriction is dramatically relaxed.
>
> This introduces two options to `cc1`:
>
> * `-fmcdc-max-conditions=32767`
> * `-fmcdc-max-test-vectors=2147483646`
>
> This change makes coverage mapping, profraw, and profdata incompatible
> with Clang-18.
>
> - Bitmap semantics changed. It is incompatible with previous format.
> - `BitmapIdx` in `Decision` points to the end of the bitmap.
> - Bitmap is packed per function.
> - `llvm-cov` can understand `profdata` generated by `llvm-profdata-18`.
>
> RFC:
> https://discourse.llvm.org/t/rfc-coverage-new-algorithm-and-file-format-for-mc-dc/76798

This reverts commit 7ead2d8.
  • Loading branch information
@zmodem
zmodem committed Jun 14, 2024
1 parent 880d370 commit b422fa6
Show file tree
Hide file tree
Showing 42 changed files with 297 additions and 529 deletions.
29 changes: 4 additions & 25 deletions clang/docs/SourceBasedCodeCoverage.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -484,31 +484,10 @@ MC/DC Instrumentation
---------------------

When instrumenting for Modified Condition/Decision Coverage (MC/DC) using the
clang option ``-fcoverage-mcdc``, there are two hard limits.

The maximum number of terms is limited to 32767, which is practical for
handwritten expressions. To be more restrictive in order to enforce coding rules,
use ``-Xclang -fmcdc-max-conditions=n``. Expressions with exceeded condition
counts ``n`` will generate warnings and will be excluded in the MC/DC coverage.

The number of test vectors (the maximum number of possible combinations of
expressions) is limited to 2,147,483,646. In this case, approximately
256MiB (==2GiB/8) is used to record test vectors.

To reduce memory usage, users can limit the maximum number of test vectors per
expression with ``-Xclang -fmcdc-max-test-vectors=m``.
If the number of test vectors resulting from the analysis of an expression
exceeds ``m``, a warning will be issued and the expression will be excluded
from the MC/DC coverage.

The number of test vectors ``m``, for ``n`` terms in an expression, can be
``m <= 2^n`` in the theoretical worst case, but is usually much smaller.
In simple cases, such as expressions consisting of a sequence of single
operators, ``m == n+1``. For example, ``(a && b && c && d && e && f && g)``
requires 8 test vectors.

Expressions such as ``((a0 && b0) || (a1 && b1) || ...)`` can cause the
number of test vectors to increase exponentially.
clang option ``-fcoverage-mcdc``, users are limited to at most **six** leaf-level
conditions in a boolean expression. A warning will be generated for boolean
expressions that contain more than six, and they will not be instrumented for
MC/DC.

Also, if a boolean expression is embedded in the nest of another boolean
expression but separated by a non-logical operator, this is also not supported.
Expand Down
2 changes: 0 additions & 2 deletions clang/include/clang/Basic/CodeGenOptions.def
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,8 +223,6 @@ CODEGENOPT(CoverageMapping , 1, 0) ///< Generate coverage mapping regions to
CODEGENOPT(DumpCoverageMapping , 1, 0) ///< Dump the generated coverage mapping
///< regions.
CODEGENOPT(MCDCCoverage , 1, 0) ///< Enable MC/DC code coverage criteria.
VALUE_CODEGENOPT(MCDCMaxConds, 16, 32767) ///< MC/DC Maximum conditions.
VALUE_CODEGENOPT(MCDCMaxTVs, 32, 0x7FFFFFFE) ///< MC/DC Maximum test vectors.

/// If -fpcc-struct-return or -freg-struct-return is specified.
ENUM_CODEGENOPT(StructReturnConvention, StructReturnConventionKind, 2, SRCK_Default)
Expand Down
8 changes: 0 additions & 8 deletions clang/include/clang/Driver/Options.td
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1790,14 +1790,6 @@ defm mcdc_coverage : BoolFOption<"coverage-mcdc",
"Enable MC/DC criteria when generating code coverage">,
NegFlag<SetFalse, [], [ClangOption], "Disable MC/DC coverage criteria">,
BothFlags<[], [ClangOption, CLOption]>>;
def fmcdc_max_conditions_EQ : Joined<["-"], "fmcdc-max-conditions=">,
Group<f_Group>, Visibility<[CC1Option]>,
HelpText<"Maximum number of conditions in MC/DC coverage">,
MarshallingInfoInt<CodeGenOpts<"MCDCMaxConds">, "32767">;
def fmcdc_max_test_vectors_EQ : Joined<["-"], "fmcdc-max-test-vectors=">,
Group<f_Group>, Visibility<[CC1Option]>,
HelpText<"Maximum number of test vectors in MC/DC coverage">,
MarshallingInfoInt<CodeGenOpts<"MCDCMaxTVs">, "0x7FFFFFFE">;
def fprofile_generate : Flag<["-"], "fprofile-generate">,
Group<f_Group>, Visibility<[ClangOption, CLOption]>,
HelpText<"Generate instrumented code to collect execution counts into default.profraw (overridden by LLVM_PROFILE_FILE env var)">;
Expand Down
50 changes: 23 additions & 27 deletions clang/lib/CodeGen/CodeGenPGO.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,8 @@ struct MapRegionCounters : public RecursiveASTVisitor<MapRegionCounters> {
PGOHash Hash;
/// The map of statements to counters.
llvm::DenseMap<const Stmt *, unsigned> &CounterMap;
/// The next bitmap byte index to assign.
unsigned NextMCDCBitmapIdx;
/// The state of MC/DC Coverage in this function.
MCDC::State &MCDCState;
/// Maximum number of supported MC/DC conditions in a boolean expression.
Expand All @@ -181,7 +183,7 @@ struct MapRegionCounters : public RecursiveASTVisitor<MapRegionCounters> {
MCDC::State &MCDCState, unsigned MCDCMaxCond,
DiagnosticsEngine &Diag)
: NextCounter(0), Hash(HashVersion), CounterMap(CounterMap),
MCDCState(MCDCState), MCDCMaxCond(MCDCMaxCond),
NextMCDCBitmapIdx(0), MCDCState(MCDCState), MCDCMaxCond(MCDCMaxCond),
ProfileVersion(ProfileVersion), Diag(Diag) {}

// Blocks and lambdas are handled as separate functions, so we need not
Expand Down Expand Up @@ -312,8 +314,11 @@ struct MapRegionCounters : public RecursiveASTVisitor<MapRegionCounters> {
return true;
}

// Otherwise, allocate the Decision.
MCDCState.DecisionByStmt[BinOp].BitmapIdx = 0;
// Otherwise, allocate the number of bytes required for the bitmap
// based on the number of conditions. Must be at least 1-byte long.
MCDCState.DecisionByStmt[BinOp].BitmapIdx = NextMCDCBitmapIdx;
unsigned SizeInBits = std::max<unsigned>(1L << NumCond, CHAR_BIT);
NextMCDCBitmapIdx += SizeInBits / CHAR_BIT;
}
return true;
}
Expand Down Expand Up @@ -1078,9 +1083,7 @@ void CodeGenPGO::mapRegionCounters(const Decl *D) {
// for most embedded applications. Setting a maximum value prevents the
// bitmap footprint from growing too large without the user's knowledge. In
// the future, this value could be adjusted with a command-line option.
unsigned MCDCMaxConditions =
(CGM.getCodeGenOpts().MCDCCoverage ? CGM.getCodeGenOpts().MCDCMaxConds
: 0);
unsigned MCDCMaxConditions = (CGM.getCodeGenOpts().MCDCCoverage) ? 6 : 0;

RegionCounterMap.reset(new llvm::DenseMap<const Stmt *, unsigned>);
RegionMCDCState.reset(new MCDC::State);
Expand All @@ -1096,6 +1099,7 @@ void CodeGenPGO::mapRegionCounters(const Decl *D) {
Walker.TraverseDecl(const_cast<CapturedDecl *>(CD));
assert(Walker.NextCounter > 0 && "no entry counter mapped for decl");
NumRegionCounters = Walker.NextCounter;
RegionMCDCState->BitmapBytes = Walker.NextMCDCBitmapIdx;
FunctionHash = Walker.Hash.finalize();
}

Expand Down Expand Up @@ -1228,7 +1232,7 @@ void CodeGenPGO::emitMCDCParameters(CGBuilderTy &Builder) {
// anything.
llvm::Value *Args[3] = {llvm::ConstantExpr::getBitCast(FuncNameVar, I8PtrTy),
Builder.getInt64(FunctionHash),
Builder.getInt32(RegionMCDCState->BitmapBits)};
Builder.getInt32(RegionMCDCState->BitmapBytes)};
Builder.CreateCall(
CGM.getIntrinsic(llvm::Intrinsic::instrprof_mcdc_parameters), Args);
}
Expand All @@ -1246,11 +1250,6 @@ void CodeGenPGO::emitMCDCTestVectorBitmapUpdate(CGBuilderTy &Builder,
if (DecisionStateIter == RegionMCDCState->DecisionByStmt.end())
return;

// Don't create tvbitmap_update if the record is allocated but excluded.
// Or `bitmap |= (1 << 0)` would be wrongly executed to the next bitmap.
if (DecisionStateIter->second.Indices.size() == 0)
return;

// Extract the offset of the global bitmap associated with this expression.
unsigned MCDCTestVectorBitmapOffset = DecisionStateIter->second.BitmapIdx;
auto *I8PtrTy = llvm::PointerType::getUnqual(CGM.getLLVMContext());
Expand All @@ -1262,7 +1261,7 @@ void CodeGenPGO::emitMCDCTestVectorBitmapUpdate(CGBuilderTy &Builder,
// index represents an executed test vector.
llvm::Value *Args[5] = {llvm::ConstantExpr::getBitCast(FuncNameVar, I8PtrTy),
Builder.getInt64(FunctionHash),
Builder.getInt32(0), // Unused
Builder.getInt32(RegionMCDCState->BitmapBytes),
Builder.getInt32(MCDCTestVectorBitmapOffset),
MCDCCondBitmapAddr.emitRawPointer(CGF)};
Builder.CreateCall(
Expand Down Expand Up @@ -1306,22 +1305,19 @@ void CodeGenPGO::emitMCDCCondBitmapUpdate(CGBuilderTy &Builder, const Expr *S,
// Extract the ID of the condition we are setting in the bitmap.
const auto &Branch = BranchStateIter->second;
assert(Branch.ID >= 0 && "Condition has no ID!");
assert(Branch.DecisionStmt);

// Cancel the emission if the Decision is erased after the allocation.
const auto DecisionIter =
RegionMCDCState->DecisionByStmt.find(Branch.DecisionStmt);
if (DecisionIter == RegionMCDCState->DecisionByStmt.end())
return;

const auto &TVIdxs = DecisionIter->second.Indices[Branch.ID];
auto *I8PtrTy = llvm::PointerType::getUnqual(CGM.getLLVMContext());

auto *CurTV = Builder.CreateLoad(MCDCCondBitmapAddr,
"mcdc." + Twine(Branch.ID + 1) + ".cur");
auto *NewTV = Builder.CreateAdd(CurTV, Builder.getInt32(TVIdxs[true]));
NewTV = Builder.CreateSelect(
Val, NewTV, Builder.CreateAdd(CurTV, Builder.getInt32(TVIdxs[false])));
Builder.CreateStore(NewTV, MCDCCondBitmapAddr);
// Emit intrinsic that updates a dedicated temporary value on the stack after
// a condition is evaluated. After the set of conditions has been updated,
// the resulting value is used to update the boolean expression's bitmap.
llvm::Value *Args[5] = {llvm::ConstantExpr::getBitCast(FuncNameVar, I8PtrTy),
Builder.getInt64(FunctionHash),
Builder.getInt32(Branch.ID),
MCDCCondBitmapAddr.emitRawPointer(CGF), Val};
Builder.CreateCall(
CGM.getIntrinsic(llvm::Intrinsic::instrprof_mcdc_condbitmap_update),
Args);
}

void CodeGenPGO::setValueProfilingFlag(llvm::Module &M) {
Expand Down
77 changes: 5 additions & 72 deletions clang/lib/CodeGen/CoverageMappingGen.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,10 +195,6 @@ class SourceMappingRegion {
return std::holds_alternative<mcdc::BranchParameters>(MCDCParams);
}

const auto &getMCDCBranchParams() const {
return mcdc::getParams<const mcdc::BranchParameters>(MCDCParams);
}

bool isMCDCDecision() const {
return std::holds_alternative<mcdc::DecisionParameters>(MCDCParams);
}
Expand All @@ -208,8 +204,6 @@ class SourceMappingRegion {
}

const mcdc::Parameters &getMCDCParams() const { return MCDCParams; }

void resetMCDCParams() { MCDCParams = mcdc::Parameters(); }
};

/// Spelling locations for the start and end of a source region.
Expand Down Expand Up @@ -754,7 +748,6 @@ struct MCDCCoverageBuilder {

llvm::SmallVector<mcdc::ConditionIDs> DecisionStack;
MCDC::State &MCDCState;
const Stmt *DecisionStmt = nullptr;
mcdc::ConditionID NextID = 0;
bool NotMapped = false;

Expand Down Expand Up @@ -784,8 +777,7 @@ struct MCDCCoverageBuilder {

/// Set the given condition's ID.
void setCondID(const Expr *Cond, mcdc::ConditionID ID) {
MCDCState.BranchByStmt[CodeGenFunction::stripCond(Cond)] = {ID,
DecisionStmt};
MCDCState.BranchByStmt[CodeGenFunction::stripCond(Cond)].ID = ID;
}

/// Return the ID of a given condition.
Expand Down Expand Up @@ -816,11 +808,6 @@ struct MCDCCoverageBuilder {
if (NotMapped)
return;

if (NextID == 0) {
DecisionStmt = E;
assert(MCDCState.DecisionByStmt.contains(E));
}

const mcdc::ConditionIDs &ParentDecision = DecisionStack.back();

// If the operator itself has an assigned ID, this means it represents a
Expand Down Expand Up @@ -2135,70 +2122,20 @@ struct CounterCoverageMappingBuilder
subtractCounters(ParentCount, TrueCount));
}

void createOrCancelDecision(const BinaryOperator *E, unsigned Since) {
void createDecision(const BinaryOperator *E) {
unsigned NumConds = MCDCBuilder.getTotalConditionsAndReset(E);
if (NumConds == 0)
return;

// Extract [ID, Conds] to construct the graph.
llvm::SmallVector<mcdc::ConditionIDs> CondIDs(NumConds);
for (const auto &SR : ArrayRef(SourceRegions).slice(Since)) {
if (SR.isMCDCBranch()) {
auto [ID, Conds] = SR.getMCDCBranchParams();
CondIDs[ID] = Conds;
}
}

// Construct the graph and calculate `Indices`.
mcdc::TVIdxBuilder Builder(CondIDs);
unsigned NumTVs = Builder.NumTestVectors;
unsigned MaxTVs = CVM.getCodeGenModule().getCodeGenOpts().MCDCMaxTVs;
assert(MaxTVs < mcdc::TVIdxBuilder::HardMaxTVs);

if (NumTVs > MaxTVs) {
// NumTVs exceeds MaxTVs -- warn and cancel the Decision.
cancelDecision(E, Since, NumTVs, MaxTVs);
return;
}

// Update the state for CodeGenPGO
assert(MCDCState.DecisionByStmt.contains(E));
MCDCState.DecisionByStmt[E] = {
MCDCState.BitmapBits, // Top
std::move(Builder.Indices),
};

auto DecisionParams = mcdc::DecisionParameters{
MCDCState.BitmapBits += NumTVs, // Tail
MCDCState.DecisionByStmt[E].BitmapIdx,
NumConds,
};

// Create MCDC Decision Region.
createDecisionRegion(E, DecisionParams);
}

// Warn and cancel the Decision.
void cancelDecision(const BinaryOperator *E, unsigned Since, int NumTVs,
int MaxTVs) {
auto &Diag = CVM.getCodeGenModule().getDiags();
unsigned DiagID =
Diag.getCustomDiagID(DiagnosticsEngine::Warning,
"unsupported MC/DC boolean expression; "
"number of test vectors (%0) exceeds max (%1). "
"Expression will not be covered");
Diag.Report(E->getBeginLoc(), DiagID) << NumTVs << MaxTVs;

// Restore MCDCBranch to Branch.
for (auto &SR : MutableArrayRef(SourceRegions).slice(Since)) {
assert(!SR.isMCDCDecision() && "Decision shouldn't be seen here");
if (SR.isMCDCBranch())
SR.resetMCDCParams();
}

// Tell CodeGenPGO not to instrument.
MCDCState.DecisionByStmt.erase(E);
}

/// Check if E belongs to system headers.
bool isExprInSystemHeader(const BinaryOperator *E) const {
return (!SystemHeadersCoverage &&
Expand All @@ -2215,8 +2152,6 @@ struct CounterCoverageMappingBuilder

bool IsRootNode = MCDCBuilder.isIdle();

unsigned SourceRegionsSince = SourceRegions.size();

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

Expand Down Expand Up @@ -2255,7 +2190,7 @@ struct CounterCoverageMappingBuilder

// Create MCDC Decision Region if at top-level (root).
if (IsRootNode)
createOrCancelDecision(E, SourceRegionsSince);
createDecision(E);
}

// Determine whether the right side of OR operation need to be visited.
Expand All @@ -2276,8 +2211,6 @@ struct CounterCoverageMappingBuilder

bool IsRootNode = MCDCBuilder.isIdle();

unsigned SourceRegionsSince = SourceRegions.size();

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

Expand Down Expand Up @@ -2320,7 +2253,7 @@ struct CounterCoverageMappingBuilder

// Create MCDC Decision Region if at top-level (root).
if (IsRootNode)
createOrCancelDecision(E, SourceRegionsSince);
createDecision(E);
}

void VisitLambdaExpr(const LambdaExpr *LE) {
Expand Down
4 changes: 1 addition & 3 deletions clang/lib/CodeGen/MCDCState.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,16 @@ using namespace llvm::coverage::mcdc;

/// Per-Function MC/DC state
struct State {
unsigned BitmapBits = 0;
unsigned BitmapBytes = 0;

struct Decision {
unsigned BitmapIdx;
llvm::SmallVector<std::array<int, 2>> Indices;
};

llvm::DenseMap<const Stmt *, Decision> DecisionByStmt;

struct Branch {
ConditionID ID;
const Stmt *DecisionStmt;
};

llvm::DenseMap<const Stmt *, Branch> BranchByStmt;
Expand Down
Loading

0 comments on commit b422fa6

Please sign in to comment.