[clang] [analyzer] Use dynamic type when invalidating by a member function call (PR #111138)
Balazs Benics via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 24 01:48:11 PDT 2024
https://github.com/steakhal updated https://github.com/llvm/llvm-project/pull/111138
>From d3e8f1fefc06e4bf52adc128b286d3c259aa3151 Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Fri, 4 Oct 2024 13:46:09 +0200
Subject: [PATCH 1/3] [analyzer] Use dynamic type when invalidating by a member
function call
When instantiating "callable<T>", the "class CallableType" nested type
will only have a declaration in the copy for the instantiation - because
it's not refered to directly by any other code that would need a
complete definition.
However, in the past, when conservative eval calling member function,
we took the static type of the "this" expr, and looked up the
CXXRecordDecl it refered to to see if it has any mutable members (to
decide if it needs to refine invalidation or not).
Unfortunately, that query needs a definition, and it asserts otherwise,
thus we crashed.
To fix this, we should consult the dynamic type of the object, because
that will have the definition.
I anyways added a check for "hasDefinition" just to be on the safe side.
Fixes #77378
---
.../Core/PathSensitive/CallEvent.h | 4 ++
clang/lib/StaticAnalyzer/Core/CallEvent.cpp | 55 ++++++++++---------
clang/test/Analysis/const-method-call.cpp | 46 ++++++++++++++++
3 files changed, 80 insertions(+), 25 deletions(-)
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
index 549c864dc91ef2..209f40b5b9f4c5 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
@@ -690,6 +690,10 @@ class CXXInstanceCall : public AnyFunctionCall {
ValueList &Values,
RegionAndSymbolInvalidationTraits *ETraits) const override;
+ /// Returns the decl refered to by the "dynamic type" of the current object.
+ /// This may be null.
+ const CXXRecordDecl *getDeclForDynamicType() const;
+
public:
/// Returns the expression representing the implicit 'this' object.
virtual const Expr *getCXXThisExpr() const { return nullptr; }
diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
index 1efac6ac1c57f4..2c04b42629b756 100644
--- a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -711,18 +711,17 @@ void CXXInstanceCall::getExtraInvalidatedValues(
if (const auto *D = cast_or_null<CXXMethodDecl>(getDecl())) {
if (!D->isConst())
return;
- // Get the record decl for the class of 'This'. D->getParent() may return a
- // base class decl, rather than the class of the instance which needs to be
- // checked for mutable fields.
- // TODO: We might as well look at the dynamic type of the object.
- const Expr *Ex = getCXXThisExpr()->IgnoreParenBaseCasts();
- QualType T = Ex->getType();
- if (T->isPointerType()) // Arrow or implicit-this syntax?
- T = T->getPointeeType();
- const CXXRecordDecl *ParentRecord = T->getAsCXXRecordDecl();
- assert(ParentRecord);
+
+ // Get the record decl for the class of 'This'. D->getParent() may return
+ // a base class decl, rather than the class of the instance which needs to
+ // be checked for mutable fields.
+ const CXXRecordDecl *ParentRecord = getDeclForDynamicType();
+ if (!ParentRecord || !ParentRecord->hasDefinition())
+ return;
+
if (ParentRecord->hasMutableFields())
return;
+
// Preserve CXXThis.
const MemRegion *ThisRegion = ThisVal.getAsRegion();
if (!ThisRegion)
@@ -748,6 +747,22 @@ SVal CXXInstanceCall::getCXXThisVal() const {
return ThisVal;
}
+const CXXRecordDecl *CXXInstanceCall::getDeclForDynamicType() const {
+ const MemRegion *R = getCXXThisVal().getAsRegion();
+ if (!R)
+ return nullptr;
+
+ DynamicTypeInfo DynType = getDynamicTypeInfo(getState(), R);
+ if (!DynType.isValid())
+ return nullptr;
+
+ QualType Ty = DynType.getType()->getPointeeType();
+ if (Ty.isNull())
+ return nullptr;
+
+ return Ty->getAsCXXRecordDecl();
+}
+
RuntimeDefinition CXXInstanceCall::getRuntimeDefinition() const {
// Do we have a decl at all?
const Decl *D = getDecl();
@@ -759,21 +774,7 @@ RuntimeDefinition CXXInstanceCall::getRuntimeDefinition() const {
if (!MD->isVirtual())
return AnyFunctionCall::getRuntimeDefinition();
- // Do we know the implicit 'this' object being called?
- const MemRegion *R = getCXXThisVal().getAsRegion();
- if (!R)
- return {};
-
- // Do we know anything about the type of 'this'?
- DynamicTypeInfo DynType = getDynamicTypeInfo(getState(), R);
- if (!DynType.isValid())
- return {};
-
- // Is the type a C++ class? (This is mostly a defensive check.)
- QualType RegionType = DynType.getType()->getPointeeType();
- assert(!RegionType.isNull() && "DynamicTypeInfo should always be a pointer.");
-
- const CXXRecordDecl *RD = RegionType->getAsCXXRecordDecl();
+ const CXXRecordDecl *RD = getDeclForDynamicType();
if (!RD || !RD->hasDefinition())
return {};
@@ -797,6 +798,10 @@ RuntimeDefinition CXXInstanceCall::getRuntimeDefinition() const {
return {};
}
+ const MemRegion *R = getCXXThisVal().getAsRegion();
+ DynamicTypeInfo DynType = getDynamicTypeInfo(getState(), R);
+ assert(DynType.isValid() && "ensured by getDeclForDynamicType()");
+
// Does the decl that we found have an implementation?
const FunctionDecl *Definition;
if (!Result->hasBody(Definition)) {
diff --git a/clang/test/Analysis/const-method-call.cpp b/clang/test/Analysis/const-method-call.cpp
index 8e1fd3b125f0e2..7da7ca5554a23e 100644
--- a/clang/test/Analysis/const-method-call.cpp
+++ b/clang/test/Analysis/const-method-call.cpp
@@ -271,3 +271,49 @@ void checkThatConstMethodCallDoesInvalidateObjectForCircularReferences() {
// FIXME: Should be UNKNOWN.
clang_analyzer_eval(t.x); // expected-warning{{TRUE}}
}
+
+namespace gh77378 {
+template <typename Signature> class callable;
+
+template <typename R> class callable<R()> {
+ struct CallableType {
+ bool operator()();
+ };
+ using MethodType = R (CallableType::*)();
+ CallableType *object_{nullptr};
+ MethodType method_;
+
+public:
+ callable() = default;
+
+ template <typename T>
+ constexpr callable(const T &obj)
+ : object_{reinterpret_cast<CallableType *>(&const_cast<T &>(obj))},
+ method_{reinterpret_cast<MethodType>(
+ static_cast<bool (T::*)() const>(&T::operator()))} {}
+
+ constexpr bool const_method() const {
+ return (object_->*(method_))();
+ }
+
+ callable call() const & {
+ static const auto L = [this]() {
+ while (true) {
+ // This should not crash when conservative eval calling the member function
+ // when it unwinds the call stack due to exhausting the budget or reaching
+ // the inlining limit.
+ if (this->const_method()) {
+ break;
+ }
+ }
+ return true;
+ };
+ return L;
+ }
+};
+
+void entry() {
+ callable<bool()>{}.call().const_method();
+ // expected-warning at -1 {{Address of stack memory associated with temporary object of type 'callable<bool ()>' is still referred to by the static variable 'L' upon returning to the caller. This will be a dangling reference}}
+}
+} // namespace gh77378
>From 02264948ed9d5d54c9df91ee268a306b87417192 Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Thu, 24 Oct 2024 10:37:05 +0200
Subject: [PATCH 2/3] Simplify code, add assert
---
clang/lib/StaticAnalyzer/Core/CallEvent.cpp | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
index 2c04b42629b756..aac16faf2ed5a8 100644
--- a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -756,11 +756,8 @@ const CXXRecordDecl *CXXInstanceCall::getDeclForDynamicType() const {
if (!DynType.isValid())
return nullptr;
- QualType Ty = DynType.getType()->getPointeeType();
- if (Ty.isNull())
- return nullptr;
-
- return Ty->getAsCXXRecordDecl();
+ assert(!DynType.getType()->getPointeeType().isNull());
+ return DynType.getType()->getPointeeCXXRecordDecl();
}
RuntimeDefinition CXXInstanceCall::getRuntimeDefinition() const {
>From a2420d708414189f02619c053ef3c1374374f739 Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Thu, 24 Oct 2024 10:44:45 +0200
Subject: [PATCH 3/3] Fix double lookup
---
.../Core/PathSensitive/CallEvent.h | 7 +++---
clang/lib/StaticAnalyzer/Core/CallEvent.cpp | 25 +++++++++----------
2 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
index 209f40b5b9f4c5..4552df2a2a31e6 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
@@ -690,9 +690,10 @@ class CXXInstanceCall : public AnyFunctionCall {
ValueList &Values,
RegionAndSymbolInvalidationTraits *ETraits) const override;
- /// Returns the decl refered to by the "dynamic type" of the current object.
- /// This may be null.
- const CXXRecordDecl *getDeclForDynamicType() const;
+ /// Returns the decl refered to by the "dynamic type" of the current object
+ /// and if the class can be a sub-class or not.
+ /// If the Pointer is null, the flag has no meaning.
+ std::pair<const CXXRecordDecl *, bool> getDeclForDynamicType() const;
public:
/// Returns the expression representing the implicit 'this' object.
diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
index aac16faf2ed5a8..270cce1d3a650e 100644
--- a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -715,7 +715,7 @@ void CXXInstanceCall::getExtraInvalidatedValues(
// Get the record decl for the class of 'This'. D->getParent() may return
// a base class decl, rather than the class of the instance which needs to
// be checked for mutable fields.
- const CXXRecordDecl *ParentRecord = getDeclForDynamicType();
+ const CXXRecordDecl *ParentRecord = getDeclForDynamicType().first;
if (!ParentRecord || !ParentRecord->hasDefinition())
return;
@@ -747,17 +747,19 @@ SVal CXXInstanceCall::getCXXThisVal() const {
return ThisVal;
}
-const CXXRecordDecl *CXXInstanceCall::getDeclForDynamicType() const {
+std::pair<const CXXRecordDecl *, bool>
+CXXInstanceCall::getDeclForDynamicType() const {
const MemRegion *R = getCXXThisVal().getAsRegion();
if (!R)
- return nullptr;
+ return {};
DynamicTypeInfo DynType = getDynamicTypeInfo(getState(), R);
if (!DynType.isValid())
- return nullptr;
+ return {};
assert(!DynType.getType()->getPointeeType().isNull());
- return DynType.getType()->getPointeeCXXRecordDecl();
+ return {DynType.getType()->getPointeeCXXRecordDecl(),
+ DynType.canBeASubClass()};
}
RuntimeDefinition CXXInstanceCall::getRuntimeDefinition() const {
@@ -771,7 +773,7 @@ RuntimeDefinition CXXInstanceCall::getRuntimeDefinition() const {
if (!MD->isVirtual())
return AnyFunctionCall::getRuntimeDefinition();
- const CXXRecordDecl *RD = getDeclForDynamicType();
+ auto [RD, CanBeSubClass] = getDeclForDynamicType();
if (!RD || !RD->hasDefinition())
return {};
@@ -795,14 +797,10 @@ RuntimeDefinition CXXInstanceCall::getRuntimeDefinition() const {
return {};
}
- const MemRegion *R = getCXXThisVal().getAsRegion();
- DynamicTypeInfo DynType = getDynamicTypeInfo(getState(), R);
- assert(DynType.isValid() && "ensured by getDeclForDynamicType()");
-
// Does the decl that we found have an implementation?
const FunctionDecl *Definition;
if (!Result->hasBody(Definition)) {
- if (!DynType.canBeASubClass())
+ if (!CanBeSubClass)
return AnyFunctionCall::getRuntimeDefinition();
return {};
}
@@ -810,8 +808,9 @@ RuntimeDefinition CXXInstanceCall::getRuntimeDefinition() const {
// We found a definition. If we're not sure that this devirtualization is
// actually what will happen at runtime, make sure to provide the region so
// that ExprEngine can decide what to do with it.
- if (DynType.canBeASubClass())
- return RuntimeDefinition(Definition, R->StripCasts());
+ if (CanBeSubClass)
+ return RuntimeDefinition(Definition,
+ getCXXThisVal().getAsRegion()->StripCasts());
return RuntimeDefinition(Definition, /*DispatchRegion=*/nullptr);
}
More information about the cfe-commits
mailing list