vsavchenko created this revision. vsavchenko added a reviewer: NoQ. Herald added a subscriber: Charusso. vsavchenko requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits.
This patch introduces a very simple inter-procedural analysis between blocks and enclosing functions. We always analyze blocks first (analysis is done as part of semantic analysis that goes side-by-side with the parsing process), and at the moment of reporting we don't know how that block will be actually used. This patch introduces new logic delaying reports of the "never called" warnings on blocks. If we are not sure that the block will be called exactly once, we shouldn't warn our users about that. Double calls, however, don't require such delays. While analyzing the enclosing function, we can actually decide what we should do with those warnings. Additionally, as a side effect, we can be more confident about blocks in such context and can treat them not as escapes, but as direct calls. rdar://74090107 Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D98688 Files: clang/include/clang/Analysis/Analyses/CalledOnceCheck.h clang/include/clang/Sema/AnalysisBasedWarnings.h clang/lib/Analysis/CalledOnceCheck.cpp clang/lib/Sema/AnalysisBasedWarnings.cpp clang/test/SemaObjC/warn-called-once.m
Index: clang/test/SemaObjC/warn-called-once.m =================================================================== --- clang/test/SemaObjC/warn-called-once.m +++ clang/test/SemaObjC/warn-called-once.m @@ -31,6 +31,16 @@ @class NSString, Protocol; extern void NSLog(NSString *format, ...); +typedef int group_t; +typedef struct dispatch_queue_s *dispatch_queue_t; +typedef void (^dispatch_block_t)(void); +extern dispatch_queue_t queue; + +void dispatch_group_async(dispatch_queue_t queue, + group_t group, + dispatch_block_t block); +void dispatch_async(dispatch_queue_t queue, dispatch_block_t block); + void escape(void (^callback)(void)); void escape_void(void *); void indirect_call(void (^callback)(void) CALLED_ONCE); @@ -225,11 +235,11 @@ } void block_call_1(void (^callback)(void) CALLED_ONCE) { - indirect_call(^{ - callback(); - }); - callback(); - // no-warning + indirect_call( // expected-note{{previous call is here}} + ^{ + callback(); + }); + callback(); // expected-warning{{'callback' parameter marked 'called_once' is called twice}} } void block_call_2(void (^callback)(void) CALLED_ONCE) { @@ -255,7 +265,7 @@ // expected-warning@-1{{'callback' parameter marked 'called_once' is never used when taking false branch}} escape(callback); } - }(); + }(); // no-warning } void block_call_5(void (^outer)(void) CALLED_ONCE) { @@ -273,6 +283,32 @@ outer(); // expected-warning{{'outer' parameter marked 'called_once' is called twice}} } +void block_dispatch_call(int cond, void (^callback)(void) CALLED_ONCE) { + dispatch_async(queue, ^{ + if (cond) // expected-warning{{'callback' parameter marked 'called_once' is never called when taking false branch}} + callback(); + }); +} + +void block_escape_call_1(int cond, void (^callback)(void) CALLED_ONCE) { + escape_void((__bridge void *)^{ + if (cond) { + // no-warning + callback(); + } + }); +} + +void block_escape_call_2(int cond, void (^callback)(void) CALLED_ONCE) { + escape_void((__bridge void *)^{ + if (cond) { + callback(); // expected-note{{previous call is here}} + } + // Double call can still be reported. + callback(); // expected-warning{{'callback' parameter marked 'called_once' is called twice}} + }); +} + void never_called_one_exit(int cond, void (^callback)(void) CALLED_ONCE) { if (!cond) // expected-warning{{'callback' parameter marked 'called_once' is never called when taking true branch}} return; @@ -822,11 +858,10 @@ - (void)block_call_1:(void (^)(void))CALLED_ONCE callback { // We consider captures by blocks as escapes - [self indirect_call:(^{ + [self indirect_call:(^{ // expected-note{{previous call is here}} callback(); })]; - callback(); - // no-warning + callback(); // expected-warning{{'callback' parameter marked 'called_once' is called twice}} } - (void)block_call_2:(int)cond callback:(void (^)(void))CALLED_ONCE callback { Index: clang/lib/Sema/AnalysisBasedWarnings.cpp =================================================================== --- clang/lib/Sema/AnalysisBasedWarnings.cpp +++ clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -1506,6 +1506,25 @@ } } +namespace clang { +namespace { +typedef SmallVector<PartialDiagnosticAt, 1> OptionalNotes; +typedef std::pair<PartialDiagnosticAt, OptionalNotes> DelayedDiag; +typedef std::list<DelayedDiag> DiagList; + +struct SortDiagBySourceLocation { + SourceManager &SM; + SortDiagBySourceLocation(SourceManager &SM) : SM(SM) {} + + bool operator()(const DelayedDiag &left, const DelayedDiag &right) { + // Although this call will be slow, this is only called when outputting + // multiple warnings. + return SM.isBeforeInTranslationUnit(left.first.first, right.first.first); + } +}; +} // anonymous namespace +} // namespace clang + namespace { class UninitValsDiagReporter : public UninitVariablesHandler { Sema &S; @@ -1626,9 +1645,35 @@ } }; +/// Inter-procedural data for the called-once checker. +class CalledOnceInterProceduralData { +public: + // Add the delayed warning for the given block. + void addDelayedWarning(const BlockDecl *Block, + PartialDiagnosticAt &&Warning) { + DelayedBlockWarnings[Block].emplace_back(std::move(Warning)); + } + // Report all of the warnings we've gathered for the given block. + void flushWarnings(const BlockDecl *Block, Sema &S) { + for (const PartialDiagnosticAt &Delayed : DelayedBlockWarnings[Block]) + S.Diag(Delayed.first, Delayed.second); + + discardWarnings(Block); + } + // Discard all of the warnings we've gathered for the given block. + void discardWarnings(const BlockDecl *Block) { + DelayedBlockWarnings.erase(Block); + } + +private: + using DelayedDiagnostics = SmallVector<PartialDiagnosticAt, 2>; + llvm::DenseMap<const BlockDecl *, DelayedDiagnostics> DelayedBlockWarnings; +}; + class CalledOnceCheckReporter : public CalledOnceCheckHandler { public: - CalledOnceCheckReporter(Sema &S) : S(S) {} + CalledOnceCheckReporter(Sema &S, CalledOnceInterProceduralData &Data) + : S(S), Data(Data) {} void handleDoubleCall(const ParmVarDecl *Parameter, const Expr *Call, const Expr *PrevCall, bool IsCompletionHandler, bool Poised) override { @@ -1649,14 +1694,24 @@ << Parameter << /* Captured */ false; } - void handleNeverCalled(const ParmVarDecl *Parameter, const Stmt *Where, - NeverCalledReason Reason, bool IsCalledDirectly, + void handleNeverCalled(const ParmVarDecl *Parameter, const Decl *Function, + const Stmt *Where, NeverCalledReason Reason, + bool IsCalledDirectly, bool IsCompletionHandler) override { auto DiagToReport = IsCompletionHandler ? diag::warn_completion_handler_never_called_when : diag::warn_called_once_never_called_when; - S.Diag(Where->getBeginLoc(), DiagToReport) - << Parameter << IsCalledDirectly << (unsigned)Reason; + PartialDiagnosticAt Warning(Where->getBeginLoc(), S.PDiag(DiagToReport) + << Parameter + << IsCalledDirectly + << (unsigned)Reason); + + if (const auto *Block = dyn_cast<BlockDecl>(Function)) { + // We shouldn't report these warnings on blocks immediately + Data.addDelayedWarning(Block, std::move(Warning)); + } else { + S.Diag(Warning.first, Warning.second); + } } void handleCapturedNeverCalled(const ParmVarDecl *Parameter, @@ -1669,8 +1724,18 @@ << Parameter << /* Captured */ true; } + void + handleBlockThatIsGuaranteedToBeCalledOnce(const BlockDecl *Block) override { + Data.flushWarnings(Block, S); + } + + void handleBlockWithNoGuarantees(const BlockDecl *Block) override { + Data.discardWarnings(Block); + } + private: Sema &S; + CalledOnceInterProceduralData &Data; }; constexpr unsigned CalledOnceWarnings[] = { @@ -1703,25 +1768,6 @@ } } // anonymous namespace -namespace clang { -namespace { -typedef SmallVector<PartialDiagnosticAt, 1> OptionalNotes; -typedef std::pair<PartialDiagnosticAt, OptionalNotes> DelayedDiag; -typedef std::list<DelayedDiag> DiagList; - -struct SortDiagBySourceLocation { - SourceManager &SM; - SortDiagBySourceLocation(SourceManager &SM) : SM(SM) {} - - bool operator()(const DelayedDiag &left, const DelayedDiag &right) { - // Although this call will be slow, this is only called when outputting - // multiple warnings. - return SM.isBeforeInTranslationUnit(left.first.first, right.first.first); - } -}; -} // anonymous namespace -} // namespace clang - //===----------------------------------------------------------------------===// // -Wthread-safety //===----------------------------------------------------------------------===// @@ -2107,54 +2153,66 @@ // warnings on a function, method, or block. //===----------------------------------------------------------------------===// -clang::sema::AnalysisBasedWarnings::Policy::Policy() { +sema::AnalysisBasedWarnings::Policy::Policy() { enableCheckFallThrough = 1; enableCheckUnreachable = 0; enableThreadSafetyAnalysis = 0; enableConsumedAnalysis = 0; } +/// InterProceduralData aims to be a storage of whatever data should be passed +/// between analyses of different functions. +/// +/// At the moment, its primary goal is to make the information gathered during +/// the analysis of the blocks available during the analysis of the enclosing +/// function. This is important due to the fact that blocks are analyzed before +/// the enclosed function is even parsed fully, so it is not viable to access +/// anything in the outer scope while analyzing the block. On the other hand, +/// re-building CFG for blocks and re-analyzing them when we do have all the +/// information (i.e. during the analysis of the enclosing function) seems to be +/// ill-designed. +class sema::AnalysisBasedWarnings::InterProceduralData { +public: + // It is important to analyze blocks within functions because it's a very + // common pattern to capture completion handler parameters by blocks. + CalledOnceInterProceduralData CalledOnceData; +}; + static unsigned isEnabled(DiagnosticsEngine &D, unsigned diag) { return (unsigned)!D.isIgnored(diag, SourceLocation()); } -clang::sema::AnalysisBasedWarnings::AnalysisBasedWarnings(Sema &s) - : S(s), - NumFunctionsAnalyzed(0), - NumFunctionsWithBadCFGs(0), - NumCFGBlocks(0), - MaxCFGBlocksPerFunction(0), - NumUninitAnalysisFunctions(0), - NumUninitAnalysisVariables(0), - MaxUninitAnalysisVariablesPerFunction(0), - NumUninitAnalysisBlockVisits(0), - MaxUninitAnalysisBlockVisitsPerFunction(0) { +sema::AnalysisBasedWarnings::AnalysisBasedWarnings(Sema &s) + : S(s), IPData(new InterProceduralData), NumFunctionsAnalyzed(0), + NumFunctionsWithBadCFGs(0), NumCFGBlocks(0), MaxCFGBlocksPerFunction(0), + NumUninitAnalysisFunctions(0), NumUninitAnalysisVariables(0), + MaxUninitAnalysisVariablesPerFunction(0), NumUninitAnalysisBlockVisits(0), + MaxUninitAnalysisBlockVisitsPerFunction(0) { using namespace diag; DiagnosticsEngine &D = S.getDiagnostics(); DefaultPolicy.enableCheckUnreachable = - isEnabled(D, warn_unreachable) || - isEnabled(D, warn_unreachable_break) || - isEnabled(D, warn_unreachable_return) || - isEnabled(D, warn_unreachable_loop_increment); + isEnabled(D, warn_unreachable) || isEnabled(D, warn_unreachable_break) || + isEnabled(D, warn_unreachable_return) || + isEnabled(D, warn_unreachable_loop_increment); - DefaultPolicy.enableThreadSafetyAnalysis = - isEnabled(D, warn_double_lock); + DefaultPolicy.enableThreadSafetyAnalysis = isEnabled(D, warn_double_lock); DefaultPolicy.enableConsumedAnalysis = - isEnabled(D, warn_use_in_invalid_state); + isEnabled(D, warn_use_in_invalid_state); } +sema::AnalysisBasedWarnings::~AnalysisBasedWarnings() { delete IPData; } + static void flushDiagnostics(Sema &S, const sema::FunctionScopeInfo *fscope) { for (const auto &D : fscope->PossiblyUnreachableDiags) S.Diag(D.Loc, D.PD); } -void clang::sema:: -AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P, - sema::FunctionScopeInfo *fscope, - const Decl *D, QualType BlockType) { +void clang::sema::AnalysisBasedWarnings::IssueWarnings( + sema::AnalysisBasedWarnings::Policy P, sema::FunctionScopeInfo *fscope, + const Decl *D, QualType BlockType) { // We avoid doing analysis-based warnings when there are errors for // two reasons: @@ -2346,7 +2404,7 @@ if (S.getLangOpts().ObjC && shouldAnalyzeCalledOnceParameters(Diags, D->getBeginLoc())) { if (AC.getCFG()) { - CalledOnceCheckReporter Reporter(S); + CalledOnceCheckReporter Reporter(S, IPData->CalledOnceData); checkCalledOnceParameters( AC, Reporter, shouldAnalyzeCalledOnceConventions(Diags, D->getBeginLoc())); Index: clang/lib/Analysis/CalledOnceCheck.cpp =================================================================== --- clang/lib/Analysis/CalledOnceCheck.cpp +++ clang/lib/Analysis/CalledOnceCheck.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "clang/Analysis/Analyses/CalledOnceCheck.h" +#include "clang/AST/ASTContext.h" #include "clang/AST/Attr.h" #include "clang/AST/Decl.h" #include "clang/AST/DeclBase.h" @@ -57,6 +58,20 @@ constexpr llvm::StringLiteral CONVENTIONAL_CONDITIONS[] = { "error", "cancel", "shouldCall", "done", "OK", "success"}; +struct KnownCalledOnceParameter { + llvm::StringLiteral FunctionName; + unsigned ParamIndex; +}; +constexpr KnownCalledOnceParameter KNOWN_CALLED_ONCE_PARAMETERS[] = { + {"dispatch_async", 1}, + {"dispatch_async_and_wait", 1}, + {"dispatch_after", 2}, + {"dispatch_sync", 1}, + {"dispatch_once", 1}, + {"dispatch_barrier_async", 1}, + {"dispatch_barrier_async_and_wait", 1}, + {"dispatch_barrier_sync", 1}}; + class ParameterStatus { public: // Status kind is basically the main part of parameter's status. @@ -929,9 +944,9 @@ "Block should have at least two successors at this point"); if (auto Clarification = NotCalledClarifier::clarify(Parent, Succ)) { const ParmVarDecl *Parameter = getParameter(Index); - Handler.handleNeverCalled(Parameter, Clarification->Location, - Clarification->Reason, !IsEscape, - !isExplicitlyMarked(Parameter)); + Handler.handleNeverCalled( + Parameter, AC.getDecl(), Clarification->Location, + Clarification->Reason, !IsEscape, !isExplicitlyMarked(Parameter)); } } } @@ -1091,6 +1106,91 @@ return false; } + // Return a call site where the block is called exactly once or null otherwise + const Expr *getBlockGuaraneedCallSite(const BlockExpr *Block) const { + ParentMap &PM = AC.getParentMap(); + + // We don't want to track the block through assignments and so on, instead + // we simply see how the block used and if it's used directly in a call, + // we decide based on call to what it is. + // + // In order to do this, we go up the parents of the block looking for + // a call or a message expressions. These might not be immediate parents + // of the actual block expression due to casts and parens, so we skip them. + for (const Stmt *Prev = Block, *Current = PM.getParent(Block); + Current != nullptr; Prev = Current, Current = PM.getParent(Current)) { + // Skip no-op (for our case) operations. + if (isa<CastExpr>(Current) || isa<ParenExpr>(Current)) + continue; + + // At this point, Prev represents our block as an immediate child of the + // call. + if (const auto *Call = dyn_cast<CallExpr>(Current)) { + // It might be the call of the Block itself... + if (Call->getCallee() == Prev) + return Call; + + // ...or it can be an indirect call of the block. + return shouldBlockArgumentBeCalledOnce(Call, Prev) ? Call : nullptr; + } + if (const auto *Message = dyn_cast<ObjCMessageExpr>(Current)) { + return shouldBlockArgumentBeCalledOnce(Message, Prev) ? Message + : nullptr; + } + + break; + } + + return nullptr; + } + + template <class CallLikeExpr> + bool shouldBlockArgumentBeCalledOnce(const CallLikeExpr *CallOrMessage, + const Stmt *BlockArgument) const { + // CallExpr::arguments does not interact nicely with llvm::enumerate. + llvm::ArrayRef<const Expr *> Arguments = llvm::makeArrayRef( + CallOrMessage->getArgs(), CallOrMessage->getNumArgs()); + + for (const auto &Argument : llvm::enumerate(Arguments)) { + if (Argument.value() == BlockArgument) { + return shouldBlockArgumentBeCalledOnce(CallOrMessage, Argument.index()); + } + } + + return false; + } + + bool shouldBlockArgumentBeCalledOnce(const CallExpr *Call, + unsigned ParamIndex) const { + const FunctionDecl *Function = Call->getDirectCallee(); + return shouldBlockArgumentBeCalledOnce(Function, ParamIndex) || + shouldBeCalledOnce(Call, ParamIndex); + } + + bool shouldBlockArgumentBeCalledOnce(const ObjCMessageExpr *Message, + unsigned ParamIndex) const { + // At the moment, we don't have any Obj-C methods we want to specifically + // check in here. + return shouldBeCalledOnce(Message, ParamIndex); + } + + static bool shouldBlockArgumentBeCalledOnce(const FunctionDecl *Function, + unsigned ParamIndex) { + // There is a list of important API functions that while not following + // conventions nor being directly annotated, still guarantee that the + // callback parameter will be called exactly once. + // + // Here we check if this is the case. + return Function && + llvm::any_of(KNOWN_CALLED_ONCE_PARAMETERS, + [Function, ParamIndex]( + const KnownCalledOnceParameter &Reference) { + return Reference.FunctionName == + Function->getName() && + Reference.ParamIndex == ParamIndex; + }); + } + /// Return true if the analyzed function is actually a default implementation /// of the method that has to be overriden. /// @@ -1437,17 +1537,44 @@ } void VisitBlockExpr(const BlockExpr *Block) { + // Block expressions are tricky. It is a very common practice to capture + // completion handlers by blocks and use them there. + // For this reason, it is important to analyze blocks and report warnings + // for completion handler misuse in blocks. + // + // However, it can be quite difficult to track how the block itself is being + // used. The full precise anlysis of that will be similar to alias analysis + // for completion handlers and can be too heavyweight for a compile-time + // diagnostic. Instead, we judge about the immediate use of the block. + // + // Here, we try to find a call expression where we know due to conventions, + // annotations, or other reasons that the block is called once and only + // once. + const Expr *CalledOnceCallSite = getBlockGuaraneedCallSite(Block); + + // We need to report this information to the handler because in the + // situation when we know that the block is called exactly once, we can be + // stricter in terms of reported diagnostics. + if (CalledOnceCallSite) { + Handler.handleBlockThatIsGuaranteedToBeCalledOnce(Block->getBlockDecl()); + } else { + Handler.handleBlockWithNoGuarantees(Block->getBlockDecl()); + } + for (const auto &Capture : Block->getBlockDecl()->captures()) { - // If a block captures a tracked parameter, it should be - // considered escaped. - // On one hand, blocks that do that should definitely call it on - // every path. However, it is not guaranteed that the block - // itself gets called whenever it gets created. - // - // Because we don't want to track blocks and whether they get called, - // we consider such parameters simply escaped. if (const auto *Param = dyn_cast<ParmVarDecl>(Capture.getVariable())) { - checkEscapee(*Param); + if (auto Index = getIndex(*Param)) { + if (CalledOnceCallSite) { + // The call site of a block can be considered a call site of the + // captured parameter we track. + processCallFor(*Index, CalledOnceCallSite); + } else { + // We still should consider this block as an escape for parameter, + // if we don't know about its call site or the number of time it + // can be invoked. + processEscapeFor(*Index); + } + } } } } Index: clang/include/clang/Sema/AnalysisBasedWarnings.h =================================================================== --- clang/include/clang/Sema/AnalysisBasedWarnings.h +++ clang/include/clang/Sema/AnalysisBasedWarnings.h @@ -47,6 +47,9 @@ Sema &S; Policy DefaultPolicy; + class InterProceduralData; + InterProceduralData *IPData; + enum VisitFlag { NotVisited = 0, Visited = 1, Pending = 2 }; llvm::DenseMap<const FunctionDecl*, VisitFlag> VisitedFD; @@ -88,6 +91,7 @@ public: AnalysisBasedWarnings(Sema &s); + ~AnalysisBasedWarnings(); void IssueWarnings(Policy P, FunctionScopeInfo *fscope, const Decl *D, QualType BlockType); @@ -97,6 +101,7 @@ void PrintStats() const; }; -}} // end namespace clang::sema +} // namespace sema +} // namespace clang #endif Index: clang/include/clang/Analysis/Analyses/CalledOnceCheck.h =================================================================== --- clang/include/clang/Analysis/Analyses/CalledOnceCheck.h +++ clang/include/clang/Analysis/Analyses/CalledOnceCheck.h @@ -17,6 +17,7 @@ namespace clang { class AnalysisDeclContext; +class BlockDecl; class CFG; class Decl; class DeclContext; @@ -79,6 +80,7 @@ /// the path containing the call and not containing the call. This helps us /// to pinpoint a bad path for the user. /// \param Parameter -- parameter that should be called once. + /// \param Function -- function declaration where the problem occured. /// \param Where -- the least common ancestor statement. /// \param Reason -- a reason describing the path without a call. /// \param IsCalledDirectly -- true, if parameter actually gets called on @@ -86,9 +88,22 @@ /// collection, passed as a parameter, etc.). /// \param IsCompletionHandler -- true, if parameter is a completion handler. virtual void handleNeverCalled(const ParmVarDecl *Parameter, - const Stmt *Where, NeverCalledReason Reason, + const Decl *Function, const Stmt *Where, + NeverCalledReason Reason, bool IsCalledDirectly, bool IsCompletionHandler) {} + + /// Called when the block is guaranteed to be called exactly once. + /// It means that we can be stricter with what we report on that block. + /// \param Block -- block declaration that is known to be called exactly once. + virtual void + handleBlockThatIsGuaranteedToBeCalledOnce(const BlockDecl *Block) {} + + /// Called when the block has no guarantees about how many times it can get + /// called. + /// It means that we should be more lenient with reporting warnings in it. + /// \param Block -- block declaration in question. + virtual void handleBlockWithNoGuarantees(const BlockDecl *Block) {} }; /// Check given CFG for 'called once' parameter violations.
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits