[clang][dataflow] Make optional checker work for types derived from optional. #84138
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang Author: None (martinboehme) Changes
It's not an option to simply ignore these derived classes because they get cast Full diff: https://github.com/llvm/llvm-project/pull/84138.diff 2 Files Affected:
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 1d31b22b6d25ff..73dbc027a239e9 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -64,28 +64,43 @@ static bool hasOptionalClassName(const CXXRecordDecl &RD) {
return false;
}
+static const CXXRecordDecl *getOptionalBaseClass(const CXXRecordDecl *RD) {
+ if (RD == nullptr)
+ return nullptr;
+ if (hasOptionalClassName(*RD))
+ return RD;
+
+ if (!RD->hasDefinition())
+ return nullptr;
+
+ for (const CXXBaseSpecifier &Base : RD->bases())
+ if (Base.getAccessSpecifier() == AS_public)
+ if (const CXXRecordDecl *BaseClass =
+ getOptionalBaseClass(Base.getType()->getAsCXXRecordDecl()))
+ return BaseClass;
+
+ return nullptr;
+}
+
namespace {
using namespace ::clang::ast_matchers;
using LatticeTransferState = TransferState<NoopLattice>;
-AST_MATCHER(CXXRecordDecl, hasOptionalClassNameMatcher) {
- return hasOptionalClassName(Node);
+AST_MATCHER(CXXRecordDecl, optionalOrDerivedClass) {
+ return getOptionalBaseClass(&Node) != nullptr;
}
-DeclarationMatcher optionalClass() {
- return classTemplateSpecializationDecl(
- hasOptionalClassNameMatcher(),
- hasTemplateArgument(0, refersToType(type().bind("T"))));
-}
-
-auto optionalOrAliasType() {
+auto desugarsToOptionalOrDerivedType() {
return hasUnqualifiedDesugaredType(
- recordType(hasDeclaration(optionalClass())));
+ recordType(hasDeclaration(cxxRecordDecl(optionalOrDerivedClass()))));
}
-/// Matches any of the spellings of the optional types and sugar, aliases, etc.
-auto hasOptionalType() { return hasType(optionalOrAliasType()); }
+/// Matches any of the spellings of the optional types and sugar, aliases,
+/// derived classes, etc.
+auto hasOptionalOrDerivedType() {
+ return hasType(desugarsToOptionalOrDerivedType());
+}
auto isOptionalMemberCallWithNameMatcher(
ast_matchers::internal::Matcher<NamedDecl> matcher,
@@ -93,9 +108,9 @@ auto isOptionalMemberCallWithNameMatcher(
auto Exception = unless(Ignorable ? expr(anyOf(*Ignorable, cxxThisExpr()))
: cxxThisExpr());
return cxxMemberCallExpr(
- on(expr(Exception,
- anyOf(hasOptionalType(),
- hasType(pointerType(pointee(optionalOrAliasType())))))),
+ on(expr(Exception, anyOf(hasOptionalOrDerivedType(),
+ hasType(pointerType(pointee(
+ desugarsToOptionalOrDerivedType())))))),
callee(cxxMethodDecl(matcher)));
}
@@ -104,7 +119,7 @@ auto isOptionalOperatorCallWithName(
const std::optional<StatementMatcher> &Ignorable = std::nullopt) {
return cxxOperatorCallExpr(
hasOverloadedOperatorName(operator_name),
- callee(cxxMethodDecl(ofClass(optionalClass()))),
+ callee(cxxMethodDecl(ofClass(optionalOrDerivedClass()))),
Ignorable ? callExpr(unless(hasArgument(0, *Ignorable))) : callExpr());
}
@@ -112,7 +127,7 @@ auto isMakeOptionalCall() {
return callExpr(callee(functionDecl(hasAnyName(
"std::make_optional", "base::make_optional",
"absl::make_optional", "folly::make_optional"))),
- hasOptionalType());
+ hasOptionalOrDerivedType());
}
auto nulloptTypeDecl() {
@@ -129,19 +144,19 @@ auto inPlaceClass() {
auto isOptionalNulloptConstructor() {
return cxxConstructExpr(
- hasOptionalType(),
+ hasOptionalOrDerivedType(),
hasDeclaration(cxxConstructorDecl(parameterCountIs(1),
hasParameter(0, hasNulloptType()))));
}
auto isOptionalInPlaceConstructor() {
- return cxxConstructExpr(hasOptionalType(),
+ return cxxConstructExpr(hasOptionalOrDerivedType(),
hasArgument(0, hasType(inPlaceClass())));
}
auto isOptionalValueOrConversionConstructor() {
return cxxConstructExpr(
- hasOptionalType(),
+ hasOptionalOrDerivedType(),
unless(hasDeclaration(
cxxConstructorDecl(anyOf(isCopyConstructor(), isMoveConstructor())))),
argumentCountIs(1), hasArgument(0, unless(hasNulloptType())));
@@ -150,28 +165,30 @@ auto isOptionalValueOrConversionConstructor() {
auto isOptionalValueOrConversionAssignment() {
return cxxOperatorCallExpr(
hasOverloadedOperatorName("="),
- callee(cxxMethodDecl(ofClass(optionalClass()))),
+ callee(cxxMethodDecl(ofClass(optionalOrDerivedClass()))),
unless(hasDeclaration(cxxMethodDecl(
anyOf(isCopyAssignmentOperator(), isMoveAssignmentOperator())))),
argumentCountIs(2), hasArgument(1, unless(hasNulloptType())));
}
auto isOptionalNulloptAssignment() {
- return cxxOperatorCallExpr(hasOverloadedOperatorName("="),
- callee(cxxMethodDecl(ofClass(optionalClass()))),
- argumentCountIs(2),
- hasArgument(1, hasNulloptType()));
+ return cxxOperatorCallExpr(
+ hasOverloadedOperatorName("="),
+ callee(cxxMethodDecl(ofClass(optionalOrDerivedClass()))),
+ argumentCountIs(2), hasArgument(1, hasNulloptType()));
}
auto isStdSwapCall() {
return callExpr(callee(functionDecl(hasName("std::swap"))),
- argumentCountIs(2), hasArgument(0, hasOptionalType()),
- hasArgument(1, hasOptionalType()));
+ argumentCountIs(2),
+ hasArgument(0, hasOptionalOrDerivedType()),
+ hasArgument(1, hasOptionalOrDerivedType()));
}
auto isStdForwardCall() {
return callExpr(callee(functionDecl(hasName("std::forward"))),
- argumentCountIs(1), hasArgument(0, hasOptionalType()));
+ argumentCountIs(1),
+ hasArgument(0, hasOptionalOrDerivedType()));
}
constexpr llvm::StringLiteral ValueOrCallID = "ValueOrCall";
@@ -181,10 +198,11 @@ auto isValueOrStringEmptyCall() {
return cxxMemberCallExpr(
callee(cxxMethodDecl(hasName("empty"))),
onImplicitObjectArgument(ignoringImplicit(
- cxxMemberCallExpr(on(expr(unless(cxxThisExpr()))),
- callee(cxxMethodDecl(hasName("value_or"),
- ofClass(optionalClass()))),
- hasArgument(0, stringLiteral(hasSize(0))))
+ cxxMemberCallExpr(
+ on(expr(unless(cxxThisExpr()))),
+ callee(cxxMethodDecl(hasName("value_or"),
+ ofClass(optionalOrDerivedClass()))),
+ hasArgument(0, stringLiteral(hasSize(0))))
.bind(ValueOrCallID))));
}
@@ -192,10 +210,11 @@ auto isValueOrNotEqX() {
auto ComparesToSame = [](ast_matchers::internal::Matcher<Stmt> Arg) {
return hasOperands(
ignoringImplicit(
- cxxMemberCallExpr(on(expr(unless(cxxThisExpr()))),
- callee(cxxMethodDecl(hasName("value_or"),
- ofClass(optionalClass()))),
- hasArgument(0, Arg))
+ cxxMemberCallExpr(
+ on(expr(unless(cxxThisExpr()))),
+ callee(cxxMethodDecl(hasName("value_or"),
+ ofClass(optionalOrDerivedClass()))),
+ hasArgument(0, Arg))
.bind(ValueOrCallID)),
ignoringImplicit(Arg));
};
@@ -212,8 +231,9 @@ auto isValueOrNotEqX() {
}
auto isCallReturningOptional() {
- return callExpr(hasType(qualType(anyOf(
- optionalOrAliasType(), referenceType(pointee(optionalOrAliasType()))))));
+ return callExpr(hasType(qualType(
+ anyOf(desugarsToOptionalOrDerivedType(),
+ referenceType(pointee(desugarsToOptionalOrDerivedType()))))));
}
template <typename L, typename R>
@@ -275,12 +295,9 @@ BoolValue *getHasValue(Environment &Env, RecordStorageLocation *OptionalLoc) {
return HasValueVal;
}
-/// Returns true if and only if `Type` is an optional type.
-bool isOptionalType(QualType Type) {
- if (!Type->isRecordType())
- return false;
- const CXXRecordDecl *D = Type->getAsCXXRecordDecl();
- return D != nullptr && hasOptionalClassName(*D);
+QualType valueTypeFromOptionalDecl(const CXXRecordDecl &RD) {
+ auto &CTSD = cast<ClassTemplateSpecializationDecl>(RD);
+ return CTSD.getTemplateArgs()[0].getAsType();
}
/// Returns the number of optional wrappers in `Type`.
@@ -288,15 +305,13 @@ bool isOptionalType(QualType Type) {
/// For example, if `Type` is `optional<optional<int>>`, the result of this
/// function will be 2.
int countOptionalWrappers(const ASTContext &ASTCtx, QualType Type) {
- if (!isOptionalType(Type))
+ const CXXRecordDecl *Optional =
+ getOptionalBaseClass(Type->getAsCXXRecordDecl());
+ if (Optional == nullptr)
return 0;
return 1 + countOptionalWrappers(
ASTCtx,
- cast<ClassTemplateSpecializationDecl>(Type->getAsRecordDecl())
- ->getTemplateArgs()
- .get(0)
- .getAsType()
- .getDesugaredType(ASTCtx));
+ valueTypeFromOptionalDecl(*Optional).getDesugaredType(ASTCtx));
}
StorageLocation *getLocBehindPossiblePointer(const Expr &E,
@@ -623,7 +638,7 @@ ignorableOptional(const UncheckedOptionalAccessModelOptions &Options) {
if (Options.IgnoreSmartPointerDereference) {
auto SmartPtrUse = expr(ignoringParenImpCasts(cxxOperatorCallExpr(
anyOf(hasOverloadedOperatorName("->"), hasOverloadedOperatorName("*")),
- unless(hasArgument(0, expr(hasOptionalType()))))));
+ unless(hasArgument(0, expr(hasOptionalOrDerivedType()))))));
return expr(
anyOf(SmartPtrUse, memberExpr(hasObjectExpression(SmartPtrUse))));
}
@@ -758,32 +773,35 @@ auto buildTransferMatchSwitch() {
// Comparisons (==, !=):
.CaseOfCFGStmt<CXXOperatorCallExpr>(
- isComparisonOperatorCall(hasOptionalType(), hasOptionalType()),
+ isComparisonOperatorCall(hasOptionalOrDerivedType(),
+ hasOptionalOrDerivedType()),
transferOptionalAndOptionalCmp)
.CaseOfCFGStmt<CXXOperatorCallExpr>(
- isComparisonOperatorCall(hasOptionalType(), hasNulloptType()),
+ isComparisonOperatorCall(hasOptionalOrDerivedType(),
+ hasNulloptType()),
[](const clang::CXXOperatorCallExpr *Cmp,
const MatchFinder::MatchResult &, LatticeTransferState &State) {
transferOptionalAndNulloptCmp(Cmp, Cmp->getArg(0), State.Env);
})
.CaseOfCFGStmt<CXXOperatorCallExpr>(
- isComparisonOperatorCall(hasNulloptType(), hasOptionalType()),
+ isComparisonOperatorCall(hasNulloptType(),
+ hasOptionalOrDerivedType()),
[](const clang::CXXOperatorCallExpr *Cmp,
const MatchFinder::MatchResult &, LatticeTransferState &State) {
transferOptionalAndNulloptCmp(Cmp, Cmp->getArg(1), State.Env);
})
.CaseOfCFGStmt<CXXOperatorCallExpr>(
isComparisonOperatorCall(
- hasOptionalType(),
- unless(anyOf(hasOptionalType(), hasNulloptType()))),
+ hasOptionalOrDerivedType(),
+ unless(anyOf(hasOptionalOrDerivedType(), hasNulloptType()))),
[](const clang::CXXOperatorCallExpr *Cmp,
const MatchFinder::MatchResult &, LatticeTransferState &State) {
transferOptionalAndValueCmp(Cmp, Cmp->getArg(0), State.Env);
})
.CaseOfCFGStmt<CXXOperatorCallExpr>(
isComparisonOperatorCall(
- unless(anyOf(hasOptionalType(), hasNulloptType())),
- hasOptionalType()),
+ unless(anyOf(hasOptionalOrDerivedType(), hasNulloptType())),
+ hasOptionalOrDerivedType()),
[](const clang::CXXOperatorCallExpr *Cmp,
const MatchFinder::MatchResult &, LatticeTransferState &State) {
transferOptionalAndValueCmp(Cmp, Cmp->getArg(1), State.Env);
@@ -843,13 +861,7 @@ auto buildDiagnoseMatchSwitch(
ast_matchers::DeclarationMatcher
UncheckedOptionalAccessModel::optionalClassDecl() {
- return optionalClass();
-}
-
-static QualType valueTypeFromOptionalType(QualType OptionalTy) {
- auto *CTSD =
- cast<ClassTemplateSpecializationDecl>(OptionalTy->getAsCXXRecordDecl());
- return CTSD->getTemplateArgs()[0].getAsType();
+ return cxxRecordDecl(optionalOrDerivedClass());
}
UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx,
@@ -858,9 +870,11 @@ UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx,
TransferMatchSwitch(buildTransferMatchSwitch()) {
Env.getDataflowAnalysisContext().setSyntheticFieldCallback(
[&Ctx](QualType Ty) -> llvm::StringMap<QualType> {
- if (!isOptionalType(Ty))
+ const CXXRecordDecl *Optional =
+ getOptionalBaseClass(Ty->getAsCXXRecordDecl());
+ if (Optional == nullptr)
return {};
- return {{"value", valueTypeFromOptionalType(Ty)},
+ return {{"value", valueTypeFromOptionalDecl(*Optional)},
{"has_value", Ctx.BoolTy}};
});
}
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index b6e4973fd7cb2b..c1d5b2183007ae 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -3383,6 +3383,36 @@ TEST_P(UncheckedOptionalAccessTest, LambdaCaptureStateNotPropagated) {
}
)");
}
+
+TEST_P(UncheckedOptionalAccessTest, ClassDerivedFromOptional) {
+ ExpectDiagnosticsFor(R"(
+ #include "unchecked_optional_access_test.h"
+
+ struct Derived : public $ns::$optional<int> {};
+
+ void target(Derived opt) {
+ *opt; // [[unsafe]]
+ if (opt)
+ *opt;
+ }
+ )");
+}
+
+TEST_P(UncheckedOptionalAccessTest, ClassTemplateDerivedFromOptional) {
+ ExpectDiagnosticsFor(R"(
+ #include "unchecked_optional_access_test.h"
+
+ template <class T>
+ struct Derived : public $ns::$optional<T> {};
+
+ void target(Derived<int> opt) {
+ *opt; // [[unsafe]]
+ if (opt)
+ *opt;
+ }
+ )");
+}
+
// FIXME: Add support for:
// - constructors (copy, move)
// - assignment operators (default, copy, move)
|
| return nullptr; | ||
|
|
||
| for (const CXXBaseSpecifier &Base : RD->bases()) | ||
| if (Base.getAccessSpecifier() == AS_public) |
There was a problem hiding this comment.
I wonder if we can still get in trouble with private or protected inheritance. E.g., if I had class Foo : std::optional, I could still call the methods of std::optional in Foo's methods. Could we get in trouble while analyzing those methods?
There was a problem hiding this comment.
Great point! I've removed the AS_public check (and added a test for exactly this scenario, which fails with this check present).
|
Per offline discussion, please see if we can be more targeted in our use of the generalized matching, given the potentially high cost incurred (for every relevant operation, like comparison, we'll be searching the entire type hierchy of the arguments). |
…ptional. `llvm::MaybeAlign` does this, for example. It's not an option to simply ignore these derived classes because they get cast back to the optional classes (for example, simply when calling the optional member functions), and our transfer functions will then run on those optional classes and therefore require them to be properly initialized.
martinboehme
force-pushed
the
piper_export_cl_613102184
branch
from
9648b1c
to
209aac4
Compare
Done! I'm still checking for "optional or derived type" in some places where this seemed appropriate, but many places are now back to checking only for the optional type.
|
There was a problem hiding this comment.
One high level question: is there a simpler path by which we avoid crashing on derived types but also don't support them? It seems like there is a lot of complexity required in getting this just right. Is it worth it (both in the code itself and in terms of runtime cost) to support these unusual cases? Of course, if it's necessary then the point is moot...
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
Show resolved
Hide resolved
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
Show resolved
Hide resolved
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
Show resolved
Hide resolved
| return Ty; | ||
| } | ||
|
|
||
| QualType Ty = getPublicType(Cast->getSubExpr()); |
There was a problem hiding this comment.
I don't understand this line. We're recursing, but then we throw away the result Ty based on how the loop turns out. Why not just test for this directly, after the loop and only recurse when the loop doesn't find what we want? See more comments below, but perhaps the problem is that I don't really understand what the loop is doing.
There was a problem hiding this comment.
I hope this has become clearer given the additional comments on what the loop is doing.
The idea behind doing the recursion first, then potentially throwing the result away, is that it makes the code proceed strictly along the patch from most-derived to most-base class.
Conceptually, we want to "drill down" into the getImplicitObjectArgument() to find the most-derived type, then walk back along the "cast path" towards the base until we find a point where we're no longer allowed to perform the cast. The code that was here expresses this most directly (the recursive call does the "drilling down"), but as you note, it's inefficient in the case where we need to throw away the result of the recursive call.
I've changed this to use tail recursion and have added comments that hopefully make it clear what the recursion is doing.
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
Outdated
Show resolved
Hide resolved
This is a good question. tl;dr: I think actually handling these cases instead of just ignoring them is the right tradeoff, but the situation certainly isn't clear cut. It's possible to avoid crashing on derived types but otherwise not support them. What we would need to do is call As I say, the tradeoff isn't clear-cut. To summarize: Bail out but don't crash
Correctly handle derived types
Will respond to the individual comments too, but probably not before Monday. |
This didn't have any documentation, so I had to do some experimenting in godbolt when I used this in llvm#84138, and my reviewer later also had some [questions](llvm#84138 (comment)) about this, so I figured it would be worth adding documentation.
|
Forgot to push a fixup commit -- now added. |
| for (const CXXBaseSpecifier *Base : Cast->path()) { | ||
| if (Base->getAccessSpecifier() != AS_public && !TyIsThisType) | ||
| if (Base->getAccessSpecifier() != AS_public && !CastingFromThis) | ||
| break; | ||
| Ty = Base->getType(); |
There was a problem hiding this comment.
optional super nit: save &Base instead, avoiding a call to getType() until after the loop.
There was a problem hiding this comment.
Done.
More importantly than the minor efficiency gain, I think this makes the code more self-documenting. Thanks!
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
Show resolved
Hide resolved
| @@ -129,19 +215,19 @@ auto inPlaceClass() { | |||
|
|
|||
| auto isOptionalNulloptConstructor() { | |||
| return cxxConstructExpr( | |||
| hasOptionalType(), | |||
| hasOptionalOrDerivedType(), | |||
There was a problem hiding this comment.
Here and below -- now that this matcher is more expensive, please move until after the less expensive matchers.
…derived from optional.
This didn't have any documentation, so I had to do some experimenting in godbolt when I used this in #84138, and my reviewer later also had some [questions](#84138 (comment)) about this, so I figured it would be worth adding documentation.
…ptional. (llvm#84138) `llvm::MaybeAlign` does this, for example. It's not an option to simply ignore these derived classes because they get cast back to the optional classes (for example, simply when calling the optional member functions), and our transfer functions will then run on those optional classes and therefore require them to be properly initialized.
This didn't have any documentation, so I had to do some experimenting in godbolt when I used this in llvm#84138, and my reviewer later also had some [questions](llvm#84138 (comment)) about this, so I figured it would be worth adding documentation.
llvm::MaybeAligndoes this, for example.It's not an option to simply ignore these derived classes because they get cast
back to the optional classes (for example, simply when calling the optional
member functions), and our transfer functions will then run on those optional
classes and therefore require them to be properly initialized.