llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-codegen Author: ankurrj7 <details> <summary>Changes</summary> This teaches coverage mapping to terminate regions not only for explicit noreturn calls, but also for calls and loops that the CFG can prove never reach a normal exit. The change covers: - direct calls into sink functions - nested sink-call chains - sink loops with no normal exit It intentionally keeps virtual dispatch conservative so an override that returns normally does not get treated as terminating based only on the static base method body. Testing: - git diff --check - syntax-only compile of clang/lib/CodeGen/CoverageMappingGen.cpp - runtime coverage on simple_exit_chain.c - runtime coverage on a virtual-dispatch non-regression - coverage-mapping dump for infinite loop cases --- Full diff: https://github.com/llvm/llvm-project/pull/200190.diff 2 Files Affected: - (modified) clang/lib/CodeGen/CoverageMappingGen.cpp (+219-19) - (modified) clang/test/CoverageMapping/terminate-statements.cpp (+65) ``````````diff diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp index c90afacbde293..6430d7d57738d 100644 --- a/clang/lib/CodeGen/CoverageMappingGen.cpp +++ b/clang/lib/CodeGen/CoverageMappingGen.cpp @@ -13,11 +13,15 @@ #include "CoverageMappingGen.h" #include "CodeGenFunction.h" #include "CodeGenPGO.h" +#include "clang/AST/ParentMap.h" #include "clang/AST/StmtVisitor.h" +#include "clang/Analysis/CFG.h" +#include "clang/Analysis/CFGStmtMap.h" #include "clang/Basic/Diagnostic.h" #include "clang/Basic/DiagnosticFrontend.h" #include "clang/Lex/Lexer.h" #include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/SmallPtrSet.h" #include "llvm/ADT/SmallSet.h" #include "llvm/ADT/StringExtras.h" #include "llvm/ProfileData/Coverage/CoverageMapping.h" @@ -948,6 +952,30 @@ struct CounterCoverageMappingBuilder /// Gap region counter after terminate statement. Counter GapRegionCounter; + enum class FunctionSinkState : uint8_t { + Unknown, + Computing, + Sinking, + NotSinking + }; + + /// Cache whether a function body can ever reach a normal exit. + llvm::DenseMap<const FunctionDecl *, FunctionSinkState> FunctionSinkCache; + + /// The current function-like declaration being mapped. + const Decl *CurrentDecl = nullptr; + + /// The current function-like body used to build statement sink analysis. + const Stmt *CurrentBody = nullptr; + + /// Lazily built CFG state for statement sink analysis in the current body. + std::unique_ptr<CFG> CurrentCFG; + std::optional<ParentMap> CurrentParentMap; + std::optional<CFGStmtMap> CurrentCFGStmtMap; + + /// Cache whether a statement has no path to a normal exit. + llvm::DenseMap<const Stmt *, bool> StatementSinkCache; + /// Return a counter for the subtraction of \c RHS from \c LHS Counter subtractCounters(Counter LHS, Counter RHS, bool Simplify = true) { assert(!llvm::EnableSingleByteCoverage && @@ -1622,6 +1650,13 @@ struct CounterCoverageMappingBuilder } } + CurrentDecl = D; + CurrentBody = Body; + CurrentCFG.reset(); + CurrentParentMap.reset(); + CurrentCFGStmtMap.reset(); + StatementSinkCache.clear(); + propagateCounts(BodyCounter, Body, /*VisitChildren=*/!Defaulted); assert(RegionStack.empty() && "Regions entered but never exited"); @@ -1684,13 +1719,155 @@ struct CounterCoverageMappingBuilder terminateRegion(S); } + bool isFunctionInevitablySinking(const FunctionDecl *Callee) { + const FunctionDecl *Definition = nullptr; + if (!Callee || !Callee->hasBody(Definition) || !Definition) + return false; + + const FunctionDecl *Key = Definition->getCanonicalDecl(); + FunctionSinkState &State = FunctionSinkCache[Key]; + if (State == FunctionSinkState::Sinking) + return true; + if (State == FunctionSinkState::NotSinking || + State == FunctionSinkState::Computing) + return false; + + State = FunctionSinkState::Computing; + + auto blockIsImmediateSink = [&](const CFGBlock *Block) { + if (Block->hasNoReturnElement()) + return true; + + for (const CFGElement &Element : *Block) { + std::optional<CFGStmt> StmtElement = Element.getAs<CFGStmt>(); + if (!StmtElement) + continue; + + const Stmt *Stmt = StmtElement->getStmt(); + if (isa<CXXThrowExpr>(Stmt)) + return true; + + if (const auto *Call = dyn_cast<CallExpr>(Stmt)) { + QualType CalleeType = Call->getCallee()->getType(); + if (getFunctionExtInfo(*CalleeType).getNoReturn() || + isDirectCalleeInevitablySinking(Call)) + return true; + } + } + + return false; + }; + + CFG::BuildOptions Options; + std::unique_ptr<CFG> CalleeCFG = CFG::buildCFG( + Definition, Definition->getBody(), &Definition->getASTContext(), + Options); + + bool IsSinking = false; + if (CalleeCFG) { + const CFG &Cfg = *CalleeCFG; + llvm::SmallVector<const CFGBlock *, 32> DFSWorkList; + llvm::SmallPtrSet<const CFGBlock *, 32> Visited; + + const CFGBlock *StartBlock = &Cfg.getEntry(); + if (blockIsImmediateSink(StartBlock)) { + IsSinking = true; + } else { + DFSWorkList.push_back(StartBlock); + while (!DFSWorkList.empty()) { + const CFGBlock *Block = DFSWorkList.pop_back_val(); + if (!Visited.insert(Block).second) + continue; + + if (Block == &Cfg.getExit()) { + IsSinking = false; + break; + } + + for (const auto &Succ : Block->succs()) { + const CFGBlock *SuccBlock = Succ.getReachableBlock(); + if (!SuccBlock || blockIsImmediateSink(SuccBlock)) + continue; + DFSWorkList.push_back(SuccBlock); + } + } + + if (!Visited.contains(&Cfg.getExit())) + IsSinking = true; + } + } + + State = IsSinking ? FunctionSinkState::Sinking + : FunctionSinkState::NotSinking; + return IsSinking; + } + + bool isDirectCalleeInevitablySinking(const CallExpr *E) { + const auto *Callee = E->getDirectCallee(); + if (!Callee) + return false; + + // The static method body is not enough proof when runtime dispatch may + // target an override that returns normally. + if (const auto *MCE = dyn_cast<CXXMemberCallExpr>(E)) + if (const auto *ME = + dyn_cast<MemberExpr>(MCE->getCallee()->IgnoreParenImpCasts())) + if (const auto *Method = MCE->getMethodDecl(); + Method && Method->isVirtual() && + ME->performsVirtualDispatch(LangOpts)) + return false; + + return isFunctionInevitablySinking(Callee); + } + + bool ensureCurrentParentMap() { + if (!CurrentParentMap && CurrentBody) + CurrentParentMap.emplace(const_cast<Stmt *>(CurrentBody)); + return CurrentParentMap.has_value(); + } + + bool isCallInevitablySinking(const CallExpr *E) { + QualType CalleeType = E->getCallee()->getType(); + return getFunctionExtInfo(*CalleeType).getNoReturn() || + isDirectCalleeInevitablySinking(E); + } + + bool buildCurrentStmtSinkAnalysis() { + if (CurrentCFGStmtMap) + return true; + if (!CurrentDecl || !CurrentBody) + return false; + + CFG::BuildOptions Options; + CurrentCFG = CFG::buildCFG(CurrentDecl, const_cast<Stmt *>(CurrentBody), + &CurrentDecl->getASTContext(), Options); + if (!CurrentCFG) + return false; + + ensureCurrentParentMap(); + CurrentCFGStmtMap.emplace(*CurrentCFG, *CurrentParentMap); + return true; + } + + bool isStmtInevitablySinking(const Stmt *S) { + auto [It, Inserted] = StatementSinkCache.try_emplace(S, false); + if (!Inserted) + return It->second; + + if (!buildCurrentStmtSinkAnalysis()) + return false; + + if (const CFGBlock *Block = CurrentCFGStmtMap->getBlock(S)) + It->second = Block->isInevitablySinking(); + return It->second; + } + void VisitCallExpr(const CallExpr *E) { VisitStmt(E); // Terminate the region when we hit a noreturn function. // (This is helpful dealing with switch statements.) - QualType CalleeType = E->getCallee()->getType(); - if (getFunctionExtInfo(*CalleeType).getNoReturn()) + if (isCallInevitablySinking(E)) terminateRegion(E); } @@ -1699,6 +1876,7 @@ struct CounterCoverageMappingBuilder Counter ParentCount = getRegion().getCounter(); Counter BodyCount = getRegionCounter(S); + bool LoopIsSink = isStmtInevitablySinking(S); // Handle the body first so that we can get the backedge count. BreakContinueStack.push_back(BreakContinue()); @@ -1706,7 +1884,6 @@ struct CounterCoverageMappingBuilder Counter BackedgeCount = propagateCounts(BodyCount, S->getBody()); BreakContinue BC = BreakContinueStack.pop_back_val(); - bool BodyHasTerminateStmt = HasTerminateStmt; HasTerminateStmt = false; // Go back to handle the condition. @@ -1724,15 +1901,18 @@ struct CounterCoverageMappingBuilder fillGapAreaWithCount(Gap->getBegin(), Gap->getEnd(), BodyCount); Counter OutCount = addCounters(BC.BreakCount, BranchCount.Skipped); - if (!IsCounterEqual(OutCount, ParentCount)) { + if (!LoopIsSink && !IsCounterEqual(OutCount, ParentCount)) { pushRegion(OutCount); GapRegionCounter = OutCount; - if (BodyHasTerminateStmt) - HasTerminateStmt = true; + HasTerminateStmt = true; } // Create Branch Region around condition. createBranchRegion(S->getCond(), BodyCount, BranchCount.Skipped); + if (LoopIsSink) { + GapRegionCounter = Counter::getZero(); + terminateRegion(S); + } } void VisitDoStmt(const DoStmt *S) { @@ -1740,6 +1920,7 @@ struct CounterCoverageMappingBuilder Counter ParentCount = getRegion().getCounter(); Counter BodyCount = getRegionCounter(S); + bool LoopIsSink = isStmtInevitablySinking(S); BreakContinueStack.push_back(BreakContinue()); extendRegion(S->getBody()); @@ -1749,7 +1930,6 @@ struct CounterCoverageMappingBuilder BreakContinue BC = BreakContinueStack.pop_back_val(); - bool BodyHasTerminateStmt = HasTerminateStmt; HasTerminateStmt = false; Counter CondCount = addCounters(BackedgeCount, BC.ContinueCount); @@ -1759,15 +1939,18 @@ struct CounterCoverageMappingBuilder propagateCounts(CondCount, S->getCond()); Counter OutCount = addCounters(BC.BreakCount, BranchCount.Skipped); - if (!IsCounterEqual(OutCount, ParentCount)) { + if (!LoopIsSink && !IsCounterEqual(OutCount, ParentCount)) { pushRegion(OutCount); GapRegionCounter = OutCount; - if (BodyHasTerminateStmt) - HasTerminateStmt = true; + HasTerminateStmt = true; } // Create Branch Region around condition. createBranchRegion(S->getCond(), BodyCount, BranchCount.Skipped); + if (LoopIsSink) { + GapRegionCounter = Counter::getZero(); + terminateRegion(S); + } } void VisitForStmt(const ForStmt *S) { @@ -1777,6 +1960,7 @@ struct CounterCoverageMappingBuilder Counter ParentCount = getRegion().getCounter(); Counter BodyCount = getRegionCounter(S); + bool LoopIsSink = isStmtInevitablySinking(S); // The loop increment may contain a break or continue. if (S->getInc()) @@ -1788,7 +1972,6 @@ struct CounterCoverageMappingBuilder Counter BackedgeCount = propagateCounts(BodyCount, S->getBody()); BreakContinue BodyBC = BreakContinueStack.pop_back_val(); - bool BodyHasTerminateStmt = HasTerminateStmt; HasTerminateStmt = false; // The increment is essentially part of the body but it needs to include @@ -1799,6 +1982,11 @@ struct CounterCoverageMappingBuilder IncrementBC = BreakContinueStack.pop_back_val(); } + // `for (;;)` has no normal exit edge unless some path breaks out. + if (!S->getCond() && BodyBC.BreakCount.isZero() && + IncrementBC.BreakCount.isZero()) + LoopIsSink = true; + // Go back to handle the condition. Counter CondCount = addCounters( addCounters(ParentCount, BackedgeCount, BodyBC.ContinueCount), @@ -1818,15 +2006,18 @@ struct CounterCoverageMappingBuilder Counter OutCount = addCounters(BodyBC.BreakCount, IncrementBC.BreakCount, BranchCount.Skipped); - if (!IsCounterEqual(OutCount, ParentCount)) { + if (!LoopIsSink && !IsCounterEqual(OutCount, ParentCount)) { pushRegion(OutCount); GapRegionCounter = OutCount; - if (BodyHasTerminateStmt) - HasTerminateStmt = true; + HasTerminateStmt = true; } // Create Branch Region around condition. createBranchRegion(S->getCond(), BodyCount, BranchCount.Skipped); + if (LoopIsSink) { + GapRegionCounter = Counter::getZero(); + terminateRegion(S); + } } void VisitCXXForRangeStmt(const CXXForRangeStmt *S) { @@ -1838,13 +2029,13 @@ struct CounterCoverageMappingBuilder Counter ParentCount = getRegion().getCounter(); Counter BodyCount = getRegionCounter(S); + bool LoopIsSink = isStmtInevitablySinking(S); BreakContinueStack.push_back(BreakContinue()); extendRegion(S->getBody()); Counter BackedgeCount = propagateCounts(BodyCount, S->getBody()); BreakContinue BC = BreakContinueStack.pop_back_val(); - bool BodyHasTerminateStmt = HasTerminateStmt; HasTerminateStmt = false; // The body count applies to the area immediately after the range. @@ -1858,15 +2049,18 @@ struct CounterCoverageMappingBuilder assert(BranchCount.Executed.isZero() || BranchCount.Executed == BodyCount); Counter OutCount = addCounters(BC.BreakCount, BranchCount.Skipped); - if (!IsCounterEqual(OutCount, ParentCount)) { + if (!LoopIsSink && !IsCounterEqual(OutCount, ParentCount)) { pushRegion(OutCount); GapRegionCounter = OutCount; - if (BodyHasTerminateStmt) - HasTerminateStmt = true; + HasTerminateStmt = true; } // Create Branch Region around condition. createBranchRegion(S->getCond(), BodyCount, BranchCount.Skipped); + if (LoopIsSink) { + GapRegionCounter = Counter::getZero(); + terminateRegion(S); + } } void VisitObjCForCollectionStmt(const ObjCForCollectionStmt *S) { @@ -1875,6 +2069,7 @@ struct CounterCoverageMappingBuilder Counter ParentCount = getRegion().getCounter(); Counter BodyCount = getRegionCounter(S); + bool LoopIsSink = isStmtInevitablySinking(S); BreakContinueStack.push_back(BreakContinue()); extendRegion(S->getBody()); @@ -1891,9 +2086,14 @@ struct CounterCoverageMappingBuilder auto BranchCount = getBranchCounterPair(S, LoopCount); assert(BranchCount.Executed.isZero() || BranchCount.Executed == BodyCount); Counter OutCount = addCounters(BC.BreakCount, BranchCount.Skipped); - if (!IsCounterEqual(OutCount, ParentCount)) { + if (!LoopIsSink && !IsCounterEqual(OutCount, ParentCount)) { pushRegion(OutCount); GapRegionCounter = OutCount; + HasTerminateStmt = true; + } + if (LoopIsSink) { + GapRegionCounter = Counter::getZero(); + terminateRegion(S); } } diff --git a/clang/test/CoverageMapping/terminate-statements.cpp b/clang/test/CoverageMapping/terminate-statements.cpp index 93d655794ccf5..bfe6b18d1595b 100644 --- a/clang/test/CoverageMapping/terminate-statements.cpp +++ b/clang/test/CoverageMapping/terminate-statements.cpp @@ -352,6 +352,67 @@ int statementexprnoret(bool crash) { return rc; // CHECK-NOT: Gap } +void sink(); + +// CHECK-LABEL: _Z2nrv: +void nr() { + __builtin_trap(); +} + +// CHECK-LABEL: _Z15directcallnoretv: +int directcallnoret() { + nr(); // CHECK: Gap,File 0, [[@LINE]]:8 -> [[@LINE+1]]:3 = 0 + sink(); // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+1]]:11 = 0 + return 0; +} + +// CHECK-LABEL: _Z3nr2v: +void nr2() { + nr(); +} + +// CHECK-LABEL: _Z15nestedcallnoretv: +int nestedcallnoret() { + nr2(); // CHECK: Gap,File 0, [[@LINE]]:9 -> [[@LINE+1]]:3 = 0 + sink(); // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+1]]:11 = 0 + return 0; +} + +// CHECK-LABEL: _Z18infinitewhilenoretv: +int infinitewhilenoret() { + while (true) { // CHECK: Branch,File 0, [[@LINE]]:10 -> [[@LINE]]:14 = #1, 0 + } // CHECK: Gap,File 0, [[@LINE]]:4 -> [[@LINE+1]]:3 = 0 + sink(); // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+1]]:11 = 0 + return 0; +} + +// CHECK-LABEL: _Z16infinitefornoretv: +int infinitefornoret() { + for (;;) { + } // CHECK: Gap,File 0, [[@LINE]]:4 -> [[@LINE+1]]:3 = 0 + sink(); // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+1]]:11 = 0 + return 0; +} + +struct Base { + virtual void nr(); +}; + +void Base::nr() { + __builtin_trap(); +} + +struct Derived : Base { + void nr() override {} +}; + +// CHECK-LABEL: _Z16virtualcallnoretR4Base: +int virtualcallnoret(Base &b) { // CHECK: File 0, [[@LINE]]:31 -> [[@LINE+4]]:2 = #0 + b.nr(); // CHECK-NOT: Gap,File 0, [[@LINE]] + sink(); + return 0; +} + // CHECK-LABEL: _Z13do_with_breaki: int do_with_break(int n) { do { @@ -385,6 +446,10 @@ int main() { abstractcondnoret(); elsecondnoret(); statementexprnoret(false); + directcallnoret(); + nestedcallnoret(); + infinitewhilenoret(); + infinitefornoret(); do_with_break(0); return 0; } `````````` </details> https://github.com/llvm/llvm-project/pull/200190 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
