[clang] [clang][dataflow] Correctly treat empty initializer lists for unions. (PR #82986)
https://github.com/martinboehme closed https://github.com/llvm/llvm-project/pull/82986 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Correctly treat empty initializer lists for unions. (PR #82986)
@@ -2392,14 +2392,88 @@ TEST(TransferTest, InitListExprAsUnion) {
} F;
public:
- constexpr target() : F{nullptr} {}
+ constexpr target() : F{nullptr} {
+int *null = nullptr;
+F.b; // Make sure we reference 'b' so it is modeled.
+// [[p]]
+ }
};
)cc";
runDataflow(
Code,
[](const llvm::StringMap> ,
ASTContext ) {
-// Just verify that it doesn't crash.
+const Environment = getEnvironmentAtAnnotation(Results, "p");
+
+auto = getFieldLoc(
+*Env.getThisPointeeStorageLocation(), "F", ASTCtx);
+auto *AVal = cast(getFieldValue(, "a", ASTCtx,
Env));
+EXPECT_EQ(AVal, (ASTCtx, Env, "null"));
+EXPECT_EQ(getFieldValue(, "b", ASTCtx, Env), nullptr);
+ });
+}
+
+TEST(TransferTest, EmptyInitListExprForUnion) {
+ // This is a crash repro.
+ std::string Code = R"cc(
+class target {
+ union {
+int *a;
+bool *b;
+ } F;
+
+ public:
+ constexpr target() : F{} {
+int *null = nullptr;
+F.b; // Make sure we reference 'b' so it is modeled.
+// [[p]]
+ }
+};
+ )cc";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap> ,
+ ASTContext ) {
+const Environment = getEnvironmentAtAnnotation(Results, "p");
+
+auto = getFieldLoc(
+*Env.getThisPointeeStorageLocation(), "F", ASTCtx);
+auto *AVal = cast(getFieldValue(, "a", ASTCtx,
Env));
martinboehme wrote:
I've added a comment explaining what the language rules are that govern the
behavior we expect here.
https://github.com/llvm/llvm-project/pull/82986
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Correctly treat empty initializer lists for unions. (PR #82986)
@@ -2392,14 +2392,88 @@ TEST(TransferTest, InitListExprAsUnion) {
} F;
public:
- constexpr target() : F{nullptr} {}
+ constexpr target() : F{nullptr} {
+int *null = nullptr;
+F.b; // Make sure we reference 'b' so it is modeled.
+// [[p]]
+ }
};
)cc";
runDataflow(
Code,
[](const llvm::StringMap> ,
ASTContext ) {
-// Just verify that it doesn't crash.
+const Environment = getEnvironmentAtAnnotation(Results, "p");
+
+auto = getFieldLoc(
+*Env.getThisPointeeStorageLocation(), "F", ASTCtx);
+auto *AVal = cast(getFieldValue(, "a", ASTCtx,
Env));
+EXPECT_EQ(AVal, (ASTCtx, Env, "null"));
+EXPECT_EQ(getFieldValue(, "b", ASTCtx, Env), nullptr);
+ });
+}
+
+TEST(TransferTest, EmptyInitListExprForUnion) {
+ // This is a crash repro.
+ std::string Code = R"cc(
+class target {
+ union {
+int *a;
+bool *b;
+ } F;
+
+ public:
+ constexpr target() : F{} {
+int *null = nullptr;
+F.b; // Make sure we reference 'b' so it is modeled.
+// [[p]]
+ }
+};
+ )cc";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap> ,
+ ASTContext ) {
+const Environment = getEnvironmentAtAnnotation(Results, "p");
+
+auto = getFieldLoc(
+*Env.getThisPointeeStorageLocation(), "F", ASTCtx);
+auto *AVal = cast(getFieldValue(, "a", ASTCtx,
Env));
+EXPECT_EQ(AVal, (ASTCtx, Env, "null"));
+EXPECT_EQ(getFieldValue(, "b", ASTCtx, Env), nullptr);
+ });
+}
+
+TEST(TransferTest, EmptyInitListExprForStruct) {
+ std::string Code = R"cc(
+class target {
+ struct {
+int *a;
+bool *b;
+ } F;
+
+ public:
+ constexpr target() : F{} {
+int *NullIntPtr = nullptr;
+bool *NullBoolPtr = nullptr;
+// [[p]]
+ }
+};
+ )cc";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap> ,
+ ASTContext ) {
+const Environment = getEnvironmentAtAnnotation(Results, "p");
+
+auto = getFieldLoc(
+*Env.getThisPointeeStorageLocation(), "F", ASTCtx);
+auto *AVal = cast(getFieldValue(, "a", ASTCtx,
Env));
+ASSERT_EQ(AVal,
martinboehme wrote:
Good point -- these should have been `EXPECT_EQ`. Changed.
https://github.com/llvm/llvm-project/pull/82986
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Correctly treat empty initializer lists for unions. (PR #82986)
https://github.com/martinboehme updated
https://github.com/llvm/llvm-project/pull/82986
>From cd64b0e04026235283016eaf1cd601076ab7aeb2 Mon Sep 17 00:00:00 2001
From: Martin Braenne
Date: Tue, 27 Feb 2024 18:23:36 +
Subject: [PATCH 1/3] Reapply "[clang][dataflow] Correctly handle
`InitListExpr` of union type." (#82856)
This reverts commit c4e94633e8a48ee33115d5d3161ee142fc1c9700.
---
.../FlowSensitive/DataflowEnvironment.h | 9 ---
.../FlowSensitive/DataflowEnvironment.cpp | 18 ++---
clang/lib/Analysis/FlowSensitive/Transfer.cpp | 25 +++
.../Analysis/FlowSensitive/TestingSupport.h | 19 ++
.../Analysis/FlowSensitive/TransferTest.cpp | 14 +--
5 files changed, 65 insertions(+), 20 deletions(-)
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index 7f8c70d169376e..62e7af7ac219bc 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -723,9 +723,12 @@ RecordStorageLocation *getImplicitObjectLocation(const
CXXMemberCallExpr ,
RecordStorageLocation *getBaseObjectLocation(const MemberExpr ,
const Environment );
-/// Returns the fields of `RD` that are initialized by an `InitListExpr`, in
the
-/// order in which they appear in `InitListExpr::inits()`.
-std::vector getFieldsForInitListExpr(const RecordDecl *RD);
+/// Returns the fields of a `RecordDecl` that are initialized by an
+/// `InitListExpr`, in the order in which they appear in
+/// `InitListExpr::inits()`.
+/// `Init->getType()` must be a record type.
+std::vector
+getFieldsForInitListExpr(const InitListExpr *InitList);
/// Associates a new `RecordValue` with `Loc` and returns the new value.
RecordValue (RecordStorageLocation , Environment );
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index d487944ce92111..0cfc26ea952cda 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -361,8 +361,8 @@ getFieldsGlobalsAndFuncs(const Stmt , FieldSet ,
if (const auto *FD = dyn_cast(VD))
Fields.insert(FD);
} else if (auto *InitList = dyn_cast()) {
-if (RecordDecl *RD = InitList->getType()->getAsRecordDecl())
- for (const auto *FD : getFieldsForInitListExpr(RD))
+if (InitList->getType()->isRecordType())
+ for (const auto *FD : getFieldsForInitListExpr(InitList))
Fields.insert(FD);
}
}
@@ -1104,12 +1104,22 @@ RecordStorageLocation *getBaseObjectLocation(const
MemberExpr ,
return Env.get(*Base);
}
-std::vector getFieldsForInitListExpr(const RecordDecl *RD) {
+std::vector
+getFieldsForInitListExpr(const InitListExpr *InitList) {
+ const RecordDecl *RD = InitList->getType()->getAsRecordDecl();
+ assert(RD != nullptr);
+
+ std::vector Fields;
+
+ if (InitList->getType()->isUnionType()) {
+Fields.push_back(InitList->getInitializedFieldInUnion());
+return Fields;
+ }
+
// Unnamed bitfields are only used for padding and do not appear in
// `InitListExpr`'s inits. However, those fields do appear in `RecordDecl`'s
// field list, and we thus need to remove them before mapping inits to
// fields to avoid mapping inits to the wrongs fields.
- std::vector Fields;
llvm::copy_if(
RD->fields(), std::back_inserter(Fields),
[](const FieldDecl *Field) { return !Field->isUnnamedBitfield(); });
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index 089854264f483a..3f7e2e27d25cfd 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -663,14 +663,7 @@ class TransferVisitor : public
ConstStmtVisitor {
void VisitInitListExpr(const InitListExpr *S) {
QualType Type = S->getType();
-if (Type->isUnionType()) {
- // FIXME: Initialize unions properly.
- if (auto *Val = Env.createValue(Type))
-Env.setValue(*S, *Val);
- return;
-}
-
-if (!Type->isStructureOrClassType()) {
+if (!Type->isRecordType()) {
// Until array initialization is implemented, we skip arrays and don't
// need to care about cases where `getNumInits() > 1`.
if (!Type->isArrayType() && S->getNumInits() == 1)
@@ -688,10 +681,9 @@ class TransferVisitor : public
ConstStmtVisitor {
llvm::DenseMap FieldLocs;
// This only contains the direct fields for the given type.
-std::vector FieldsForInit =
-getFieldsForInitListExpr(Type->getAsRecordDecl());
+std::vector FieldsForInit = getFieldsForInitListExpr(S);
-// `S->inits()` contains all the initializer epressions, including the
+// `S->inits()` contains all the initializer expressions,
[clang] [clang][dataflow] Correctly treat empty initializer lists for unions. (PR #82986)
@@ -685,9 +685,22 @@ class TransferVisitor : public
ConstStmtVisitor {
// `S->inits()` contains all the initializer expressions, including the
// ones for direct base classes.
-auto Inits = S->inits();
+ArrayRef Inits = S->inits();
size_t InitIdx = 0;
+// Unions initialized with an empty initializer list need special
treatment.
martinboehme wrote:
It appears that CodeGen also appears to special-case this like we do:
https://github.com/llvm/llvm-project/blob/e08fe575d5953b6ca0d7a578c1afa00247f0a12f/clang/lib/CodeGen/CGExprAgg.cpp#L1760
And if we changed the AST so that we had an `ImplicitValueInitExpr` here, it
looks as if the general case in CodeGen would do the right thing.
I'll make a note of this for myself.
https://github.com/llvm/llvm-project/pull/82986
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Correctly treat empty initializer lists for unions. (PR #82986)
@@ -2392,14 +2392,88 @@ TEST(TransferTest, InitListExprAsUnion) {
} F;
public:
- constexpr target() : F{nullptr} {}
+ constexpr target() : F{nullptr} {
+int *null = nullptr;
+F.b; // Make sure we reference 'b' so it is modeled.
+// [[p]]
+ }
};
)cc";
runDataflow(
Code,
[](const llvm::StringMap> ,
ASTContext ) {
-// Just verify that it doesn't crash.
+const Environment = getEnvironmentAtAnnotation(Results, "p");
+
+auto = getFieldLoc(
+*Env.getThisPointeeStorageLocation(), "F", ASTCtx);
+auto *AVal = cast(getFieldValue(, "a", ASTCtx,
Env));
+EXPECT_EQ(AVal, (ASTCtx, Env, "null"));
+EXPECT_EQ(getFieldValue(, "b", ASTCtx, Env), nullptr);
+ });
+}
+
+TEST(TransferTest, EmptyInitListExprForUnion) {
+ // This is a crash repro.
+ std::string Code = R"cc(
+class target {
+ union {
+int *a;
+bool *b;
+ } F;
+
+ public:
+ constexpr target() : F{} {
+int *null = nullptr;
+F.b; // Make sure we reference 'b' so it is modeled.
+// [[p]]
+ }
+};
+ )cc";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap> ,
+ ASTContext ) {
+const Environment = getEnvironmentAtAnnotation(Results, "p");
+
+auto = getFieldLoc(
+*Env.getThisPointeeStorageLocation(), "F", ASTCtx);
+auto *AVal = cast(getFieldValue(, "a", ASTCtx,
Env));
+EXPECT_EQ(AVal, (ASTCtx, Env, "null"));
+EXPECT_EQ(getFieldValue(, "b", ASTCtx, Env), nullptr);
+ });
+}
+
+TEST(TransferTest, EmptyInitListExprForStruct) {
+ std::string Code = R"cc(
+class target {
+ struct {
+int *a;
+bool *b;
+ } F;
+
+ public:
+ constexpr target() : F{} {
+int *NullIntPtr = nullptr;
+bool *NullBoolPtr = nullptr;
+// [[p]]
+ }
+};
+ )cc";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap> ,
+ ASTContext ) {
+const Environment = getEnvironmentAtAnnotation(Results, "p");
+
+auto = getFieldLoc(
+*Env.getThisPointeeStorageLocation(), "F", ASTCtx);
+auto *AVal = cast(getFieldValue(, "a", ASTCtx,
Env));
+ASSERT_EQ(AVal,
ymand wrote:
why ASSERT (vs EXPECT, like above)?
https://github.com/llvm/llvm-project/pull/82986
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Correctly treat empty initializer lists for unions. (PR #82986)
@@ -2392,14 +2392,88 @@ TEST(TransferTest, InitListExprAsUnion) {
} F;
public:
- constexpr target() : F{nullptr} {}
+ constexpr target() : F{nullptr} {
+int *null = nullptr;
+F.b; // Make sure we reference 'b' so it is modeled.
+// [[p]]
+ }
};
)cc";
runDataflow(
Code,
[](const llvm::StringMap> ,
ASTContext ) {
-// Just verify that it doesn't crash.
+const Environment = getEnvironmentAtAnnotation(Results, "p");
+
+auto = getFieldLoc(
+*Env.getThisPointeeStorageLocation(), "F", ASTCtx);
+auto *AVal = cast(getFieldValue(, "a", ASTCtx,
Env));
+EXPECT_EQ(AVal, (ASTCtx, Env, "null"));
+EXPECT_EQ(getFieldValue(, "b", ASTCtx, Env), nullptr);
+ });
+}
+
+TEST(TransferTest, EmptyInitListExprForUnion) {
+ // This is a crash repro.
+ std::string Code = R"cc(
+class target {
+ union {
+int *a;
+bool *b;
+ } F;
+
+ public:
+ constexpr target() : F{} {
+int *null = nullptr;
+F.b; // Make sure we reference 'b' so it is modeled.
+// [[p]]
+ }
+};
+ )cc";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap> ,
+ ASTContext ) {
+const Environment = getEnvironmentAtAnnotation(Results, "p");
+
+auto = getFieldLoc(
+*Env.getThisPointeeStorageLocation(), "F", ASTCtx);
+auto *AVal = cast(getFieldValue(, "a", ASTCtx,
Env));
ymand wrote:
Why is `a` set when the initializer is empty? I'm guessing it's because line
699 in Transfer.cpp uses `front()` and so initializes the first field of the
union. If so, perhaps say this explicitly in the comment preceding those lines?
https://github.com/llvm/llvm-project/pull/82986
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Correctly treat empty initializer lists for unions. (PR #82986)
https://github.com/ymand edited https://github.com/llvm/llvm-project/pull/82986 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Correctly treat empty initializer lists for unions. (PR #82986)
https://github.com/ymand approved this pull request. https://github.com/llvm/llvm-project/pull/82986 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Correctly treat empty initializer lists for unions. (PR #82986)
martinboehme wrote: Just modified this PR to essentially reapply https://github.com/llvm/llvm-project/pull/82348 under it. https://github.com/llvm/llvm-project/pull/82986 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Correctly treat empty initializer lists for unions. (PR #82986)
https://github.com/martinboehme updated
https://github.com/llvm/llvm-project/pull/82986
>From cd64b0e04026235283016eaf1cd601076ab7aeb2 Mon Sep 17 00:00:00 2001
From: Martin Braenne
Date: Tue, 27 Feb 2024 18:23:36 +
Subject: [PATCH 1/2] Reapply "[clang][dataflow] Correctly handle
`InitListExpr` of union type." (#82856)
This reverts commit c4e94633e8a48ee33115d5d3161ee142fc1c9700.
---
.../FlowSensitive/DataflowEnvironment.h | 9 ---
.../FlowSensitive/DataflowEnvironment.cpp | 18 ++---
clang/lib/Analysis/FlowSensitive/Transfer.cpp | 25 +++
.../Analysis/FlowSensitive/TestingSupport.h | 19 ++
.../Analysis/FlowSensitive/TransferTest.cpp | 14 +--
5 files changed, 65 insertions(+), 20 deletions(-)
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index 7f8c70d169376e..62e7af7ac219bc 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -723,9 +723,12 @@ RecordStorageLocation *getImplicitObjectLocation(const
CXXMemberCallExpr ,
RecordStorageLocation *getBaseObjectLocation(const MemberExpr ,
const Environment );
-/// Returns the fields of `RD` that are initialized by an `InitListExpr`, in
the
-/// order in which they appear in `InitListExpr::inits()`.
-std::vector getFieldsForInitListExpr(const RecordDecl *RD);
+/// Returns the fields of a `RecordDecl` that are initialized by an
+/// `InitListExpr`, in the order in which they appear in
+/// `InitListExpr::inits()`.
+/// `Init->getType()` must be a record type.
+std::vector
+getFieldsForInitListExpr(const InitListExpr *InitList);
/// Associates a new `RecordValue` with `Loc` and returns the new value.
RecordValue (RecordStorageLocation , Environment );
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index d487944ce92111..0cfc26ea952cda 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -361,8 +361,8 @@ getFieldsGlobalsAndFuncs(const Stmt , FieldSet ,
if (const auto *FD = dyn_cast(VD))
Fields.insert(FD);
} else if (auto *InitList = dyn_cast()) {
-if (RecordDecl *RD = InitList->getType()->getAsRecordDecl())
- for (const auto *FD : getFieldsForInitListExpr(RD))
+if (InitList->getType()->isRecordType())
+ for (const auto *FD : getFieldsForInitListExpr(InitList))
Fields.insert(FD);
}
}
@@ -1104,12 +1104,22 @@ RecordStorageLocation *getBaseObjectLocation(const
MemberExpr ,
return Env.get(*Base);
}
-std::vector getFieldsForInitListExpr(const RecordDecl *RD) {
+std::vector
+getFieldsForInitListExpr(const InitListExpr *InitList) {
+ const RecordDecl *RD = InitList->getType()->getAsRecordDecl();
+ assert(RD != nullptr);
+
+ std::vector Fields;
+
+ if (InitList->getType()->isUnionType()) {
+Fields.push_back(InitList->getInitializedFieldInUnion());
+return Fields;
+ }
+
// Unnamed bitfields are only used for padding and do not appear in
// `InitListExpr`'s inits. However, those fields do appear in `RecordDecl`'s
// field list, and we thus need to remove them before mapping inits to
// fields to avoid mapping inits to the wrongs fields.
- std::vector Fields;
llvm::copy_if(
RD->fields(), std::back_inserter(Fields),
[](const FieldDecl *Field) { return !Field->isUnnamedBitfield(); });
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index 089854264f483a..3f7e2e27d25cfd 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -663,14 +663,7 @@ class TransferVisitor : public
ConstStmtVisitor {
void VisitInitListExpr(const InitListExpr *S) {
QualType Type = S->getType();
-if (Type->isUnionType()) {
- // FIXME: Initialize unions properly.
- if (auto *Val = Env.createValue(Type))
-Env.setValue(*S, *Val);
- return;
-}
-
-if (!Type->isStructureOrClassType()) {
+if (!Type->isRecordType()) {
// Until array initialization is implemented, we skip arrays and don't
// need to care about cases where `getNumInits() > 1`.
if (!Type->isArrayType() && S->getNumInits() == 1)
@@ -688,10 +681,9 @@ class TransferVisitor : public
ConstStmtVisitor {
llvm::DenseMap FieldLocs;
// This only contains the direct fields for the given type.
-std::vector FieldsForInit =
-getFieldsForInitListExpr(Type->getAsRecordDecl());
+std::vector FieldsForInit = getFieldsForInitListExpr(S);
-// `S->inits()` contains all the initializer epressions, including the
+// `S->inits()` contains all the initializer expressions,
[clang] [clang][dataflow] Correctly treat empty initializer lists for unions. (PR #82986)
https://github.com/Xazax-hun approved this pull request. https://github.com/llvm/llvm-project/pull/82986 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Correctly treat empty initializer lists for unions. (PR #82986)
@@ -685,9 +685,22 @@ class TransferVisitor : public
ConstStmtVisitor {
// `S->inits()` contains all the initializer expressions, including the
// ones for direct base classes.
-auto Inits = S->inits();
+ArrayRef Inits = S->inits();
size_t InitIdx = 0;
+// Unions initialized with an empty initializer list need special
treatment.
Xazax-hun wrote:
I am wondering how does CodeGen deal with this? If it happens to have similar
extra logic, we could simplify both by changing the AST.
https://github.com/llvm/llvm-project/pull/82986
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Correctly treat empty initializer lists for unions. (PR #82986)
https://github.com/Xazax-hun edited https://github.com/llvm/llvm-project/pull/82986 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Correctly treat empty initializer lists for unions. (PR #82986)
https://github.com/ymand approved this pull request. https://github.com/llvm/llvm-project/pull/82986 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Correctly treat empty initializer lists for unions. (PR #82986)
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: None (martinboehme)
Changes
This fixes a crash introduced by https://github.com/llvm/llvm-project/pull/82348
but also adds additional handling to make sure that we treat empty initializer
lists for both unions and structs/classes correctly (see tests added in this
patch).
---
Full diff: https://github.com/llvm/llvm-project/pull/82986.diff
3 Files Affected:
- (modified) clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp (+6-1)
- (modified) clang/lib/Analysis/FlowSensitive/Transfer.cpp (+14-1)
- (modified) clang/unittests/Analysis/FlowSensitive/TransferTest.cpp (+66-2)
``diff
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 0cfc26ea952cda..fd7b06efcc7861 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -983,7 +983,7 @@ StorageLocation ::createObjectInternal(const
ValueDecl *D,
}
Value *Val = nullptr;
- if (InitExpr)
+ if (InitExpr) {
// In the (few) cases where an expression is intentionally
// "uninterpreted", `InitExpr` is not associated with a value. There are
// two ways to handle this situation: propagate the status, so that
@@ -998,6 +998,11 @@ StorageLocation ::createObjectInternal(const
ValueDecl *D,
// default value (assuming we don't update the environment API to return
// references).
Val = getValue(*InitExpr);
+
+if (!Val && isa(InitExpr) &&
+InitExpr->getType()->isPointerType())
+ Val =
(InitExpr->getType()->getPointeeType());
+ }
if (!Val)
Val = createValue(Ty);
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index cd1f04e53cff68..d73965806a533f 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -685,9 +685,22 @@ class TransferVisitor : public
ConstStmtVisitor {
// `S->inits()` contains all the initializer expressions, including the
// ones for direct base classes.
-auto Inits = S->inits();
+ArrayRef Inits = S->inits();
size_t InitIdx = 0;
+// Unions initialized with an empty initializer list need special
treatment.
+// For structs/classes initialized with an empty initializer list, Clang
+// puts `ImplicitValueInitExpr`s in `InitListExpr::inits()`, but for
unions,
+// it doesn't do this -- so we create an `ImplicitValueInitExpr` ourselves.
+std::optional ImplicitValueInitForUnion;
+SmallVector InitsForUnion;
+if (S->getType()->isUnionType() && Inits.empty()) {
+ assert(FieldsForInit.size() == 1);
+ ImplicitValueInitForUnion.emplace(FieldsForInit.front()->getType());
+ InitsForUnion.push_back(&*ImplicitValueInitForUnion);
+ Inits = InitsForUnion;
+}
+
// Initialize base classes.
if (auto* R = S->getType()->getAsCXXRecordDecl()) {
assert(FieldsForInit.size() + R->getNumBases() == Inits.size());
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index e7d74581865a32..2eb9a37f289426 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -2393,8 +2393,72 @@ TEST(TransferTest, InitListExprAsUnion) {
auto = getFieldLoc(
*Env.getThisPointeeStorageLocation(), "F", ASTCtx);
auto *AVal = cast(getFieldValue(, "a", ASTCtx,
Env));
-ASSERT_EQ(AVal, (ASTCtx, Env, "null"));
-ASSERT_EQ(getFieldValue(, "b", ASTCtx, Env), nullptr);
+EXPECT_EQ(AVal, (ASTCtx, Env, "null"));
+EXPECT_EQ(getFieldValue(, "b", ASTCtx, Env), nullptr);
+ });
+}
+
+TEST(TransferTest, EmptyInitListExprForUnion) {
+ // This is a crash repro.
+ std::string Code = R"cc(
+class target {
+ union {
+int *a;
+bool *b;
+ } F;
+
+ public:
+ constexpr target() : F{} {
+int *null = nullptr;
+F.b; // Make sure we reference 'b' so it is modeled.
+// [[p]]
+ }
+};
+ )cc";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap> ,
+ ASTContext ) {
+const Environment = getEnvironmentAtAnnotation(Results, "p");
+
+auto = getFieldLoc(
+*Env.getThisPointeeStorageLocation(), "F", ASTCtx);
+auto *AVal = cast(getFieldValue(, "a", ASTCtx,
Env));
+EXPECT_EQ(AVal, (ASTCtx, Env, "null"));
+EXPECT_EQ(getFieldValue(, "b", ASTCtx, Env), nullptr);
+ });
+}
+
+TEST(TransferTest, EmptyInitListExprForStruct) {
+ std::string Code = R"cc(
+class target {
+ struct {
+int *a;
+bool *b;
+ } F;
+
+ public:
+ constexpr target() : F{} {
+int *NullIntPtr = nullptr;
+bool *NullBoolPtr = nullptr;
+// [[p]]
+
[clang] [clang][dataflow] Correctly treat empty initializer lists for unions. (PR #82986)
https://github.com/martinboehme created
https://github.com/llvm/llvm-project/pull/82986
This fixes a crash introduced by https://github.com/llvm/llvm-project/pull/82348
but also adds additional handling to make sure that we treat empty initializer
lists for both unions and structs/classes correctly (see tests added in this
patch).
>From a48dc2e9a481ba5ed5d801a59c1dbc23fe0092d1 Mon Sep 17 00:00:00 2001
From: Martin Braenne
Date: Mon, 26 Feb 2024 11:41:08 +
Subject: [PATCH] [clang][dataflow] Correctly treat empty initializer lists for
unions.
This fixes a crash introduced by https://github.com/llvm/llvm-project/pull/82348
but also adds additional handling to make sure that we treat empty initializer
lists for both unions and structs/classes correctly (see tests added in this
patch).
---
.../FlowSensitive/DataflowEnvironment.cpp | 7 +-
clang/lib/Analysis/FlowSensitive/Transfer.cpp | 15 +++-
.../Analysis/FlowSensitive/TransferTest.cpp | 68 ++-
3 files changed, 86 insertions(+), 4 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 0cfc26ea952cda..fd7b06efcc7861 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -983,7 +983,7 @@ StorageLocation ::createObjectInternal(const
ValueDecl *D,
}
Value *Val = nullptr;
- if (InitExpr)
+ if (InitExpr) {
// In the (few) cases where an expression is intentionally
// "uninterpreted", `InitExpr` is not associated with a value. There are
// two ways to handle this situation: propagate the status, so that
@@ -998,6 +998,11 @@ StorageLocation ::createObjectInternal(const
ValueDecl *D,
// default value (assuming we don't update the environment API to return
// references).
Val = getValue(*InitExpr);
+
+if (!Val && isa(InitExpr) &&
+InitExpr->getType()->isPointerType())
+ Val =
(InitExpr->getType()->getPointeeType());
+ }
if (!Val)
Val = createValue(Ty);
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index cd1f04e53cff68..d73965806a533f 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -685,9 +685,22 @@ class TransferVisitor : public
ConstStmtVisitor {
// `S->inits()` contains all the initializer expressions, including the
// ones for direct base classes.
-auto Inits = S->inits();
+ArrayRef Inits = S->inits();
size_t InitIdx = 0;
+// Unions initialized with an empty initializer list need special
treatment.
+// For structs/classes initialized with an empty initializer list, Clang
+// puts `ImplicitValueInitExpr`s in `InitListExpr::inits()`, but for
unions,
+// it doesn't do this -- so we create an `ImplicitValueInitExpr` ourselves.
+std::optional ImplicitValueInitForUnion;
+SmallVector InitsForUnion;
+if (S->getType()->isUnionType() && Inits.empty()) {
+ assert(FieldsForInit.size() == 1);
+ ImplicitValueInitForUnion.emplace(FieldsForInit.front()->getType());
+ InitsForUnion.push_back(&*ImplicitValueInitForUnion);
+ Inits = InitsForUnion;
+}
+
// Initialize base classes.
if (auto* R = S->getType()->getAsCXXRecordDecl()) {
assert(FieldsForInit.size() + R->getNumBases() == Inits.size());
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index e7d74581865a32..2eb9a37f289426 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -2393,8 +2393,72 @@ TEST(TransferTest, InitListExprAsUnion) {
auto = getFieldLoc(
*Env.getThisPointeeStorageLocation(), "F", ASTCtx);
auto *AVal = cast(getFieldValue(, "a", ASTCtx,
Env));
-ASSERT_EQ(AVal, (ASTCtx, Env, "null"));
-ASSERT_EQ(getFieldValue(, "b", ASTCtx, Env), nullptr);
+EXPECT_EQ(AVal, (ASTCtx, Env, "null"));
+EXPECT_EQ(getFieldValue(, "b", ASTCtx, Env), nullptr);
+ });
+}
+
+TEST(TransferTest, EmptyInitListExprForUnion) {
+ // This is a crash repro.
+ std::string Code = R"cc(
+class target {
+ union {
+int *a;
+bool *b;
+ } F;
+
+ public:
+ constexpr target() : F{} {
+int *null = nullptr;
+F.b; // Make sure we reference 'b' so it is modeled.
+// [[p]]
+ }
+};
+ )cc";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap> ,
+ ASTContext ) {
+const Environment = getEnvironmentAtAnnotation(Results, "p");
+
+auto = getFieldLoc(
+*Env.getThisPointeeStorageLocation(), "F", ASTCtx);
+auto *AVal = cast(getFieldValue(, "a", ASTCtx,
Env));
+EXPECT_EQ(AVal, (ASTCtx, Env, "null"));
+
