Szelethus created this revision.
Szelethus added reviewers: NoQ, steakhal, xazax.hun, martong, ASDenysPetrov.
Herald added subscribers: manas, gamesh411, dkrupp, donat.nagy,
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware.
Szelethus requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
As of now, we have two interfaces to for defining signatures:
`StdLibraryFunctionsChecker::Signature` and `CallDescription`. An example for
how `Signature`s are used can be seen here:
addToFunctionSummaryMap(
"isprint", Signature(ArgTypes{IntTy}, RetType{IntTy}),
Summary(EvalCallAsPure)
.Case({ArgumentCondition(0U, WithinRange, Range(32, 126)),
ReturnValueCondition(OutOfRange, SingleValue(0))})
.Case({ArgumentCondition(0U, OutOfRange, Range(32, 126)),
ReturnValueCondition(WithinRange, SingleValue(0))}));
The name of the function is searched for in translation unit's identifier
table, then the `Signature` is matched against each decl `Decl` with the same
name. Ideally, this yields a `FunctionDecl`, which is mapped to its `Summary`.
This works well for C functions, but doesn't support C++ at all.
`CallDescription` emerged with a strong emphasis on recognizing C++ functions.
The common example brough up is `std::string`, which in some standard library
implementations is actually called something like `std::__cxx11::basic_string`,
but not in others. Matching this can be a nightmare for checker developers. For
this reason, `CallDescription`s can be defined like this:
InnerPointerChecker()
: AppendFn({"std", "basic_string", "append"}),
AssignFn({"std", "basic_string", "assign"}),
AddressofFn({"std", "addressof"}),
ClearFn({"std", "basic_string", "clear"}),
CStrFn({"std", "basic_string", "c_str"}), DataFn({"std", "data"}, 1),
DataMemberFn({"std", "basic_string", "data"}),
EraseFn({"std", "basic_string", "erase"}),
InsertFn({"std", "basic_string", "insert"}),
PopBackFn({"std", "basic_string", "pop_back"}),
PushBackFn({"std", "basic_string", "push_back"}),
ReplaceFn({"std", "basic_string", "replace"}),
ReserveFn({"std", "basic_string", "reserve"}),
ResizeFn({"std", "basic_string", "resize"}),
ShrinkToFitFn({"std", "basic_string", "shrink_to_fit"}),
SwapFn({"std", "basic_string", "swap"}) {}
Any identifier which matches //at least// these identifiers are considered a
match (which sometimes leads to incorrect matching, e.g. D81745
<https://reviews.llvm.org/D81745>).
`CallDescription`s are (usually) not used for digging up `FunctionDecl`s from
the translation unit, but rather during symbolic execution to check in a
pre/post call event whether the called function matches the `CallDescription`:
bool InnerPointerChecker::isInnerPointerAccessFunction(
const CallEvent &Call) const {
return matchesAny(Call, CStrFn, DataFn, DataMemberFn);
}
Most of the new checkers implementing pre/post condition checks on functions
now use `CallDescriptionMap` or `CallDescriptionSet`. Its up to debate whether
the newer `Signature` approach is better, but its not obvious, and converting
from one to the other may be non-trivial as well.
---
Now, onto this patch. Since `CallDescription`s can only be matched against
`CallEvent`s that are created during symbolic execution, it was not possible to
use it in syntactic-only contexts. For example, even though
`InnerPointerChecker` can check with its set of `CallDescription`s whether a
function call is interested during analysis, its unable to check without hassle
whether a non-analyzer piece of code also calls such a function.
The patch adds the ability to use `CallDescription`s in syntactic contexts as
well. While we already have that in `Signature`, we still want to leverage the
ability to use dynamic information when we have it (function pointers, for
example). This could be done with `Signature` as well
(`StdLibraryFunctionsChecker` does it), but it makes it even less of a drop-in
replacement.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D119004
Files:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
clang/lib/StaticAnalyzer/Core/CallDescription.cpp
clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
Index: clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
===================================================================
--- clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
+++ clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
@@ -6,11 +6,18 @@
//
//===----------------------------------------------------------------------===//
+#include "CheckerRegistration.h"
#include "Reusables.h"
#include "clang/AST/ExprCXX.h"
+#include "clang/Analysis/PathDiagnostic.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
+#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
#include "clang/Tooling/Tooling.h"
#include "gtest/gtest.h"
#include <type_traits>
@@ -40,6 +47,7 @@
// If it's a function we expected to find, remember that we've found it.
if (Result && *Result)
++Found;
+
return Result;
}
@@ -489,6 +497,77 @@
"}"));
}
+//===----------------------------------------------------------------------===//
+// Testing through a checker interface.
+//
+// Above, the static analyzer isn't run properly, only the bare minimum to
+// create CallEvents. This causes CallEvents through function pointers to not
+// refer to the pointee function, but this works fine if we run
+// AnalysisASTConsumer.
+//===----------------------------------------------------------------------===//
+
+class CallDescChecker
+ : public Checker<check::PreCall, check::PreStmt<CallExpr>> {
+ CallDescriptionSet Set = {{"bar", 0}};
+
+public:
+ void checkPreCall(const CallEvent &Call, CheckerContext &C) const {
+ if (Set.contains(Call)) {
+ C.getBugReporter().EmitBasicReport(
+ Call.getDecl(), this, "CallEvent match", categories::LogicError,
+ "CallEvent match",
+ PathDiagnosticLocation{Call.getDecl(), C.getSourceManager()});
+ }
+ }
+
+ void checkPreStmt(const CallExpr *CE, CheckerContext &C) const {
+ if (Set.containsImprecise(*CE)) {
+ C.getBugReporter().EmitBasicReport(
+ CE->getCalleeDecl(), this, "CallExpr match", categories::LogicError,
+ "CallExpr match",
+ PathDiagnosticLocation{CE->getCalleeDecl(), C.getSourceManager()});
+ }
+ }
+};
+
+void addCallDescChecker(AnalysisASTConsumer &AnalysisConsumer,
+ AnalyzerOptions &AnOpts) {
+ AnOpts.CheckersAndPackages = {{"test.CallDescChecker", true}};
+ AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
+ Registry.addChecker<CallDescChecker>("test.CallDescChecker", "Description",
+ "");
+ });
+}
+
+TEST(CallDescription, CheckCallExprMatching) {
+ // Imprecise matching shouldn't catch the call to bar, because its obscured
+ // by a function pointer.
+ constexpr StringRef FnPtrCode = R"code(
+ void bar();
+ void foo() {
+ void (*fnptr)() = bar;
+ fnptr();
+ })code";
+ std::string Diags;
+ EXPECT_TRUE(runCheckerOnCode<addCallDescChecker>(FnPtrCode.str(), Diags,
+ /*OnlyEmitWarnings*/ true));
+ EXPECT_EQ("test.CallDescChecker: CallEvent match\n", Diags);
+
+ // This should be caught properly by imprecise matching, as the call is done
+ // purely through syntactic means.
+ constexpr StringRef Code = R"code(
+ void bar();
+ void foo() {
+ bar();
+ })code";
+ Diags.clear();
+ EXPECT_TRUE(runCheckerOnCode<addCallDescChecker>(Code.str(), Diags,
+ /*OnlyEmitWarnings*/ true));
+ EXPECT_EQ("test.CallDescChecker: CallEvent match\n"
+ "test.CallDescChecker: CallExpr match\n",
+ Diags);
+}
+
} // namespace
} // namespace ento
} // namespace clang
Index: clang/lib/StaticAnalyzer/Core/CallDescription.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/CallDescription.cpp
+++ clang/lib/StaticAnalyzer/Core/CallDescription.cpp
@@ -13,6 +13,7 @@
//===----------------------------------------------------------------------===//
#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/AST/Decl.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
#include "llvm/ADT/ArrayRef.h"
@@ -61,15 +62,32 @@
if (!FD)
return false;
+ return matchesImpl(FD, Call.getNumArgs(), Call.parameters().size());
+}
+
+bool ento::CallDescription::matchesImprecise(const CallExpr &CE) const {
+ const auto *FD = dyn_cast_or_null<FunctionDecl>(CE.getCalleeDecl());
+ if (!FD)
+ return false;
+
+ return matchesImpl(FD, CE.getNumArgs(), FD->param_size());
+}
+
+bool ento::CallDescription::matchesImpl(const FunctionDecl *Callee,
+ size_t ArgCount,
+ size_t ParamCount) const {
+ const auto *FD = Callee;
+ if (!FD)
+ return false;
+
if (Flags & CDF_MaybeBuiltin) {
return CheckerContext::isCLibraryFunction(FD, getFunctionName()) &&
- (!RequiredArgs || *RequiredArgs <= Call.getNumArgs()) &&
- (!RequiredParams || *RequiredParams <= Call.parameters().size());
+ (!RequiredArgs || *RequiredArgs <= ArgCount) &&
+ (!RequiredParams || *RequiredParams <= ParamCount);
}
if (!II.hasValue()) {
- II = &Call.getState()->getStateManager().getContext().Idents.get(
- getFunctionName());
+ II = &FD->getASTContext().Idents.get(getFunctionName());
}
const auto MatchNameOnly = [](const CallDescription &CD,
@@ -86,11 +104,11 @@
};
const auto ExactMatchArgAndParamCounts =
- [](const CallEvent &Call, const CallDescription &CD) -> bool {
- const bool ArgsMatch =
- !CD.RequiredArgs || *CD.RequiredArgs == Call.getNumArgs();
+ [](size_t ArgCount, size_t ParamCount,
+ const CallDescription &CD) -> bool {
+ const bool ArgsMatch = !CD.RequiredArgs || *CD.RequiredArgs == ArgCount;
const bool ParamsMatch =
- !CD.RequiredParams || *CD.RequiredParams == Call.parameters().size();
+ !CD.RequiredParams || *CD.RequiredParams == ParamCount;
return ArgsMatch && ParamsMatch;
};
@@ -122,7 +140,7 @@
};
// Let's start matching...
- if (!ExactMatchArgAndParamCounts(Call, *this))
+ if (!ExactMatchArgAndParamCounts(ArgCount, ParamCount, *this))
return false;
if (!MatchNameOnly(*this, FD))
@@ -144,3 +162,7 @@
bool ento::CallDescriptionSet::contains(const CallEvent &Call) const {
return static_cast<bool>(Impl.lookup(Call));
}
+
+bool ento::CallDescriptionSet::containsImprecise(const CallExpr &CE) const {
+ return static_cast<bool>(Impl.lookupImprecise(CE));
+}
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
===================================================================
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
@@ -99,6 +99,28 @@
/// calls.
bool matches(const CallEvent &Call) const;
+ /// Returns true if the CallEvent is a call to a function that matches
+ /// the CallDescription.
+ ///
+ /// When available, always prefer matching with a CallEvent! This function
+ /// exists only when that is not available, for example, when _only_
+ /// syntactic check is done on a piece of code.
+ ///
+ /// Also, StdLibraryFunctionsChecker::Signature is likely a better candicade
+ /// for syntactic only matching if you are writing a new checker. This is
+ /// handy if a CallDescriptionMap is already there.
+ ///
+ /// The function is imprecise because CallEvent understands the precise
+ /// argument count better (see comments for CallEvent::getNumArgs), may
+ /// know the called function if it was called through a function pointer,
+ /// and other information not available syntactically.
+ bool matchesImprecise(const CallExpr &CE) const;
+
+private:
+ bool matchesImpl(const FunctionDecl *Callee, size_t ArgCount,
+ size_t ParamCount) const;
+
+public:
/// Returns true whether the CallEvent matches on any of the CallDescriptions
/// supplied.
///
@@ -156,6 +178,18 @@
return nullptr;
}
+
+ /// ALWAYS prefer lookup with a CallEvent, when available. See comments above
+ /// CallDescription::matchesImprecise.
+ LLVM_NODISCARD const T *lookupImprecise(const CallExpr &Call) const {
+ // Slow path: linear lookup.
+ // TODO: Implement some sort of fast path.
+ for (const std::pair<CallDescription, T> &I : LinearMap)
+ if (I.first.matchesImprecise(Call))
+ return &I.second;
+
+ return nullptr;
+ }
};
/// An immutable set of CallDescriptions.
@@ -171,6 +205,7 @@
CallDescriptionSet &operator=(const CallDescription &) = delete;
LLVM_NODISCARD bool contains(const CallEvent &Call) const;
+ LLVM_NODISCARD bool containsImprecise(const CallExpr &CE) const;
};
} // namespace ento
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
