[clang] [clang][dataflow] Fix crash when `operator=` result type is not destination type. (PR #90898)
https://github.com/martinboehme closed https://github.com/llvm/llvm-project/pull/90898 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Fix crash when `operator=` result type is not destination type. (PR #90898)
martinboehme wrote: CI failures appear to be unrelated. https://github.com/llvm/llvm-project/pull/90898 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Fix crash when `operator=` result type is not destination type. (PR #90898)
@@ -556,14 +556,23 @@ class TransferVisitor : public
ConstStmtVisitor {
copyRecord(*LocSrc, *LocDst, Env);
- // If the expr is a glvalue, we can reasonably assume the operator is
- // returning T& and thus we can assign it `LocDst`.
- if (S->isGLValue()) {
+ // The assignment operator can have an arbitrary return type. We model
the
+ // return value only if the return type is the same as or a base class of
+ // the destination type.
+ if (S->getType().getCanonicalType().getUnqualifiedType() !=
+ LocDst->getType().getCanonicalType().getUnqualifiedType()) {
+auto ReturnDecl = S->getType()->getAsCXXRecordDecl();
+auto DstDecl = LocDst->getType()->getAsCXXRecordDecl();
+if (ReturnDecl == nullptr || DstDecl == nullptr)
+ return;
+if (!DstDecl->isDerivedFrom(ReturnDecl))
martinboehme wrote:
I think it's consistent with our more general approach -- we treat "storage
location not set" as meaning "we have no information" [^1]. For example, if
above we discover that this isn't a copy or move assignment operator, we bail
out without doing anything.
Granted, this case is a bit different in that we have already modeled the
assignment itself (by doing the `copyRecord()`) above, but I think it's still
consistent to do this but decide we don't know how to model the return value /
location.
[^1]: Of course, a fresh storage location that isn't otherwise constrained
expresses essentially the same thing, but it's simpler to instead not assign a
storage location at all.
https://github.com/llvm/llvm-project/pull/90898
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Fix crash when `operator=` result type is not destination type. (PR #90898)
@@ -556,14 +556,23 @@ class TransferVisitor : public
ConstStmtVisitor {
copyRecord(*LocSrc, *LocDst, Env);
- // If the expr is a glvalue, we can reasonably assume the operator is
- // returning T& and thus we can assign it `LocDst`.
- if (S->isGLValue()) {
+ // The assignment operator can have an arbitrary return type. We model
the
+ // return value only if the return type is the same as or a base class of
+ // the destination type.
+ if (S->getType().getCanonicalType().getUnqualifiedType() !=
+ LocDst->getType().getCanonicalType().getUnqualifiedType()) {
+auto ReturnDecl = S->getType()->getAsCXXRecordDecl();
+auto DstDecl = LocDst->getType()->getAsCXXRecordDecl();
+if (ReturnDecl == nullptr || DstDecl == nullptr)
+ return;
+if (!DstDecl->isDerivedFrom(ReturnDecl))
Xazax-hun wrote:
Would we want to create a fresh storage location here?
https://github.com/llvm/llvm-project/pull/90898
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Fix crash when `operator=` result type is not destination type. (PR #90898)
https://github.com/Xazax-hun approved this pull request. https://github.com/llvm/llvm-project/pull/90898 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Fix crash when `operator=` result type is not destination type. (PR #90898)
https://github.com/ymand approved this pull request. https://github.com/llvm/llvm-project/pull/90898 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Fix crash when `operator=` result type is not destination type. (PR #90898)
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: None (martinboehme)
Changes
The existing code was full of comments about how we assume this is always the
case, but it's not mandated by the standard, and there is code out there that
returns a different type. So check that the result type is in fact the same as
the destination type before attempting to copy to the result.
To make sure that we don't bail out in more cases than intended, I've extended
existing tests to verify that in the common case, we do return the destination
object (by reference or value, as the case may be).
---
Full diff: https://github.com/llvm/llvm-project/pull/90898.diff
2 Files Affected:
- (modified) clang/lib/Analysis/FlowSensitive/Transfer.cpp (+15-6)
- (modified) clang/unittests/Analysis/FlowSensitive/TransferTest.cpp (+71-2)
``diff
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index fd224aeb79b151..5a57f11b000d8a 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -556,14 +556,23 @@ class TransferVisitor : public
ConstStmtVisitor {
copyRecord(*LocSrc, *LocDst, Env);
- // If the expr is a glvalue, we can reasonably assume the operator is
- // returning T& and thus we can assign it `LocDst`.
- if (S->isGLValue()) {
+ // The assignment operator can have an arbitrary return type. We model
the
+ // return value only if the return type is the same as or a base class of
+ // the destination type.
+ if (S->getType().getCanonicalType().getUnqualifiedType() !=
+ LocDst->getType().getCanonicalType().getUnqualifiedType()) {
+auto ReturnDecl = S->getType()->getAsCXXRecordDecl();
+auto DstDecl = LocDst->getType()->getAsCXXRecordDecl();
+if (ReturnDecl == nullptr || DstDecl == nullptr)
+ return;
+if (!DstDecl->isDerivedFrom(ReturnDecl))
+ return;
+ }
+
+ if (S->isGLValue())
Env.setStorageLocation(*S, *LocDst);
- } else if (S->getType()->isRecordType()) {
-// Assume that the assignment returns the assigned value.
+ else
copyRecord(*LocDst, Env.getResultObjectLocation(*S), Env);
- }
return;
}
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 301bec32c0cf1d..0b441faa6c76da 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -2228,7 +2228,7 @@ TEST(TransferTest, AssignmentOperator) {
A Foo = { 1 };
A Bar = { 2 };
// [[p1]]
- Foo = Bar;
+ A = (Foo = Bar);
// [[p2]]
Foo.Baz = 3;
// [[p3]]
@@ -2274,6 +2274,7 @@ TEST(TransferTest, AssignmentOperator) {
cast(Env2.getStorageLocation(*BarDecl));
EXPECT_TRUE(recordsEqual(*FooLoc2, *BarLoc2, Env2));
+ EXPECT_EQ((ASTCtx, Env2, "Rval"), FooLoc2);
const auto *FooBazVal2 =
cast(getFieldValue(FooLoc2, *BazDecl, Env2));
@@ -2441,7 +2442,75 @@ TEST(TransferTest, AssignmentOperatorReturnsByValue) {
// This is a crash repro.
std::string Code = R"(
struct S {
- S operator=(S&& other);
+ S operator=(const S&);
+ int i;
+};
+void target() {
+ S S1 = { 1 };
+ S S2 = { 2 };
+ S S3 = { 3 };
+ // [[before]]
+ // Test that the returned value is modeled by assigning to another value.
+ S1 = (S2 = S3);
+ (void)0;
+ // [[after]]
+}
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap> ,
+ ASTContext ) {
+const ValueDecl *S1Decl = findValueDecl(ASTCtx, "S1");
+const ValueDecl *S2Decl = findValueDecl(ASTCtx, "S2");
+const ValueDecl *S3Decl = findValueDecl(ASTCtx, "S3");
+
+const Environment =
+getEnvironmentAtAnnotation(Results, "before");
+
+EXPECT_FALSE(recordsEqual(
+*EnvBefore.get(*S1Decl),
+*EnvBefore.get(*S2Decl), EnvBefore));
+EXPECT_FALSE(recordsEqual(
+*EnvBefore.get(*S2Decl),
+*EnvBefore.get(*S3Decl), EnvBefore));
+
+const Environment =
+getEnvironmentAtAnnotation(Results, "after");
+
+EXPECT_TRUE(recordsEqual(*EnvAfter.get(*S1Decl),
+ *EnvAfter.get(*S2Decl),
+ EnvAfter));
+EXPECT_TRUE(recordsEqual(*EnvAfter.get(*S2Decl),
+ *EnvAfter.get(*S3Decl),
+ EnvAfter));
+ });
+}
+
+TEST(TransferTest, AssignmentOperatorReturnsDifferentTypeByRef) {
+ // This is a crash repro.
+ std::string Code = R"(
+struct DifferentType {};
+struct S {
+ DifferentType& operator=(const S&);
+};
+void target() {
+ S s;
+ s = S();
+ //
[clang] [clang][dataflow] Fix crash when `operator=` result type is not destination type. (PR #90898)
llvmbot wrote:
@llvm/pr-subscribers-clang-analysis
Author: None (martinboehme)
Changes
The existing code was full of comments about how we assume this is always the
case, but it's not mandated by the standard, and there is code out there that
returns a different type. So check that the result type is in fact the same as
the destination type before attempting to copy to the result.
To make sure that we don't bail out in more cases than intended, I've extended
existing tests to verify that in the common case, we do return the destination
object (by reference or value, as the case may be).
---
Full diff: https://github.com/llvm/llvm-project/pull/90898.diff
2 Files Affected:
- (modified) clang/lib/Analysis/FlowSensitive/Transfer.cpp (+15-6)
- (modified) clang/unittests/Analysis/FlowSensitive/TransferTest.cpp (+71-2)
``diff
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index fd224aeb79b151..5a57f11b000d8a 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -556,14 +556,23 @@ class TransferVisitor : public
ConstStmtVisitor {
copyRecord(*LocSrc, *LocDst, Env);
- // If the expr is a glvalue, we can reasonably assume the operator is
- // returning T& and thus we can assign it `LocDst`.
- if (S->isGLValue()) {
+ // The assignment operator can have an arbitrary return type. We model
the
+ // return value only if the return type is the same as or a base class of
+ // the destination type.
+ if (S->getType().getCanonicalType().getUnqualifiedType() !=
+ LocDst->getType().getCanonicalType().getUnqualifiedType()) {
+auto ReturnDecl = S->getType()->getAsCXXRecordDecl();
+auto DstDecl = LocDst->getType()->getAsCXXRecordDecl();
+if (ReturnDecl == nullptr || DstDecl == nullptr)
+ return;
+if (!DstDecl->isDerivedFrom(ReturnDecl))
+ return;
+ }
+
+ if (S->isGLValue())
Env.setStorageLocation(*S, *LocDst);
- } else if (S->getType()->isRecordType()) {
-// Assume that the assignment returns the assigned value.
+ else
copyRecord(*LocDst, Env.getResultObjectLocation(*S), Env);
- }
return;
}
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 301bec32c0cf1d..0b441faa6c76da 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -2228,7 +2228,7 @@ TEST(TransferTest, AssignmentOperator) {
A Foo = { 1 };
A Bar = { 2 };
// [[p1]]
- Foo = Bar;
+ A = (Foo = Bar);
// [[p2]]
Foo.Baz = 3;
// [[p3]]
@@ -2274,6 +2274,7 @@ TEST(TransferTest, AssignmentOperator) {
cast(Env2.getStorageLocation(*BarDecl));
EXPECT_TRUE(recordsEqual(*FooLoc2, *BarLoc2, Env2));
+ EXPECT_EQ((ASTCtx, Env2, "Rval"), FooLoc2);
const auto *FooBazVal2 =
cast(getFieldValue(FooLoc2, *BazDecl, Env2));
@@ -2441,7 +2442,75 @@ TEST(TransferTest, AssignmentOperatorReturnsByValue) {
// This is a crash repro.
std::string Code = R"(
struct S {
- S operator=(S&& other);
+ S operator=(const S&);
+ int i;
+};
+void target() {
+ S S1 = { 1 };
+ S S2 = { 2 };
+ S S3 = { 3 };
+ // [[before]]
+ // Test that the returned value is modeled by assigning to another value.
+ S1 = (S2 = S3);
+ (void)0;
+ // [[after]]
+}
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap> ,
+ ASTContext ) {
+const ValueDecl *S1Decl = findValueDecl(ASTCtx, "S1");
+const ValueDecl *S2Decl = findValueDecl(ASTCtx, "S2");
+const ValueDecl *S3Decl = findValueDecl(ASTCtx, "S3");
+
+const Environment =
+getEnvironmentAtAnnotation(Results, "before");
+
+EXPECT_FALSE(recordsEqual(
+*EnvBefore.get(*S1Decl),
+*EnvBefore.get(*S2Decl), EnvBefore));
+EXPECT_FALSE(recordsEqual(
+*EnvBefore.get(*S2Decl),
+*EnvBefore.get(*S3Decl), EnvBefore));
+
+const Environment =
+getEnvironmentAtAnnotation(Results, "after");
+
+EXPECT_TRUE(recordsEqual(*EnvAfter.get(*S1Decl),
+ *EnvAfter.get(*S2Decl),
+ EnvAfter));
+EXPECT_TRUE(recordsEqual(*EnvAfter.get(*S2Decl),
+ *EnvAfter.get(*S3Decl),
+ EnvAfter));
+ });
+}
+
+TEST(TransferTest, AssignmentOperatorReturnsDifferentTypeByRef) {
+ // This is a crash repro.
+ std::string Code = R"(
+struct DifferentType {};
+struct S {
+ DifferentType& operator=(const S&);
+};
+void target() {
+ S s;
+ s = S();
+
[clang] [clang][dataflow] Fix crash when `operator=` result type is not destination type. (PR #90898)
https://github.com/martinboehme created
https://github.com/llvm/llvm-project/pull/90898
The existing code was full of comments about how we assume this is always the
case, but it's not mandated by the standard, and there is code out there that
returns a different type. So check that the result type is in fact the same as
the destination type before attempting to copy to the result.
To make sure that we don't bail out in more cases than intended, I've extended
existing tests to verify that in the common case, we do return the destination
object (by reference or value, as the case may be).
>From 6760984c6c243afc92c888e901bc0840e180c4d5 Mon Sep 17 00:00:00 2001
From: Martin Braenne
Date: Thu, 2 May 2024 19:50:32 +
Subject: [PATCH] [clang][dataflow] Fix crash when `operator=` result type is
not destination type.
The existing code was full of comments about how we assume this is always the
case, but it's not mandated by the standard, and there is code out there that
returns a different type. So check that the result type is in fact the same as
the destination type before attempting to copy to the result.
To make sure that we don't bail out in more cases than intended, I've extended
existing tests to verify that in the common case, we do return the destination
object (by reference or value, as the case may be).
---
clang/lib/Analysis/FlowSensitive/Transfer.cpp | 21 --
.../Analysis/FlowSensitive/TransferTest.cpp | 73 ++-
2 files changed, 86 insertions(+), 8 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index fd224aeb79b151..5a57f11b000d8a 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -556,14 +556,23 @@ class TransferVisitor : public
ConstStmtVisitor {
copyRecord(*LocSrc, *LocDst, Env);
- // If the expr is a glvalue, we can reasonably assume the operator is
- // returning T& and thus we can assign it `LocDst`.
- if (S->isGLValue()) {
+ // The assignment operator can have an arbitrary return type. We model
the
+ // return value only if the return type is the same as or a base class of
+ // the destination type.
+ if (S->getType().getCanonicalType().getUnqualifiedType() !=
+ LocDst->getType().getCanonicalType().getUnqualifiedType()) {
+auto ReturnDecl = S->getType()->getAsCXXRecordDecl();
+auto DstDecl = LocDst->getType()->getAsCXXRecordDecl();
+if (ReturnDecl == nullptr || DstDecl == nullptr)
+ return;
+if (!DstDecl->isDerivedFrom(ReturnDecl))
+ return;
+ }
+
+ if (S->isGLValue())
Env.setStorageLocation(*S, *LocDst);
- } else if (S->getType()->isRecordType()) {
-// Assume that the assignment returns the assigned value.
+ else
copyRecord(*LocDst, Env.getResultObjectLocation(*S), Env);
- }
return;
}
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 301bec32c0cf1d..0b441faa6c76da 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -2228,7 +2228,7 @@ TEST(TransferTest, AssignmentOperator) {
A Foo = { 1 };
A Bar = { 2 };
// [[p1]]
- Foo = Bar;
+ A = (Foo = Bar);
// [[p2]]
Foo.Baz = 3;
// [[p3]]
@@ -2274,6 +2274,7 @@ TEST(TransferTest, AssignmentOperator) {
cast(Env2.getStorageLocation(*BarDecl));
EXPECT_TRUE(recordsEqual(*FooLoc2, *BarLoc2, Env2));
+ EXPECT_EQ((ASTCtx, Env2, "Rval"), FooLoc2);
const auto *FooBazVal2 =
cast(getFieldValue(FooLoc2, *BazDecl, Env2));
@@ -2441,7 +2442,75 @@ TEST(TransferTest, AssignmentOperatorReturnsByValue) {
// This is a crash repro.
std::string Code = R"(
struct S {
- S operator=(S&& other);
+ S operator=(const S&);
+ int i;
+};
+void target() {
+ S S1 = { 1 };
+ S S2 = { 2 };
+ S S3 = { 3 };
+ // [[before]]
+ // Test that the returned value is modeled by assigning to another value.
+ S1 = (S2 = S3);
+ (void)0;
+ // [[after]]
+}
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap> ,
+ ASTContext ) {
+const ValueDecl *S1Decl = findValueDecl(ASTCtx, "S1");
+const ValueDecl *S2Decl = findValueDecl(ASTCtx, "S2");
+const ValueDecl *S3Decl = findValueDecl(ASTCtx, "S3");
+
+const Environment =
+getEnvironmentAtAnnotation(Results, "before");
+
+EXPECT_FALSE(recordsEqual(
+*EnvBefore.get(*S1Decl),
+*EnvBefore.get(*S2Decl), EnvBefore));
+EXPECT_FALSE(recordsEqual(
+*EnvBefore.get(*S2Decl),
+*EnvBefore.get(*S3Decl), EnvBefore));
+
+const Environment =
+
