ymandel updated this revision to Diff 417032.
ymandel added a comment.
Addressed comments.
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
@@ -1210,7 +1210,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<
@@ -1911,6 +1913,34 @@
UnorderedElementsAre(Pair("check", "unsafe: input.cc:6: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 unique_ptr {
+ T& operator*() &;
+ T* operator->();
+ };
+
+ struct Member {
+ $ns::$optional<int> opt;
+ };
+
+ void target() {
+ unique_ptr<Member> 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() {
@@ -283,10 +291,22 @@
transferAssignment(E, State.Env.getBoolLiteralValue(false), 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.
@@ -321,19 +341,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"),
@@ -366,10 +387,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,15 @@
namespace clang {
namespace dataflow {
+struct UncheckedOptionalAccessModelOptions {
+ /// Ignore optionals reachable by derefencing a smart pointer (other than
+ /// optional itself). The analysis does not track smart-pointer pointees, so
+ /// it can't identify when optionals resulting from dereferencing such
+ /// pointers are used safely, 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 +43,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
