diff --git a/clang/include/clang/Analysis/Analyses/CalledOnceCheck.h b/clang/include/clang/Analysis/Analyses/CalledOnceCheck.h --- a/clang/include/clang/Analysis/Analyses/CalledOnceCheck.h +++ b/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. diff --git a/clang/include/clang/Sema/AnalysisBasedWarnings.h b/clang/include/clang/Sema/AnalysisBasedWarnings.h --- a/clang/include/clang/Sema/AnalysisBasedWarnings.h +++ b/clang/include/clang/Sema/AnalysisBasedWarnings.h @@ -14,6 +14,7 @@ #define LLVM_CLANG_SEMA_ANALYSISBASEDWARNINGS_H #include "llvm/ADT/DenseMap.h" +#include namespace clang { @@ -47,6 +48,9 @@ Sema &S; Policy DefaultPolicy; + class InterProceduralData; + std::unique_ptr IPData; + enum VisitFlag { NotVisited = 0, Visited = 1, Pending = 2 }; llvm::DenseMap VisitedFD; @@ -88,6 +92,7 @@ public: AnalysisBasedWarnings(Sema &s); + ~AnalysisBasedWarnings(); void IssueWarnings(Policy P, FunctionScopeInfo *fscope, const Decl *D, QualType BlockType); @@ -97,6 +102,7 @@ void PrintStats() const; }; -}} // end namespace clang::sema +} // namespace sema +} // namespace clang #endif diff --git a/clang/lib/Analysis/CalledOnceCheck.cpp b/clang/lib/Analysis/CalledOnceCheck.cpp --- a/clang/lib/Analysis/CalledOnceCheck.cpp +++ b/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(Current) || isa(Current)) + continue; + + // At this point, Prev represents our block as an immediate child of the + // call. + if (const auto *Call = dyn_cast(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(Current)) { + return shouldBlockArgumentBeCalledOnce(Message, Prev) ? Message + : nullptr; + } + + break; + } + + return nullptr; + } + + template + bool shouldBlockArgumentBeCalledOnce(const CallLikeExpr *CallOrMessage, + const Stmt *BlockArgument) const { + // CallExpr::arguments does not interact nicely with llvm::enumerate. + llvm::ArrayRef 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(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); + } + } } } } diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -1506,6 +1506,25 @@ } } +namespace clang { +namespace { +typedef SmallVector OptionalNotes; +typedef std::pair DelayedDiag; +typedef std::list 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; + llvm::DenseMap 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(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 OptionalNotes; -typedef std::pair DelayedDiag; -typedef std::list 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,68 @@ // 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(std::make_unique()), + 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); } +// We need this here for unique_ptr with forward declared class. +sema::AnalysisBasedWarnings::~AnalysisBasedWarnings() = default; + 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 +2406,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())); diff --git a/clang/test/SemaObjC/warn-called-once.m b/clang/test/SemaObjC/warn-called-once.m --- a/clang/test/SemaObjC/warn-called-once.m +++ b/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 {