[clang] 2d539db - [clang][dataflow] When analyzing ctors, don't initialize fields of `*this` with values. (#84164)
via cfe-commits
cfe-commits at lists.llvm.org
Thu Mar 7 23:19:06 PST 2024
Author: martinboehme
Date: 2024-03-08T08:19:02+01:00
New Revision: 2d539db246fd9d26201255b84d04dacf2782eddf
URL: https://github.com/llvm/llvm-project/commit/2d539db246fd9d26201255b84d04dacf2782eddf
DIFF: https://github.com/llvm/llvm-project/commit/2d539db246fd9d26201255b84d04dacf2782eddf.diff
LOG: [clang][dataflow] When analyzing ctors, don't initialize fields of `*this` with values. (#84164)
This is the constructor's job, and we want to be able to test that it
does this.
Added:
Modified:
clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index e8f009ef6c7913..2330697299fdd7 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -445,6 +445,11 @@ class Environment {
return createObjectInternal(&D, D.getType(), InitExpr);
}
+ /// Initializes the fields (including synthetic fields) of `Loc` with values,
+ /// unless values of the field type are not supported or we hit one of the
+ /// limits at which we stop producing values.
+ void initializeFieldsWithValues(RecordStorageLocation &Loc);
+
/// Assigns `Val` as the value of `Loc` in the environment.
void setValue(const StorageLocation &Loc, Value &Val);
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 62332a18c44a4a..1d2bd9a9b08af3 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -432,8 +432,15 @@ void Environment::initialize() {
}
} else if (MethodDecl->isImplicitObjectMemberFunction()) {
QualType ThisPointeeType = MethodDecl->getFunctionObjectParameterType();
- setThisPointeeStorageLocation(
- cast<RecordStorageLocation>(createObject(ThisPointeeType)));
+ auto &ThisLoc =
+ cast<RecordStorageLocation>(createStorageLocation(ThisPointeeType));
+ setThisPointeeStorageLocation(ThisLoc);
+ refreshRecordValue(ThisLoc, *this);
+ // Initialize fields of `*this` with values, but only if we're not
+ // analyzing a constructor; after all, it's the constructor's job to do
+ // this (and we want to be able to test that).
+ if (!isa<CXXConstructorDecl>(MethodDecl))
+ initializeFieldsWithValues(ThisLoc);
}
}
}
@@ -819,6 +826,16 @@ PointerValue &Environment::getOrCreateNullPointerValue(QualType PointeeType) {
return DACtx->getOrCreateNullPointerValue(PointeeType);
}
+void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc) {
+ llvm::DenseSet<QualType> Visited;
+ int CreatedValuesCount = 0;
+ initializeFieldsWithValues(Loc, Visited, 0, CreatedValuesCount);
+ if (CreatedValuesCount > MaxCompositeValueSize) {
+ llvm::errs() << "Attempting to initialize a huge value of type: "
+ << Loc.getType() << '\n';
+ }
+}
+
void Environment::setValue(const StorageLocation &Loc, Value &Val) {
assert(!isa<RecordValue>(&Val) || &cast<RecordValue>(&Val)->getLoc() == &Loc);
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index a9f39e153d0ce1..939247c047c66e 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -406,7 +406,6 @@ builtinTransferInitializer(const CFGInitializer &Elt,
}
}
assert(Member != nullptr);
- assert(MemberLoc != nullptr);
// FIXME: Instead of these case distinctions, we would ideally want to be able
// to simply use `Environment::createObject()` here, the same way that we do
@@ -422,6 +421,7 @@ builtinTransferInitializer(const CFGInitializer &Elt,
ParentLoc->setChild(*Member, InitExprLoc);
} else if (auto *InitExprVal = Env.getValue(*InitExpr)) {
+ assert(MemberLoc != nullptr);
if (Member->getType()->isRecordType()) {
auto *InitValStruct = cast<RecordValue>(InitExprVal);
// FIXME: Rather than performing a copy here, we should really be
diff --git a/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp b/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
index 09f5524e152c9f..5c4d42c6ccdcf8 100644
--- a/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
@@ -179,7 +179,10 @@ llvm::Error test::checkDataflowWithNoopAnalysis(
// -fnodelayed-template-parsing is the default everywhere but on Windows.
// Set it explicitly so that tests behave the same on Windows as on other
// platforms.
- "-fsyntax-only", "-fno-delayed-template-parsing",
+ // Set -Wno-unused-value because it's often desirable in tests to write
+ // expressions with unused value, and we don't want the output to be
+ // cluttered with warnings about them.
+ "-fsyntax-only", "-fno-delayed-template-parsing", "-Wno-unused-value",
"-std=" +
std::string(LangStandard::getLangStandardForKind(Std).getName())};
AnalysisInputs<NoopAnalysis> AI(
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index f534ccb1254701..9fde4179db1c49 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -1476,6 +1476,69 @@ TEST(TransferTest, BaseClassInitializer) {
llvm::Succeeded());
}
+TEST(TransferTest, FieldsDontHaveValuesInConstructor) {
+ // In a constructor, unlike in regular member functions, we don't want fields
+ // to be pre-initialized with values, because doing so is the job of the
+ // constructor.
+ std::string Code = R"(
+ struct target {
+ target() {
+ 0;
+ // [[p]]
+ // Mention the field so it is modeled;
+ Val;
+ }
+
+ int Val;
+ };
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {
+ const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+ EXPECT_EQ(getFieldValue(Env.getThisPointeeStorageLocation(), "Val",
+ ASTCtx, Env),
+ nullptr);
+ });
+}
+
+TEST(TransferTest, FieldsDontHaveValuesInConstructorWithBaseClass) {
+ // See above, but for a class with a base class.
+ std::string Code = R"(
+ struct Base {
+ int BaseVal;
+ };
+
+ struct target : public Base {
+ target() {
+ 0;
+ // [[p]]
+ // Mention the fields so they are modeled.
+ BaseVal;
+ Val;
+ }
+
+ int Val;
+ };
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {
+ const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+ // FIXME: The field of the base class should already have been
+ // initialized with a value by the base constructor. This test documents
+ // the current buggy behavior.
+ EXPECT_EQ(getFieldValue(Env.getThisPointeeStorageLocation(), "BaseVal",
+ ASTCtx, Env),
+ nullptr);
+ EXPECT_EQ(getFieldValue(Env.getThisPointeeStorageLocation(), "Val",
+ ASTCtx, Env),
+ nullptr);
+ });
+}
+
TEST(TransferTest, StructModeledFieldsWithAccessor) {
std::string Code = R"(
class S {
More information about the cfe-commits
mailing list