This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
ymandel marked 3 inline comments as done.
Closed by commit rGa184a0d8aae6: [clang][dataflow] Add support for disabling
warnings on smart pointers. (authored by ymandel).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122143/new/
https://reviews.llvm.org/D122143
Files:
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===================================================================
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -1219,7 +1219,9 @@
llvm::Error Error = checkDataflow<UncheckedOptionalAccessModel>(
SourceCode, FuncMatcher,
[](ASTContext &Ctx, Environment &) {
- return UncheckedOptionalAccessModel(Ctx);
+ return UncheckedOptionalAccessModel(
+ Ctx, UncheckedOptionalAccessModelOptions{
+ /*IgnoreSmartPointerDereference=*/true});
},
[&MatchesLatticeChecks](
llvm::ArrayRef<std::pair<
@@ -2004,6 +2006,34 @@
Pair("check-4", "unsafe: input.cc:13:7")));
}
+TEST_P(UncheckedOptionalAccessTest, UniquePtrToStructWithOptionalField) {
+ // We suppress diagnostics for values reachable from smart pointers (other
+ // than `optional` itself).
+ ExpectLatticeChecksFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ template <typename T>
+ struct smart_ptr {
+ T& operator*() &;
+ T* operator->();
+ };
+
+ struct Foo {
+ $ns::$optional<int> opt;
+ };
+
+ void target() {
+ smart_ptr<Foo> foo;
+ *foo->opt;
+ /*[[check-1]]*/
+ *(*foo).opt;
+ /*[[check-2]]*/
+ }
+ )",
+ UnorderedElementsAre(Pair("check-1", "safe"), Pair("check-2", "safe")));
+}
+
// FIXME: Add support for:
// - constructors (copy, move)
// - assignment operators (default, copy, move)
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -45,15 +45,23 @@
auto hasOptionalType() { return hasType(optionalClass()); }
-auto isOptionalMemberCallWithName(llvm::StringRef MemberName) {
+auto isOptionalMemberCallWithName(
+ llvm::StringRef MemberName,
+ llvm::Optional<StatementMatcher> Ignorable = llvm::None) {
+ auto Exception = unless(Ignorable ? expr(anyOf(*Ignorable, cxxThisExpr()))
+ : cxxThisExpr());
return cxxMemberCallExpr(
- on(expr(unless(cxxThisExpr()))),
+ on(expr(Exception)),
callee(cxxMethodDecl(hasName(MemberName), ofClass(optionalClass()))));
}
-auto isOptionalOperatorCallWithName(llvm::StringRef OperatorName) {
- return cxxOperatorCallExpr(hasOverloadedOperatorName(OperatorName),
- callee(cxxMethodDecl(ofClass(optionalClass()))));
+auto isOptionalOperatorCallWithName(
+ llvm::StringRef operator_name,
+ llvm::Optional<StatementMatcher> Ignorable = llvm::None) {
+ return cxxOperatorCallExpr(
+ hasOverloadedOperatorName(operator_name),
+ callee(cxxMethodDecl(ofClass(optionalClass()))),
+ Ignorable ? callExpr(unless(hasArgument(0, *Ignorable))) : callExpr());
}
auto isMakeOptionalCall() {
@@ -333,10 +341,22 @@
transferSwap(*OptionalLoc1, *OptionalLoc2, State);
}
-auto buildTransferMatchSwitch() {
+llvm::Optional<StatementMatcher>
+ignorableOptional(const UncheckedOptionalAccessModelOptions &Options) {
+ if (Options.IgnoreSmartPointerDereference)
+ return memberExpr(hasObjectExpression(ignoringParenImpCasts(
+ cxxOperatorCallExpr(anyOf(hasOverloadedOperatorName("->"),
+ hasOverloadedOperatorName("*")),
+ unless(hasArgument(0, expr(hasOptionalType())))))));
+ return llvm::None;
+}
+
+auto buildTransferMatchSwitch(
+ const UncheckedOptionalAccessModelOptions &Options) {
// FIXME: Evaluate the efficiency of matchers. If using matchers results in a
// lot of duplicated work (e.g. string comparisons), consider providing APIs
// that avoid it through memoization.
+ auto IgnorableOptional = ignorableOptional(Options);
return MatchSwitchBuilder<LatticeTransferState>()
// Attach a symbolic "has_value" state to optional values that we see for
// the first time.
@@ -371,19 +391,20 @@
// optional::value
.CaseOf<CXXMemberCallExpr>(
- isOptionalMemberCallWithName("value"),
+ isOptionalMemberCallWithName("value", IgnorableOptional),
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
transferUnwrapCall(E, E->getImplicitObjectArgument(), State);
})
// optional::operator*, optional::operator->
- .CaseOf<CallExpr>(expr(anyOf(isOptionalOperatorCallWithName("*"),
- isOptionalOperatorCallWithName("->"))),
- [](const CallExpr *E, const MatchFinder::MatchResult &,
- LatticeTransferState &State) {
- transferUnwrapCall(E, E->getArg(0), State);
- })
+ .CaseOf<CallExpr>(
+ expr(anyOf(isOptionalOperatorCallWithName("*", IgnorableOptional),
+ isOptionalOperatorCallWithName("->", IgnorableOptional))),
+ [](const CallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+ transferUnwrapCall(E, E->getArg(0), State);
+ })
// optional::has_value
.CaseOf<CXXMemberCallExpr>(isOptionalMemberCallWithName("has_value"),
@@ -423,10 +444,11 @@
} // namespace
-UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx)
+UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(
+ ASTContext &Ctx, UncheckedOptionalAccessModelOptions Options)
: DataflowAnalysis<UncheckedOptionalAccessModel, SourceLocationsLattice>(
Ctx),
- TransferMatchSwitch(buildTransferMatchSwitch()) {}
+ TransferMatchSwitch(buildTransferMatchSwitch(Options)) {}
void UncheckedOptionalAccessModel::transfer(const Stmt *S,
SourceLocationsLattice &L,
Index: clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
===================================================================
--- clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
+++ clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
@@ -24,6 +24,18 @@
namespace clang {
namespace dataflow {
+// FIXME: Explore using an allowlist-approach, where constructs supported by the
+// analysis are always enabled and additional constructs are enabled through the
+// `Options`.
+struct UncheckedOptionalAccessModelOptions {
+ /// Ignore optionals reachable through overloaded `operator*` or `operator->`
+ /// (other than those of the optional type itself). The analysis does not
+ /// equate the results of such calls, so it can't identify when their results
+ /// are used safely (across calls), resulting in false positives in all such
+ /// cases. Note: this option does not cover access through `operator[]`.
+ bool IgnoreSmartPointerDereference = false;
+};
+
/// Dataflow analysis that discovers unsafe accesses of optional values and
/// adds the respective source locations to the lattice.
///
@@ -34,7 +46,8 @@
: public DataflowAnalysis<UncheckedOptionalAccessModel,
SourceLocationsLattice> {
public:
- explicit UncheckedOptionalAccessModel(ASTContext &AstContext);
+ UncheckedOptionalAccessModel(
+ ASTContext &AstContext, UncheckedOptionalAccessModelOptions Options = {});
static SourceLocationsLattice initialElement() {
return SourceLocationsLattice();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
