[clang] f9026cf - [clang][dataflow] Fix Record initialization with InitListExpr and inheritances
Martin Braenne via cfe-commits
cfe-commits at lists.llvm.org
Thu Sep 7 00:38:23 PDT 2023
Author: Kinuko Yasuda
Date: 2023-09-07T07:37:50Z
New Revision: f9026cfb7680e2c2a4c8c91dd33f710ea1d321a3
URL: https://github.com/llvm/llvm-project/commit/f9026cfb7680e2c2a4c8c91dd33f710ea1d321a3
DIFF: https://github.com/llvm/llvm-project/commit/f9026cfb7680e2c2a4c8c91dd33f710ea1d321a3.diff
LOG: [clang][dataflow] Fix Record initialization with InitListExpr and inheritances
Usually RecordValues for record objects (e.g. struct) are initialized with
`Environment::createValue()` which internally calls `getObjectFields()` to
collects all fields from the current and base classes, and then filter them
with `ModeledValues` via `DACtx::getModeledFields()` so that the fields that
are actually referenced are modeled.
The consistent set of fields should be initialized when a record is initialized
with an initializer list (InitListExpr), however the existing code's behavior
was different.
Before this patch:
* When a struct is initialized with InitListExpr, its fields are
initialized based on what is returned by `getFieldsForInitListExpr()`, which
only collects the direct fields in the current class, but not from the base
classes. Moreover, if the base classes have their own InitListExpr, values
that are initialized by their InitListExpr's weren't merged into the
child objects.
After this patch:
* When a struct is initialized with InitListExpr, it collects and merges the
fields in the base classes that were initialized by their InitListExpr's.
The code also asserts that the consistent set of fields are initialized
with the ModeledFields.
Reviewed By: mboehme
Differential Revision: https://reviews.llvm.org/D159284
Added:
Modified:
clang/lib/Analysis/FlowSensitive/Transfer.cpp
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
Removed:
################################################################################
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index fcd9b20027cde9..67d8be392ae605 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -27,12 +27,12 @@
#include "clang/Analysis/FlowSensitive/Value.h"
#include "clang/Basic/Builtins.h"
#include "clang/Basic/OperatorKinds.h"
-#include "llvm/ADT/STLExtras.h"
#include "llvm/Support/Casting.h"
-#include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/Debug.h"
+#include <assert.h>
#include <cassert>
-#include <memory>
-#include <tuple>
+
+#define DEBUG_TYPE "dataflow"
namespace clang {
namespace dataflow {
@@ -629,17 +629,66 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
return;
}
- std::vector<FieldDecl *> Fields =
- getFieldsForInitListExpr(Type->getAsRecordDecl());
llvm::DenseMap<const ValueDecl *, StorageLocation *> FieldLocs;
- for (auto [Field, Init] : llvm::zip(Fields, S->inits())) {
- assert(Field != nullptr);
- assert(Init != nullptr);
+ // This only contains the direct fields for the given type.
+ std::vector<FieldDecl *> FieldsForInit =
+ getFieldsForInitListExpr(Type->getAsRecordDecl());
- FieldLocs.insert({Field, &Env.createObject(Field->getType(), Init)});
+ // `S->inits()` contains all the initializer epressions, including the
+ // ones for direct base classes.
+ auto Inits = S->inits();
+ size_t InitIdx = 0;
+
+ // Initialize base classes.
+ if (auto* R = S->getType()->getAsCXXRecordDecl()) {
+ assert(FieldsForInit.size() + R->getNumBases() == Inits.size());
+ for ([[maybe_unused]] const CXXBaseSpecifier &Base : R->bases()) {
+ assert(InitIdx < Inits.size());
+ auto Init = Inits[InitIdx++];
+ assert(Base.getType().getCanonicalType() ==
+ Init->getType().getCanonicalType());
+ auto* BaseVal = cast_or_null<RecordValue>(Env.getValue(*Init));
+ if (!BaseVal)
+ BaseVal = cast<RecordValue>(Env.createValue(Init->getType()));
+ // Take ownership of the fields of the `RecordValue` for the base class
+ // and incorporate them into the "flattened" set of fields for the
+ // derived class.
+ auto Children = BaseVal->getLoc().children();
+ FieldLocs.insert(Children.begin(), Children.end());
+ }
}
+ assert(FieldsForInit.size() == Inits.size() - InitIdx);
+ for (auto Field : FieldsForInit) {
+ assert(InitIdx < Inits.size());
+ auto Init = Inits[InitIdx++];
+ assert(
+ // The types are same, or
+ Field->getType().getCanonicalType().getUnqualifiedType() ==
+ Init->getType().getCanonicalType() ||
+ // The field's type is T&, and initializer is T
+ (Field->getType()->isReferenceType() &&
+ Field->getType().getCanonicalType()->getPointeeType() ==
+ Init->getType().getCanonicalType()));
+ auto& Loc = Env.createObject(Field->getType(), Init);
+ FieldLocs.insert({Field, &Loc});
+ }
+
+ LLVM_DEBUG({
+ // Check that we satisfy the invariant that a `RecordStorageLoation`
+ // contains exactly the set of modeled fields for that type.
+ // `ModeledFields` includes fields from all the bases, but only the
+ // modeled ones. However, if a class type is initialized with an
+ // `InitListExpr`, all fields in the class, including those from base
+ // classes, are included in the set of modeled fields. The code above
+ // should therefore populate exactly the modeled fields.
+ auto ModeledFields = Env.getDataflowAnalysisContext().getModeledFields(Type);
+ assert(ModeledFields.size() == FieldLocs.size());
+ for ([[maybe_unused]] auto [Field, Loc] : FieldLocs)
+ assert(ModeledFields.contains(cast_or_null<FieldDecl>(Field)));
+ });
+
auto &Loc =
Env.getDataflowAnalysisContext().arena().create<RecordStorageLocation>(
Type, std::move(FieldLocs));
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 28d062196cb07b..ca6c00343e58d8 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -1446,6 +1446,99 @@ TEST(TransferTest, BaseClassInitializer) {
llvm::Succeeded());
}
+TEST(TransferTest, StructModeledFieldsWithComplicatedInheritance) {
+ std::string Code = R"(
+ struct Base1 {
+ int base1_1;
+ int base1_2;
+ };
+ struct Intermediate : Base1 {
+ int intermediate_1;
+ int intermediate_2;
+ };
+ struct Base2 {
+ int base2_1;
+ int base2_2;
+ };
+ struct MostDerived : public Intermediate, Base2 {
+ int most_derived_1;
+ int most_derived_2;
+ };
+
+ void target() {
+ MostDerived MD;
+ MD.base1_2 = 1;
+ MD.intermediate_2 = 1;
+ MD.base2_2 = 1;
+ MD.most_derived_2 = 1;
+ // [[p]]
+ }
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {
+ const Environment &Env =
+ getEnvironmentAtAnnotation(Results, "p");
+
+ // Only the accessed fields should exist in the model.
+ auto &MDLoc = getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "MD");
+ std::vector<const ValueDecl*> Fields;
+ for (auto [Field, _] : MDLoc.children())
+ Fields.push_back(Field);
+ ASSERT_THAT(Fields, UnorderedElementsAre(
+ findValueDecl(ASTCtx, "base1_2"),
+ findValueDecl(ASTCtx, "intermediate_2"),
+ findValueDecl(ASTCtx, "base2_2"),
+ findValueDecl(ASTCtx, "most_derived_2")));
+ });
+}
+
+TEST(TransferTest, StructInitializerListWithComplicatedInheritance) {
+ std::string Code = R"(
+ struct Base1 {
+ int base1;
+ };
+ struct Intermediate : Base1 {
+ int intermediate;
+ };
+ struct Base2 {
+ int base2;
+ };
+ struct MostDerived : public Intermediate, Base2 {
+ int most_derived;
+ };
+
+ void target() {
+ MostDerived MD = {};
+ // [[p]]
+ }
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {
+ const Environment &Env =
+ getEnvironmentAtAnnotation(Results, "p");
+
+ // When a struct is initialized with a initializer list, all the
+ // fields are considered "accessed", and therefore do exist.
+ auto &MD = getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "MD");
+ ASSERT_THAT(cast<IntegerValue>(
+ getFieldValue(&MD, *findValueDecl(ASTCtx, "base1"), Env)),
+ NotNull());
+ ASSERT_THAT(cast<IntegerValue>(
+ getFieldValue(&MD, *findValueDecl(ASTCtx, "intermediate"), Env)),
+ NotNull());
+ ASSERT_THAT(cast<IntegerValue>(
+ getFieldValue(&MD, *findValueDecl(ASTCtx, "base2"), Env)),
+ NotNull());
+ ASSERT_THAT(cast<IntegerValue>(
+ getFieldValue(&MD, *findValueDecl(ASTCtx, "most_derived"), Env)),
+ NotNull());
+ });
+}
+
TEST(TransferTest, ReferenceMember) {
std::string Code = R"(
struct A {
@@ -2051,6 +2144,26 @@ TEST(TransferTest, AssignmentOperatorFromCallResult) {
});
}
+TEST(TransferTest, AssignmentOperatorWithInitAndInheritance) {
+ // This is a crash repro.
+ std::string Code = R"(
+ struct B { int Foo; };
+ struct S : public B {};
+ void target() {
+ S S1 = { 1 };
+ S S2;
+ S S3;
+ S1 = S2; // Only Dst has InitListExpr.
+ S3 = S1; // Only Src has InitListExpr.
+ // [[p]]
+ }
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {});
+}
+
TEST(TransferTest, CopyConstructor) {
std::string Code = R"(
struct A {
More information about the cfe-commits
mailing list