vsavchenko updated this revision to Diff 331186.
vsavchenko added a comment.
Replace manual memory management of IPData with std::unique_ptr
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98688/new/
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,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<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);
}
+// 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()));
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
@@ -14,6 +14,7 @@
#define LLVM_CLANG_SEMA_ANALYSISBASEDWARNINGS_H
#include "llvm/ADT/DenseMap.h"
+#include <memory>
namespace clang {
@@ -47,6 +48,9 @@
Sema &S;
Policy DefaultPolicy;
+ class InterProceduralData;
+ std::unique_ptr<InterProceduralData> IPData;
+
enum VisitFlag { NotVisited = 0, Visited = 1, Pending = 2 };
llvm::DenseMap<const FunctionDecl*, VisitFlag> 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
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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
