[clang] [clang][dataflow] When analyzing ctors, don't initialize fields of `*this` with values. (PR #84164)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Mar 6 05:09:21 PST 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-analysis
Author: None (martinboehme)
<details>
<summary>Changes</summary>
This is the constructor's job, and we want to be able to test that it does this.
---
Full diff: https://github.com/llvm/llvm-project/pull/84164.diff
5 Files Affected:
- (modified) clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h (+5)
- (modified) clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp (+19-2)
- (modified) clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp (+1-1)
- (modified) clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp (+4-1)
- (modified) clang/unittests/Analysis/FlowSensitive/TransferTest.cpp (+63)
``````````diff
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index 62e7af7ac219bc..7f4a8f0341c9ba 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -436,6 +436,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 fd7b06efcc7861..e359f037ea3a7a 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -414,8 +414,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);
}
}
}
@@ -799,6 +806,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 4c88c46142d64d..4bc6b19f2553fb 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -388,7 +388,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
@@ -404,6 +403,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 {
``````````
</details>
https://github.com/llvm/llvm-project/pull/84164
More information about the cfe-commits
mailing list