[clang] [clang][dataflow] Add synthetic fields to `RecordStorageLocation` (PR #73860)
via cfe-commits
cfe-commits at lists.llvm.org
Fri Dec 1 05:56:20 PST 2023
https://github.com/martinboehme updated https://github.com/llvm/llvm-project/pull/73860
>From a7138289989afaa1348185e52469d670c316eb45 Mon Sep 17 00:00:00 2001
From: Martin Braenne <mboehme at google.com>
Date: Wed, 29 Nov 2023 21:32:55 +0000
Subject: [PATCH 1/4] [clang][dataflow] Defer initialization of `Environment`.
The `Environment` is now only initialized with storage locations and values for
global variables, parameters, and the like after the analysis has been created.
Followup commits will add the ability to set callbacks that create synthetic
properties and associate these synthetic properties with values. The analysis
needs to be able to set these callbacks before any storage locations and values
are created.
---
.../FlowSensitive/DataflowEnvironment.h | 22 +++++--
.../FlowSensitive/DataflowEnvironment.cpp | 61 +++----------------
.../TypeErasedDataflowAnalysis.cpp | 61 ++++++++++++++++++-
.../FlowSensitive/DataflowEnvironmentTest.cpp | 5 ++
4 files changed, 91 insertions(+), 58 deletions(-)
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index 7c1f5491096326b..7026f289d62403c 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -166,6 +166,10 @@ class Environment {
/// with a symbolic representation of the `this` pointee.
Environment(DataflowAnalysisContext &DACtx, const DeclContext &DeclCtx);
+ /// Assigns storage locations and values to all global variables, fields
+ /// and functions referenced in `FuncDecl`. `FuncDecl` must have a body.
+ void initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl);
+
/// Returns a new environment that is a copy of this one.
///
/// The state of the program is initially the same, but can be mutated without
@@ -283,7 +287,15 @@ class Environment {
/// Returns the storage location assigned to the `this` pointee in the
/// environment or null if the `this` pointee has no assigned storage location
/// in the environment.
- RecordStorageLocation *getThisPointeeStorageLocation() const;
+ RecordStorageLocation *getThisPointeeStorageLocation() const {
+ return ThisPointeeLoc;
+ }
+
+ /// Sets the storage location assigned to the `this` pointee in the
+ /// environment.
+ void setThisPointeeStorageLocation(RecordStorageLocation &Loc) {
+ ThisPointeeLoc = &Loc;
+ }
/// Returns the location of the result object for a record-type prvalue.
///
@@ -570,6 +582,9 @@ class Environment {
return dyn_cast<FunctionDecl>(getDeclCtx());
}
+ /// Returns the size of the call stack.
+ size_t callStackSize() const { return CallStack.size(); }
+
/// Returns whether this `Environment` can be extended to analyze the given
/// `Callee` (i.e. if `pushCall` can be used), with recursion disallowed and a
/// given `MaxDepth`.
@@ -629,10 +644,7 @@ class Environment {
void pushCallInternal(const FunctionDecl *FuncDecl,
ArrayRef<const Expr *> Args);
- /// Assigns storage locations and values to all global variables, fields
- /// and functions referenced in `FuncDecl`. `FuncDecl` must have a body.
- void initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl);
-
+private:
// `DACtx` is not null and not owned by this object.
DataflowAnalysisContext *DACtx;
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 525ab188b01b8aa..131a68c03ed7563 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -367,6 +367,16 @@ getFieldsGlobalsAndFuncs(const Stmt &S, FieldSet &Fields,
}
}
+Environment::Environment(DataflowAnalysisContext &DACtx)
+ : DACtx(&DACtx),
+ FlowConditionToken(DACtx.arena().makeFlowConditionToken()) {}
+
+Environment::Environment(DataflowAnalysisContext &DACtx,
+ const DeclContext &DeclCtx)
+ : Environment(DACtx) {
+ CallStack.push_back(&DeclCtx);
+}
+
// FIXME: Add support for resetting globals after function calls to enable
// the implementation of sound analyses.
void Environment::initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl) {
@@ -416,59 +426,12 @@ void Environment::initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl) {
}
}
-Environment::Environment(DataflowAnalysisContext &DACtx)
- : DACtx(&DACtx),
- FlowConditionToken(DACtx.arena().makeFlowConditionToken()) {}
-
Environment Environment::fork() const {
Environment Copy(*this);
Copy.FlowConditionToken = DACtx->forkFlowCondition(FlowConditionToken);
return Copy;
}
-Environment::Environment(DataflowAnalysisContext &DACtx,
- const DeclContext &DeclCtx)
- : Environment(DACtx) {
- CallStack.push_back(&DeclCtx);
-
- if (const auto *FuncDecl = dyn_cast<FunctionDecl>(&DeclCtx)) {
- assert(FuncDecl->getBody() != nullptr);
-
- initFieldsGlobalsAndFuncs(FuncDecl);
-
- for (const auto *ParamDecl : FuncDecl->parameters()) {
- assert(ParamDecl != nullptr);
- setStorageLocation(*ParamDecl, createObject(*ParamDecl, nullptr));
- }
- }
-
- if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(&DeclCtx)) {
- auto *Parent = MethodDecl->getParent();
- assert(Parent != nullptr);
-
- if (Parent->isLambda()) {
- for (auto Capture : Parent->captures()) {
- if (Capture.capturesVariable()) {
- const auto *VarDecl = Capture.getCapturedVar();
- assert(VarDecl != nullptr);
- setStorageLocation(*VarDecl, createObject(*VarDecl, nullptr));
- } else if (Capture.capturesThis()) {
- const auto *SurroundingMethodDecl =
- cast<CXXMethodDecl>(DeclCtx.getNonClosureAncestor());
- QualType ThisPointeeType =
- SurroundingMethodDecl->getFunctionObjectParameterType();
- ThisPointeeLoc =
- &cast<RecordValue>(createValue(ThisPointeeType))->getLoc();
- }
- }
- } else if (MethodDecl->isImplicitObjectMemberFunction()) {
- QualType ThisPointeeType = MethodDecl->getFunctionObjectParameterType();
- ThisPointeeLoc =
- &cast<RecordValue>(createValue(ThisPointeeType))->getLoc();
- }
- }
-}
-
bool Environment::canDescend(unsigned MaxDepth,
const DeclContext *Callee) const {
return CallStack.size() <= MaxDepth && !llvm::is_contained(CallStack, Callee);
@@ -727,10 +690,6 @@ StorageLocation *Environment::getStorageLocation(const Expr &E) const {
return getStorageLocationInternal(E);
}
-RecordStorageLocation *Environment::getThisPointeeStorageLocation() const {
- return ThisPointeeLoc;
-}
-
RecordStorageLocation &
Environment::getResultObjectLocation(const Expr &RecordPRValue) {
assert(RecordPRValue.getType()->isRecordType());
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index ade8c84f19366f4..462a45ef01e1775 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -492,6 +492,56 @@ transferCFGBlock(const CFGBlock &Block, AnalysisContext &AC,
return State;
}
+static Environment initializeEnvironment(const Environment &InitEnv) {
+ Environment ResultEnv = InitEnv.fork();
+
+ const DeclContext *DeclCtx = ResultEnv.getDeclCtx();
+ if (DeclCtx == nullptr)
+ return ResultEnv;
+
+ if (const auto *FuncDecl = dyn_cast<FunctionDecl>(DeclCtx)) {
+ assert(FuncDecl->getBody() != nullptr);
+
+ ResultEnv.initFieldsGlobalsAndFuncs(FuncDecl);
+
+ for (const auto *ParamDecl : FuncDecl->parameters()) {
+ assert(ParamDecl != nullptr);
+ ResultEnv.setStorageLocation(*ParamDecl,
+ ResultEnv.createObject(*ParamDecl, nullptr));
+ }
+ }
+
+ if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(DeclCtx)) {
+ auto *Parent = MethodDecl->getParent();
+ assert(Parent != nullptr);
+
+ if (Parent->isLambda()) {
+ for (auto Capture : Parent->captures()) {
+ if (Capture.capturesVariable()) {
+ const auto *VarDecl = Capture.getCapturedVar();
+ assert(VarDecl != nullptr);
+ ResultEnv.setStorageLocation(
+ *VarDecl, ResultEnv.createObject(*VarDecl, nullptr));
+ } else if (Capture.capturesThis()) {
+ const auto *SurroundingMethodDecl =
+ cast<CXXMethodDecl>(DeclCtx->getNonClosureAncestor());
+ QualType ThisPointeeType =
+ SurroundingMethodDecl->getFunctionObjectParameterType();
+ ResultEnv.setThisPointeeStorageLocation(
+ cast<RecordValue>(ResultEnv.createValue(ThisPointeeType))
+ ->getLoc());
+ }
+ }
+ } else if (MethodDecl->isImplicitObjectMemberFunction()) {
+ QualType ThisPointeeType = MethodDecl->getFunctionObjectParameterType();
+ ResultEnv.setThisPointeeStorageLocation(
+ cast<RecordValue>(ResultEnv.createValue(ThisPointeeType))->getLoc());
+ }
+ }
+
+ return ResultEnv;
+}
+
llvm::Expected<std::vector<std::optional<TypeErasedDataflowAnalysisState>>>
runTypeErasedDataflowAnalysis(
const ControlFlowContext &CFCtx, TypeErasedDataflowAnalysis &Analysis,
@@ -501,6 +551,13 @@ runTypeErasedDataflowAnalysis(
PostVisitCFG) {
PrettyStackTraceAnalysis CrashInfo(CFCtx, "runTypeErasedDataflowAnalysis");
+ auto MaybeStartingEnv =
+ InitEnv.callStackSize() == 1
+ ? std::make_optional<Environment>(initializeEnvironment(InitEnv))
+ : std::nullopt;
+ const Environment &StartingEnv =
+ MaybeStartingEnv ? *MaybeStartingEnv : InitEnv;
+
const clang::CFG &CFG = CFCtx.getCFG();
PostOrderCFGView POV(&CFG);
ForwardDataflowWorklist Worklist(CFG, &POV);
@@ -511,10 +568,10 @@ runTypeErasedDataflowAnalysis(
// The entry basic block doesn't contain statements so it can be skipped.
const CFGBlock &Entry = CFG.getEntry();
BlockStates[Entry.getBlockID()] = {Analysis.typeErasedInitialElement(),
- InitEnv.fork()};
+ StartingEnv.fork()};
Worklist.enqueueSuccessors(&Entry);
- AnalysisContext AC(CFCtx, Analysis, InitEnv, BlockStates);
+ AnalysisContext AC(CFCtx, Analysis, StartingEnv, BlockStates);
// Bugs in lattices and transfer functions can prevent the analysis from
// converging. To limit the damage (infinite loops) that these bugs can cause,
diff --git a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
index ff6cbec5d20b744..595d05e2ba5d1bb 100644
--- a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
@@ -96,6 +96,7 @@ TEST_F(EnvironmentTest, CreateValueRecursiveType) {
// Verify that the struct and the field (`R`) with first appearance of the
// type is created successfully.
Environment Env(DAContext, *Fun);
+ Env.initFieldsGlobalsAndFuncs(Fun);
auto &SLoc = cast<RecordStorageLocation>(Env.createObject(Ty));
PointerValue *PV = cast_or_null<PointerValue>(getFieldValue(&SLoc, *R, Env));
EXPECT_THAT(PV, NotNull());
@@ -175,6 +176,7 @@ TEST_F(EnvironmentTest, InitGlobalVarsFun) {
// Verify the global variable is populated when we analyze `Target`.
Environment Env(DAContext, *Fun);
+ Env.initFieldsGlobalsAndFuncs(Fun);
EXPECT_THAT(Env.getValue(*Var), NotNull());
}
@@ -225,6 +227,7 @@ TEST_F(EnvironmentTest, IncludeFieldsFromDefaultInitializers) {
// Verify that the `X` field of `S` is populated when analyzing the
// constructor, even though it is not referenced directly in the constructor.
Environment Env(DAContext, *Constructor);
+ Env.initFieldsGlobalsAndFuncs(Constructor);
auto &Loc = cast<RecordStorageLocation>(Env.createObject(QTy));
EXPECT_THAT(getFieldValue(&Loc, *XDecl, Env), NotNull());
}
@@ -268,6 +271,7 @@ TEST_F(EnvironmentTest, InitGlobalVarsFieldFun) {
// Verify the global variable is populated when we analyze `Target`.
Environment Env(DAContext, *Fun);
+ Env.initFieldsGlobalsAndFuncs(Fun);
const auto *GlobalLoc =
cast<RecordStorageLocation>(Env.getStorageLocation(*GlobalDecl));
auto *BarVal = getFieldValue(GlobalLoc, *BarDecl, Env);
@@ -303,6 +307,7 @@ TEST_F(EnvironmentTest, InitGlobalVarsConstructor) {
// Verify the global variable is populated when we analyze `Target`.
Environment Env(DAContext, *Ctor);
+ Env.initFieldsGlobalsAndFuncs(Ctor);
EXPECT_THAT(Env.getValue(*Var), NotNull());
}
>From 44270dc5830b4c7e4cdacd288fab4e5d8f82ce26 Mon Sep 17 00:00:00 2001
From: Martin Braenne <mboehme at google.com>
Date: Wed, 29 Nov 2023 21:33:14 +0000
Subject: [PATCH 2/4] [clang][dataflow] Add synthetic fields to
`RecordStorageLocation`.
---
.../FlowSensitive/DataflowAnalysisContext.h | 46 +++++++++++++++
.../FlowSensitive/DataflowEnvironment.h | 3 +-
.../clang/Analysis/FlowSensitive/RecordOps.h | 17 +++---
.../Analysis/FlowSensitive/StorageLocation.h | 32 ++++++++--
.../FlowSensitive/DataflowAnalysisContext.cpp | 40 ++++++++++++-
.../FlowSensitive/DataflowEnvironment.cpp | 12 +++-
.../lib/Analysis/FlowSensitive/HTMLLogger.cpp | 4 ++
.../lib/Analysis/FlowSensitive/RecordOps.cpp | 24 ++++++++
clang/lib/Analysis/FlowSensitive/Transfer.cpp | 26 ++++----
.../Analysis/FlowSensitive/RecordOpsTest.cpp | 59 ++++++++++++++++---
.../Analysis/FlowSensitive/TestingSupport.cpp | 9 ++-
.../Analysis/FlowSensitive/TestingSupport.h | 4 +-
12 files changed, 231 insertions(+), 45 deletions(-)
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
index c1aa8484817a9d9..20e45cc27b01fa8 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
@@ -58,6 +58,10 @@ using FieldSet = llvm::SmallSetVector<const FieldDecl *, 4>;
/// Returns the set of all fields in the type.
FieldSet getObjectFields(QualType Type);
+/// Returns whether `Fields` and `FieldLocs` contain the same fields.
+bool containsSameFields(const FieldSet &Fields,
+ const RecordStorageLocation::FieldToLoc &FieldLocs);
+
struct ContextSensitiveOptions {
/// The maximum depth to analyze. A value of zero is equivalent to disabling
/// context-sensitive analysis entirely.
@@ -92,11 +96,39 @@ class DataflowAnalysisContext {
/*Logger=*/nullptr});
~DataflowAnalysisContext();
+ /// Sets a callback that returns the names and types of the synthetic fields
+ /// to add to a `RecordStorageLocation` of a given type.
+ /// Typically, this is called from the constructor of a `DataflowAnalysis`
+ ///
+ /// To maintain the invariant that all `RecordStorageLocation`s of a given
+ /// type have the same fields:
+ /// * The callback must always return the same result for a given type
+ /// * `setSyntheticFieldCallback()` must be called before any
+ // `RecordStorageLocation`s are created.
+ void setSyntheticFieldCallback(
+ std::function<llvm::StringMap<QualType>(QualType)> CB) {
+ assert(!RecordStorageLocationCreated);
+ SyntheticFieldCallback = CB;
+ }
+
/// Returns a new storage location appropriate for `Type`.
///
/// A null `Type` is interpreted as the pointee type of `std::nullptr_t`.
StorageLocation &createStorageLocation(QualType Type);
+ /// Creates a `RecordStorageLocation` for the given type and with the given
+ /// fields.
+ ///
+ /// Requirements:
+ ///
+ /// `FieldLocs` must contain exactly the fields returned by
+ /// `getModeledFields(Type)`.
+ /// `SyntheticFields` must contain exactly the fields returned by
+ /// `getSyntheticFields(Type)`.
+ RecordStorageLocation &createRecordStorageLocation(
+ QualType Type, RecordStorageLocation::FieldToLoc FieldLocs,
+ RecordStorageLocation::SyntheticFieldMap SyntheticFields);
+
/// Returns a stable storage location for `D`.
StorageLocation &getStableStorageLocation(const ValueDecl &D);
@@ -169,6 +201,15 @@ class DataflowAnalysisContext {
/// context.
FieldSet getModeledFields(QualType Type);
+ /// Returns the names and types of the synthetic fields for the given record
+ /// type.
+ llvm::StringMap<QualType> getSyntheticFields(QualType Type) {
+ assert(Type->isRecordType());
+ if (SyntheticFieldCallback)
+ return SyntheticFieldCallback(Type);
+ return {};
+ }
+
private:
friend class Environment;
@@ -250,6 +291,11 @@ class DataflowAnalysisContext {
FieldSet ModeledFields;
std::unique_ptr<Logger> LogOwner; // If created via flags.
+
+ std::function<llvm::StringMap<QualType>(QualType)> SyntheticFieldCallback;
+
+ /// Has any `RecordStorageLocation` been created yet?
+ bool RecordStorageLocationCreated = false;
};
} // namespace dataflow
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index 7026f289d62403c..8502f4da7e5414f 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -379,7 +379,8 @@ class Environment {
/// storage locations and values for indirections until it finds a
/// non-pointer/non-reference type.
///
- /// If `Type` is a class, struct, or union type, calls `setValue()` to
+ /// If `Type` is a class, struct, or union type, creates values for all
+ /// modeled fields (including synthetic fields) and calls `setValue()` to
/// associate the `RecordValue` with its storage location
/// (`RecordValue::getLoc()`).
///
diff --git a/clang/include/clang/Analysis/FlowSensitive/RecordOps.h b/clang/include/clang/Analysis/FlowSensitive/RecordOps.h
index f007a947a82f132..7b87840d626b4b6 100644
--- a/clang/include/clang/Analysis/FlowSensitive/RecordOps.h
+++ b/clang/include/clang/Analysis/FlowSensitive/RecordOps.h
@@ -21,10 +21,10 @@ namespace dataflow {
/// Copies a record (struct, class, or union) from `Src` to `Dst`.
///
-/// This performs a deep copy, i.e. it copies every field and recurses on
-/// fields of record type. It also copies properties from the `RecordValue`
-/// associated with `Src` to the `RecordValue` associated with `Dst` (if these
-/// `RecordValue`s exist).
+/// This performs a deep copy, i.e. it copies every field (including synthetic
+/// fields) and recurses on fields of record type. It also copies properties
+/// from the `RecordValue` associated with `Src` to the `RecordValue` associated
+/// with `Dst` (if these `RecordValue`s exist).
///
/// If there is a `RecordValue` associated with `Dst` in the environment, this
/// function creates a new `RecordValue` and associates it with `Dst`; clients
@@ -47,10 +47,11 @@ void copyRecord(RecordStorageLocation &Src, RecordStorageLocation &Dst,
/// retrieved from `Env2`. A convenience overload retrieves values for `Loc1`
/// and `Loc2` from the same environment.
///
-/// This performs a deep comparison, i.e. it compares every field and recurses
-/// on fields of record type. Fields of reference type compare equal if they
-/// refer to the same storage location. If `RecordValue`s are associated with
-/// `Loc1` and `Loc2`, it also compares the properties on those `RecordValue`s.
+/// This performs a deep comparison, i.e. it compares every field (including
+/// synthetic fields) and recurses on fields of record type. Fields of reference
+/// type compare equal if they refer to the same storage location. If
+/// `RecordValue`s are associated with `Loc1` and Loc2`, it also compares the
+/// properties on those `RecordValue`s.
///
/// Note on how to interpret the result:
/// - If this returns true, the records are guaranteed to be equal at runtime.
diff --git a/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h b/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
index 89d2bbfbb69f9fb..b7203378189d6bf 100644
--- a/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
+++ b/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
@@ -73,7 +73,13 @@ class ScalarStorageLocation final : public StorageLocation {
///
/// Contains storage locations for all modeled fields of the record (also
/// referred to as "children"). The child map is flat, so accessible members of
-/// the base class are directly accesible as children of this location.
+/// the base class are directly accessible as children of this location.
+///
+/// Record storage locations may also contain so-called synthetic fields. These
+/// are typically used to the internal state of a class (e.g. the value stored
+/// in a `std::optional`) without having to depend on that class's
+/// implementation details. All `RecordStorageLocation`s of a given type should
+/// have the same synthetic fields.
///
/// The storage location for a field of reference type may be null. This
/// typically occurs in one of two situations:
@@ -88,12 +94,12 @@ class ScalarStorageLocation final : public StorageLocation {
class RecordStorageLocation final : public StorageLocation {
public:
using FieldToLoc = llvm::DenseMap<const ValueDecl *, StorageLocation *>;
+ using SyntheticFieldMap = llvm::StringMap<StorageLocation *>;
- explicit RecordStorageLocation(QualType Type)
- : RecordStorageLocation(Type, FieldToLoc()) {}
-
- RecordStorageLocation(QualType Type, FieldToLoc TheChildren)
- : StorageLocation(Kind::Record, Type), Children(std::move(TheChildren)) {
+ RecordStorageLocation(QualType Type, FieldToLoc TheChildren,
+ SyntheticFieldMap TheSyntheticFields)
+ : StorageLocation(Kind::Record, Type), Children(std::move(TheChildren)),
+ SyntheticFields(std::move(TheSyntheticFields)) {
assert(!Type.isNull());
assert(Type->isRecordType());
assert([this] {
@@ -133,6 +139,19 @@ class RecordStorageLocation final : public StorageLocation {
return It->second;
}
+ /// Returns the storage location for the synthetic field `Name`.
+ /// The synthetic field must exist.
+ StorageLocation &getSyntheticField(llvm::StringRef Name) const {
+ StorageLocation *Loc = SyntheticFields.lookup(Name);
+ assert(Loc != nullptr);
+ return *Loc;
+ }
+
+ llvm::iterator_range<SyntheticFieldMap::const_iterator>
+ synthetic_fields() const {
+ return {SyntheticFields.begin(), SyntheticFields.end()};
+ }
+
/// Changes the child storage location for a field `D` of reference type.
/// All other fields cannot change their storage location and always retain
/// the storage location passed to the `RecordStorageLocation` constructor.
@@ -151,6 +170,7 @@ class RecordStorageLocation final : public StorageLocation {
private:
FieldToLoc Children;
+ SyntheticFieldMap SyntheticFields;
};
} // namespace dataflow
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp b/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
index 0a2fcd4ad695a30..fa114979c8e3263 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
@@ -68,11 +68,38 @@ StorageLocation &DataflowAnalysisContext::createStorageLocation(QualType Type) {
else
FieldLocs.insert({Field, &createStorageLocation(
Field->getType().getNonReferenceType())});
- return arena().create<RecordStorageLocation>(Type, std::move(FieldLocs));
+
+ RecordStorageLocation::SyntheticFieldMap SyntheticFields;
+ for (const auto &Entry : getSyntheticFields(Type))
+ SyntheticFields.insert(
+ {Entry.getKey(),
+ &createStorageLocation(Entry.getValue().getNonReferenceType())});
+
+ return createRecordStorageLocation(Type, std::move(FieldLocs),
+ std::move(SyntheticFields));
}
return arena().create<ScalarStorageLocation>(Type);
}
+// Returns the keys for a given `StringMap`.
+// Can't use `StringSet` as the return type as it doesn't support `operator==`.
+template <typename T>
+static llvm::DenseSet<llvm::StringRef> getKeys(const llvm::StringMap<T> &Map) {
+ return llvm::DenseSet<llvm::StringRef>(Map.keys().begin(), Map.keys().end());
+}
+
+RecordStorageLocation &DataflowAnalysisContext::createRecordStorageLocation(
+ QualType Type, RecordStorageLocation::FieldToLoc FieldLocs,
+ RecordStorageLocation::SyntheticFieldMap SyntheticFields) {
+ assert(Type->isRecordType());
+ assert(containsSameFields(getModeledFields(Type), FieldLocs));
+ assert(getKeys(getSyntheticFields(Type)) == getKeys(SyntheticFields));
+
+ RecordStorageLocationCreated = true;
+ return arena().create<RecordStorageLocation>(Type, std::move(FieldLocs),
+ std::move(SyntheticFields));
+}
+
StorageLocation &
DataflowAnalysisContext::getStableStorageLocation(const ValueDecl &D) {
if (auto *Loc = DeclToLoc.lookup(&D))
@@ -367,3 +394,14 @@ clang::dataflow::FieldSet clang::dataflow::getObjectFields(QualType Type) {
getFieldsFromClassHierarchy(Type, Fields);
return Fields;
}
+
+bool clang::dataflow::containsSameFields(
+ const clang::dataflow::FieldSet &Fields,
+ const clang::dataflow::RecordStorageLocation::FieldToLoc &FieldLocs) {
+ if (Fields.size() != FieldLocs.size())
+ return false;
+ for ([[maybe_unused]] auto [Field, Loc] : FieldLocs)
+ if (!Fields.contains(cast_or_null<FieldDecl>(Field)))
+ return false;
+ return true;
+}
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 131a68c03ed7563..983375da9cc3a83 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -811,8 +811,16 @@ Value *Environment::createValueUnlessSelfReferential(
CreatedValuesCount)});
}
- RecordStorageLocation &Loc =
- arena().create<RecordStorageLocation>(Type, std::move(FieldLocs));
+ RecordStorageLocation::SyntheticFieldMap SyntheticFieldLocs;
+ for (const auto &Entry : DACtx->getSyntheticFields(Type)) {
+ SyntheticFieldLocs.insert(
+ {Entry.getKey(),
+ &createLocAndMaybeValue(Entry.getValue(), Visited, Depth + 1,
+ CreatedValuesCount)});
+ }
+
+ RecordStorageLocation &Loc = DACtx->createRecordStorageLocation(
+ Type, std::move(FieldLocs), std::move(SyntheticFieldLocs));
RecordValue &RecordVal = create<RecordValue>(Loc);
// As we already have a storage location for the `RecordValue`, we can and
diff --git a/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp b/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
index 8329367098b1dbb..2cf3f321fcf4546 100644
--- a/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
+++ b/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
@@ -136,6 +136,10 @@ class ModelDumper {
if (Value *Val = Env.getValue(*Child.second))
dump(*Val);
});
+
+ for (const auto &Prop : RLoc->synthetic_fields())
+ JOS.attributeObject(("sf:" + Prop.first()).str(),
+ [&] { dump(*Prop.second); });
}
}
diff --git a/clang/lib/Analysis/FlowSensitive/RecordOps.cpp b/clang/lib/Analysis/FlowSensitive/RecordOps.cpp
index 38638f8a9e5892d..47a713472bc39f1 100644
--- a/clang/lib/Analysis/FlowSensitive/RecordOps.cpp
+++ b/clang/lib/Analysis/FlowSensitive/RecordOps.cpp
@@ -54,6 +54,18 @@ void clang::dataflow::copyRecord(RecordStorageLocation &Src,
}
}
+ for (const auto &[Name, PropLocSrc] : Src.synthetic_fields()) {
+ if (PropLocSrc->getType()->isRecordType()) {
+ copyRecord(*cast<RecordStorageLocation>(PropLocSrc),
+ cast<RecordStorageLocation>(Dst.getSyntheticField(Name)), Env);
+ } else {
+ if (Value *Val = Env.getValue(*PropLocSrc))
+ Env.setValue(Dst.getSyntheticField(Name), *Val);
+ else
+ Env.clearValue(Dst.getSyntheticField(Name));
+ }
+ }
+
RecordValue *SrcVal = cast_or_null<RecordValue>(Env.getValue(Src));
RecordValue *DstVal = cast_or_null<RecordValue>(Env.getValue(Dst));
@@ -101,6 +113,18 @@ bool clang::dataflow::recordsEqual(const RecordStorageLocation &Loc1,
}
}
+ for (const auto &[Name, PropLoc1] : Loc1.synthetic_fields()) {
+ if (PropLoc1->getType()->isRecordType()) {
+ if (!recordsEqual(
+ *cast<RecordStorageLocation>(PropLoc1), Env1,
+ cast<RecordStorageLocation>(Loc2.getSyntheticField(Name)), Env2))
+ return false;
+ } else if (Env1.getValue(*PropLoc1) !=
+ Env2.getValue(Loc2.getSyntheticField(Name))) {
+ return false;
+ }
+ }
+
llvm::StringMap<Value *> Props1, Props2;
if (RecordValue *Val1 = cast_or_null<RecordValue>(Env1.getValue(Loc1)))
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index 4343af790081301..bbf5f12359bc70e 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -703,20 +703,18 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
// `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.
- assert([&]() {
- auto ModeledFields =
- Env.getDataflowAnalysisContext().getModeledFields(Type);
- if (ModeledFields.size() != FieldLocs.size())
- return false;
- for ([[maybe_unused]] auto [Field, Loc] : FieldLocs)
- if (!ModeledFields.contains(cast_or_null<FieldDecl>(Field)))
- return false;
- return true;
- }());
-
- auto &Loc =
- Env.getDataflowAnalysisContext().arena().create<RecordStorageLocation>(
- Type, std::move(FieldLocs));
+ assert(containsSameFields(
+ Env.getDataflowAnalysisContext().getModeledFields(Type), FieldLocs));
+
+ RecordStorageLocation::SyntheticFieldMap SyntheticFieldLocs;
+ for (const auto &Entry :
+ Env.getDataflowAnalysisContext().getSyntheticFields(Type)) {
+ SyntheticFieldLocs.insert(
+ {Entry.getKey(), &Env.createObject(Entry.getValue())});
+ }
+
+ auto &Loc = Env.getDataflowAnalysisContext().createRecordStorageLocation(
+ Type, std::move(FieldLocs), std::move(SyntheticFieldLocs));
RecordValue &RecordVal = Env.create<RecordValue>(Loc);
Env.setValue(Loc, RecordVal);
diff --git a/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp b/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp
index 6ba1c2ebd833d47..84fe675c32c2d0b 100644
--- a/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp
@@ -18,17 +18,24 @@ namespace {
void runDataflow(
llvm::StringRef Code,
+ std::function<llvm::StringMap<QualType>(QualType)> SyntheticFieldCallback,
std::function<
void(const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &,
ASTContext &)>
- VerifyResults,
- LangStandard::Kind Std = LangStandard::lang_cxx17,
- llvm::StringRef TargetFun = "target") {
- ASSERT_THAT_ERROR(
- checkDataflowWithNoopAnalysis(Code, VerifyResults,
- DataflowAnalysisOptions{BuiltinOptions{}},
- Std, TargetFun),
- llvm::Succeeded());
+ VerifyResults) {
+ ASSERT_THAT_ERROR(checkDataflowWithNoopAnalysis(
+ Code, ast_matchers::hasName("target"), VerifyResults,
+ {BuiltinOptions()}, LangStandard::lang_cxx17,
+ SyntheticFieldCallback),
+ llvm::Succeeded());
+}
+
+const FieldDecl *getFieldNamed(RecordDecl *RD, llvm::StringRef Name) {
+ for (const FieldDecl *FD : RD->fields())
+ if (FD->getName() == Name)
+ return FD;
+ assert(false);
+ return nullptr;
}
TEST(RecordOpsTest, CopyRecord) {
@@ -49,6 +56,13 @@ TEST(RecordOpsTest, CopyRecord) {
)";
runDataflow(
Code,
+ [](QualType Ty) -> llvm::StringMap<QualType> {
+ if (Ty.getAsString() != "S")
+ return {};
+ QualType IntTy =
+ getFieldNamed(Ty->getAsRecordDecl(), "outer_int")->getType();
+ return {{"synth_int", IntTy}};
+ },
[](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
ASTContext &ASTCtx) {
Environment Env = getEnvironmentAtAnnotation(Results, "p").fork();
@@ -68,6 +82,8 @@ TEST(RecordOpsTest, CopyRecord) {
EXPECT_NE(S1.getChild(*RefDecl), S2.getChild(*RefDecl));
EXPECT_NE(getFieldValue(&Inner1, *InnerIntDecl, Env),
getFieldValue(&Inner2, *InnerIntDecl, Env));
+ EXPECT_NE(Env.getValue(S1.getSyntheticField("synth_int")),
+ Env.getValue(S2.getSyntheticField("synth_int")));
auto *S1Val = cast<RecordValue>(Env.getValue(S1));
auto *S2Val = cast<RecordValue>(Env.getValue(S2));
@@ -82,6 +98,8 @@ TEST(RecordOpsTest, CopyRecord) {
EXPECT_EQ(S1.getChild(*RefDecl), S2.getChild(*RefDecl));
EXPECT_EQ(getFieldValue(&Inner1, *InnerIntDecl, Env),
getFieldValue(&Inner2, *InnerIntDecl, Env));
+ EXPECT_EQ(Env.getValue(S1.getSyntheticField("synth_int")),
+ Env.getValue(S2.getSyntheticField("synth_int")));
S1Val = cast<RecordValue>(Env.getValue(S1));
S2Val = cast<RecordValue>(Env.getValue(S2));
@@ -109,6 +127,13 @@ TEST(RecordOpsTest, RecordsEqual) {
)";
runDataflow(
Code,
+ [](QualType Ty) -> llvm::StringMap<QualType> {
+ if (Ty.getAsString() != "S")
+ return {};
+ QualType IntTy =
+ getFieldNamed(Ty->getAsRecordDecl(), "outer_int")->getType();
+ return {{"synth_int", IntTy}};
+ },
[](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
ASTContext &ASTCtx) {
Environment Env = getEnvironmentAtAnnotation(Results, "p").fork();
@@ -122,6 +147,9 @@ TEST(RecordOpsTest, RecordsEqual) {
auto &S2 = getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "s2");
auto &Inner2 = *cast<RecordStorageLocation>(S2.getChild(*InnerDecl));
+ Env.setValue(S1.getSyntheticField("synth_int"),
+ Env.create<IntegerValue>());
+
cast<RecordValue>(Env.getValue(S1))
->setProperty("prop", Env.getBoolLiteralValue(true));
@@ -162,6 +190,19 @@ TEST(RecordOpsTest, RecordsEqual) {
copyRecord(S1, S2, Env);
EXPECT_TRUE(recordsEqual(S1, S2, Env));
+ // S2 has a different synth_int.
+ Env.setValue(S2.getSyntheticField("synth_int"),
+ Env.create<IntegerValue>());
+ EXPECT_FALSE(recordsEqual(S1, S2, Env));
+ copyRecord(S1, S2, Env);
+ EXPECT_TRUE(recordsEqual(S1, S2, Env));
+
+ // S2 doesn't have a value for synth_int.
+ Env.clearValue(S2.getSyntheticField("synth_int"));
+ EXPECT_FALSE(recordsEqual(S1, S2, Env));
+ copyRecord(S1, S2, Env);
+ EXPECT_TRUE(recordsEqual(S1, S2, Env));
+
// S1 and S2 have the same property with different values.
cast<RecordValue>(Env.getValue(S2))
->setProperty("prop", Env.getBoolLiteralValue(false));
@@ -209,7 +250,7 @@ TEST(TransferTest, CopyRecordFromDerivedToBase) {
}
)";
runDataflow(
- Code,
+ Code, /*SyntheticFieldCallback=*/{},
[](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
ASTContext &ASTCtx) {
Environment Env = getEnvironmentAtAnnotation(Results, "p").fork();
diff --git a/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp b/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
index e24ff25cb8292fb..3726f56fc824d5d 100644
--- a/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
@@ -173,7 +173,8 @@ llvm::Error test::checkDataflowWithNoopAnalysis(
void(const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &,
ASTContext &)>
VerifyResults,
- DataflowAnalysisOptions Options, LangStandard::Kind Std) {
+ DataflowAnalysisOptions Options, LangStandard::Kind Std,
+ std::function<llvm::StringMap<QualType>(QualType)> SyntheticFieldCallback) {
llvm::SmallVector<std::string, 3> ASTBuildArgs = {
// -fnodelayed-template-parsing is the default everywhere but on Windows.
// Set it explicitly so that tests behave the same on Windows as on other
@@ -183,8 +184,10 @@ llvm::Error test::checkDataflowWithNoopAnalysis(
std::string(LangStandard::getLangStandardForKind(Std).getName())};
AnalysisInputs<NoopAnalysis> AI(
Code, TargetFuncMatcher,
- [UseBuiltinModel = Options.BuiltinOpts.has_value()](ASTContext &C,
- Environment &Env) {
+ [UseBuiltinModel = Options.BuiltinOpts.has_value(),
+ &SyntheticFieldCallback](ASTContext &C, Environment &Env) {
+ Env.getDataflowAnalysisContext().setSyntheticFieldCallback(
+ std::move(SyntheticFieldCallback));
return NoopAnalysis(
C,
DataflowAnalysisOptions{
diff --git a/clang/unittests/Analysis/FlowSensitive/TestingSupport.h b/clang/unittests/Analysis/FlowSensitive/TestingSupport.h
index 100d78378695d3c..95ffcbd6f322ec7 100644
--- a/clang/unittests/Analysis/FlowSensitive/TestingSupport.h
+++ b/clang/unittests/Analysis/FlowSensitive/TestingSupport.h
@@ -420,7 +420,9 @@ llvm::Error checkDataflowWithNoopAnalysis(
ASTContext &)>
VerifyResults = [](const auto &, auto &) {},
DataflowAnalysisOptions Options = {BuiltinOptions()},
- LangStandard::Kind Std = LangStandard::lang_cxx17);
+ LangStandard::Kind Std = LangStandard::lang_cxx17,
+ std::function<llvm::StringMap<QualType>(QualType)> SyntheticFieldCallback =
+ {});
/// Returns the `ValueDecl` for the given identifier.
///
>From c49199a3f3e2199207921c0c9b128ffef1e3d53d Mon Sep 17 00:00:00 2001
From: Martin Braenne <mboehme at google.com>
Date: Wed, 29 Nov 2023 21:33:32 +0000
Subject: [PATCH 3/4] [clang][dataflow] Make UncheckedOptionalAccessModel use
synthetic fields.
---
.../Analysis/FlowSensitive/DataflowAnalysis.h | 18 +-
.../Models/UncheckedOptionalAccessModel.h | 13 +-
.../Models/UncheckedOptionalAccessModel.cpp | 378 +++++-------------
.../UncheckedOptionalAccessModelTest.cpp | 4 +-
4 files changed, 128 insertions(+), 285 deletions(-)
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
index c5f14cfcd4f7bee..1dffbe8a09c3609 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
@@ -234,6 +234,22 @@ runDataflowAnalysis(
return std::move(BlockStates);
}
+// Create an analysis class that is derived from `DataflowAnalysis`. This is an
+// SFINAE adapter that allows us to call two different variants of constructor
+// (either with or without the optional `Environment` parameter).
+// FIXME: Make all classes derived from `DataflowAnalysis` take an `Environment`
+// parameter in their constructor so that we can get rid of this abomination.
+template <typename AnalysisT>
+auto createAnalysis(ASTContext &ASTCtx, Environment &Env)
+ -> decltype(AnalysisT(ASTCtx, Env)) {
+ return AnalysisT(ASTCtx, Env);
+}
+template <typename AnalysisT>
+auto createAnalysis(ASTContext &ASTCtx, Environment &Env)
+ -> decltype(AnalysisT(ASTCtx)) {
+ return AnalysisT(ASTCtx);
+}
+
/// Runs a dataflow analysis over the given function and then runs `Diagnoser`
/// over the results. Returns a list of diagnostics for `FuncDecl` or an
/// error. Currently, errors can occur (at least) because the analysis requires
@@ -262,7 +278,7 @@ llvm::Expected<llvm::SmallVector<Diagnostic>> diagnoseFunction(
const WatchedLiteralsSolver *Solver = OwnedSolver.get();
DataflowAnalysisContext AnalysisContext(std::move(OwnedSolver));
Environment Env(AnalysisContext, FuncDecl);
- AnalysisT Analysis(ASTCtx);
+ AnalysisT Analysis = createAnalysis<AnalysisT>(ASTCtx, Env);
llvm::SmallVector<Diagnostic> Diagnostics;
if (llvm::Error Err =
runTypeErasedDataflowAnalysis(
diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
index aeaf75b2154222c..09eb8b93822612f 100644
--- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
+++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
@@ -45,7 +45,7 @@ struct UncheckedOptionalAccessModelOptions {
class UncheckedOptionalAccessModel
: public DataflowAnalysis<UncheckedOptionalAccessModel, NoopLattice> {
public:
- UncheckedOptionalAccessModel(ASTContext &Ctx);
+ UncheckedOptionalAccessModel(ASTContext &Ctx, dataflow::Environment &Env);
/// Returns a matcher for the optional classes covered by this model.
static ast_matchers::DeclarationMatcher optionalClassDecl();
@@ -54,17 +54,6 @@ class UncheckedOptionalAccessModel
void transfer(const CFGElement &Elt, NoopLattice &L, Environment &Env);
- ComparisonResult compare(QualType Type, const Value &Val1,
- const Environment &Env1, const Value &Val2,
- const Environment &Env2) override;
-
- bool merge(QualType Type, const Value &Val1, const Environment &Env1,
- const Value &Val2, const Environment &Env2, Value &MergedVal,
- Environment &MergedEnv) override;
-
- Value *widen(QualType Type, Value &Prev, const Environment &PrevEnv,
- Value &Current, Environment &CurrentEnv) override;
-
private:
CFGMatchSwitch<TransferState<NoopLattice>> TransferMatchSwitch;
};
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 55d0713639d90da..69ac2c2b82cff24 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -122,12 +122,6 @@ auto nulloptTypeDecl() {
auto hasNulloptType() { return hasType(nulloptTypeDecl()); }
-// `optional` or `nullopt_t`
-auto hasAnyOptionalType() {
- return hasType(hasUnqualifiedDesugaredType(
- recordType(hasDeclaration(anyOf(nulloptTypeDecl(), optionalClass())))));
-}
-
auto inPlaceClass() {
return recordDecl(hasAnyName("std::in_place_t", "absl::in_place_t",
"base::in_place_t", "folly::in_place_t"));
@@ -162,11 +156,6 @@ auto isOptionalValueOrConversionAssignment() {
argumentCountIs(2), hasArgument(1, unless(hasNulloptType())));
}
-auto isNulloptConstructor() {
- return cxxConstructExpr(hasNulloptType(), argumentCountIs(1),
- hasArgument(0, hasNulloptType()));
-}
-
auto isOptionalNulloptAssignment() {
return cxxOperatorCallExpr(hasOverloadedOperatorName("="),
callee(cxxMethodDecl(ofClass(optionalClass()))),
@@ -246,10 +235,19 @@ const Formula &forceBoolValue(Environment &Env, const Expr &Expr) {
return Value->formula();
}
+StorageLocation &locForHasValue(const RecordStorageLocation &OptionalLoc) {
+ return OptionalLoc.getSyntheticField("has_value");
+}
+
+StorageLocation &locForValue(const RecordStorageLocation &OptionalLoc) {
+ return OptionalLoc.getSyntheticField("value");
+}
+
/// Sets `HasValueVal` as the symbolic value that represents the "has_value"
-/// property of the optional value `OptionalVal`.
-void setHasValue(Value &OptionalVal, BoolValue &HasValueVal) {
- OptionalVal.setProperty("has_value", HasValueVal);
+/// property of the optional at `OptionalLoc`.
+void setHasValue(RecordStorageLocation &OptionalLoc, BoolValue &HasValueVal,
+ Environment &Env) {
+ Env.setValue(locForHasValue(OptionalLoc), HasValueVal);
}
/// Creates a symbolic value for an `optional` value at an existing storage
@@ -259,23 +257,22 @@ RecordValue &createOptionalValue(RecordStorageLocation &Loc,
BoolValue &HasValueVal, Environment &Env) {
auto &OptionalVal = Env.create<RecordValue>(Loc);
Env.setValue(Loc, OptionalVal);
- setHasValue(OptionalVal, HasValueVal);
+ setHasValue(Loc, HasValueVal, Env);
return OptionalVal;
}
/// Returns the symbolic value that represents the "has_value" property of the
-/// optional value `OptionalVal`. Returns null if `OptionalVal` is null.
-BoolValue *getHasValue(Environment &Env, Value *OptionalVal) {
- if (OptionalVal != nullptr) {
- auto *HasValueVal =
- cast_or_null<BoolValue>(OptionalVal->getProperty("has_value"));
- if (HasValueVal == nullptr) {
- HasValueVal = &Env.makeAtomicBoolValue();
- OptionalVal->setProperty("has_value", *HasValueVal);
- }
- return HasValueVal;
+/// optional at `OptionalLoc`. Returns null if `OptionalLoc` is null.
+BoolValue *getHasValue(Environment &Env, RecordStorageLocation *OptionalLoc) {
+ if (OptionalLoc == nullptr)
+ return nullptr;
+ StorageLocation &HasValueLoc = locForHasValue(*OptionalLoc);
+ auto *HasValueVal = cast_or_null<BoolValue>(Env.getValue(HasValueLoc));
+ if (HasValueVal == nullptr) {
+ HasValueVal = &Env.makeAtomicBoolValue();
+ Env.setValue(HasValueLoc, *HasValueVal);
}
- return nullptr;
+ return HasValueVal;
}
/// Returns true if and only if `Type` is an optional type.
@@ -302,155 +299,31 @@ int countOptionalWrappers(const ASTContext &ASTCtx, QualType Type) {
.getDesugaredType(ASTCtx));
}
-/// Tries to initialize the `optional`'s value (that is, contents), and return
-/// its location. Returns nullptr if the value can't be represented.
-StorageLocation *maybeInitializeOptionalValueMember(QualType Q,
- Value &OptionalVal,
- Environment &Env) {
- // The "value" property represents a synthetic field. As such, it needs
- // `StorageLocation`, like normal fields (and other variables). So, we model
- // it with a `PointerValue`, since that includes a storage location. Once
- // the property is set, it will be shared by all environments that access the
- // `Value` representing the optional (here, `OptionalVal`).
- if (auto *ValueProp = OptionalVal.getProperty("value")) {
- auto *ValuePtr = clang::cast<PointerValue>(ValueProp);
- auto &ValueLoc = ValuePtr->getPointeeLoc();
- if (Env.getValue(ValueLoc) != nullptr)
- return &ValueLoc;
-
- // The property was previously set, but the value has been lost. This can
- // happen in various situations, for example:
- // - Because of an environment merge (where the two environments mapped the
- // property to different values, which resulted in them both being
- // discarded).
- // - When two blocks in the CFG, with neither a dominator of the other,
- // visit the same optional value. (FIXME: This is something we can and
- // should fix -- see also the lengthy FIXME below.)
- // - Or even when a block is revisited during testing to collect
- // per-statement state.
- // FIXME: This situation means that the optional contents are not shared
- // between branches and the like. Practically, this lack of sharing
- // reduces the precision of the model when the contents are relevant to
- // the check, like another optional or a boolean that influences control
- // flow.
- if (ValueLoc.getType()->isRecordType()) {
- refreshRecordValue(cast<RecordStorageLocation>(ValueLoc), Env);
- return &ValueLoc;
- } else {
- auto *ValueVal = Env.createValue(ValueLoc.getType());
- if (ValueVal == nullptr)
- return nullptr;
- Env.setValue(ValueLoc, *ValueVal);
- return &ValueLoc;
- }
- }
-
- auto Ty = Q.getNonReferenceType();
- auto &ValueLoc = Env.createObject(Ty);
- auto &ValuePtr = Env.create<PointerValue>(ValueLoc);
- // FIXME:
- // The change we make to the `value` property below may become visible to
- // other blocks that aren't successors of the current block and therefore
- // don't see the change we made above mapping `ValueLoc` to `ValueVal`. For
- // example:
- //
- // void target(optional<int> oo, bool b) {
- // // `oo` is associated with a `RecordValue` here, which we will call
- // // `OptionalVal`.
- //
- // // The `has_value` property is set on `OptionalVal` (but not the
- // // `value` property yet).
- // if (!oo.has_value()) return;
- //
- // if (b) {
- // // Let's assume we transfer the `if` branch first.
- // //
- // // This causes us to call `maybeInitializeOptionalValueMember()`,
- // // which causes us to set the `value` property on `OptionalVal`
- // // (which had not been set until this point). This `value` property
- // // refers to a `PointerValue`, which in turn refers to a
- // // StorageLocation` that is associated to an `IntegerValue`.
- // oo.value();
- // } else {
- // // Let's assume we transfer the `else` branch after the `if` branch.
- // //
- // // We see the `value` property that the `if` branch set on
- // // `OptionalVal`, but in the environment for this block, the
- // // `StorageLocation` in the `PointerValue` is not associated with any
- // // `Value`.
- // oo.value();
- // }
- // }
- //
- // This situation is currently "saved" by the code above that checks whether
- // the `value` property is already set, and if, the `ValueLoc` is not
- // associated with a `ValueVal`, creates a new `ValueVal`.
- //
- // However, what we should really do is to make sure that the change to the
- // `value` property does not "leak" to other blocks that are not successors
- // of this block. To do this, instead of simply setting the `value` property
- // on the existing `OptionalVal`, we should create a new `Value` for the
- // optional, set the property on that, and associate the storage location that
- // is currently associated with the existing `OptionalVal` with the newly
- // created `Value` instead.
- OptionalVal.setProperty("value", ValuePtr);
- return &ValueLoc;
-}
-
-void initializeOptionalReference(const Expr *OptionalExpr,
- const MatchFinder::MatchResult &,
- LatticeTransferState &State) {
- if (auto *OptionalVal = State.Env.getValue(*OptionalExpr)) {
- if (OptionalVal->getProperty("has_value") == nullptr) {
- setHasValue(*OptionalVal, State.Env.makeAtomicBoolValue());
- }
+StorageLocation *getLocBehindPossiblePointer(const Expr &E,
+ const Environment &Env) {
+ if (E.isPRValue()) {
+ if (auto *PointerVal = dyn_cast_or_null<PointerValue>(Env.getValue(E)))
+ return &PointerVal->getPointeeLoc();
+ return nullptr;
}
-}
-
-/// Returns true if and only if `OptionalVal` is initialized and known to be
-/// empty in `Env`.
-bool isEmptyOptional(const Value &OptionalVal, const Environment &Env) {
- auto *HasValueVal =
- cast_or_null<BoolValue>(OptionalVal.getProperty("has_value"));
- return HasValueVal != nullptr &&
- Env.proves(Env.arena().makeNot(HasValueVal->formula()));
-}
-
-/// Returns true if and only if `OptionalVal` is initialized and known to be
-/// non-empty in `Env`.
-bool isNonEmptyOptional(const Value &OptionalVal, const Environment &Env) {
- auto *HasValueVal =
- cast_or_null<BoolValue>(OptionalVal.getProperty("has_value"));
- return HasValueVal != nullptr && Env.proves(HasValueVal->formula());
-}
-
-Value *getValueBehindPossiblePointer(const Expr &E, const Environment &Env) {
- Value *Val = Env.getValue(E);
- if (auto *PointerVal = dyn_cast_or_null<PointerValue>(Val))
- return Env.getValue(PointerVal->getPointeeLoc());
- return Val;
+ return Env.getStorageLocation(E);
}
void transferUnwrapCall(const Expr *UnwrapExpr, const Expr *ObjectExpr,
LatticeTransferState &State) {
- if (auto *OptionalVal =
- getValueBehindPossiblePointer(*ObjectExpr, State.Env)) {
+ if (auto *OptionalLoc = cast_or_null<RecordStorageLocation>(
+ getLocBehindPossiblePointer(*ObjectExpr, State.Env))) {
if (State.Env.getStorageLocation(*UnwrapExpr) == nullptr)
- if (auto *Loc = maybeInitializeOptionalValueMember(
- UnwrapExpr->getType(), *OptionalVal, State.Env))
- State.Env.setStorageLocation(*UnwrapExpr, *Loc);
+ State.Env.setStorageLocation(*UnwrapExpr, locForValue(*OptionalLoc));
}
}
void transferArrowOpCall(const Expr *UnwrapExpr, const Expr *ObjectExpr,
LatticeTransferState &State) {
- if (auto *OptionalVal =
- getValueBehindPossiblePointer(*ObjectExpr, State.Env)) {
- if (auto *Loc = maybeInitializeOptionalValueMember(
- UnwrapExpr->getType()->getPointeeType(), *OptionalVal, State.Env)) {
- State.Env.setValue(*UnwrapExpr, State.Env.create<PointerValue>(*Loc));
- }
- }
+ if (auto *OptionalLoc = cast_or_null<RecordStorageLocation>(
+ getLocBehindPossiblePointer(*ObjectExpr, State.Env)))
+ State.Env.setValue(
+ *UnwrapExpr, State.Env.create<PointerValue>(locForValue(*OptionalLoc)));
}
void transferMakeOptionalCall(const CallExpr *E,
@@ -465,8 +338,7 @@ void transferOptionalHasValueCall(const CXXMemberCallExpr *CallExpr,
const MatchFinder::MatchResult &,
LatticeTransferState &State) {
if (auto *HasValueVal = getHasValue(
- State.Env, getValueBehindPossiblePointer(
- *CallExpr->getImplicitObjectArgument(), State.Env))) {
+ State.Env, getImplicitObjectLocation(*CallExpr, State.Env))) {
State.Env.setValue(*CallExpr, *HasValueVal);
}
}
@@ -480,12 +352,11 @@ void transferValueOrImpl(
const Formula &HasValueVal)) {
auto &Env = State.Env;
- const auto *ObjectArgumentExpr =
- Result.Nodes.getNodeAs<clang::CXXMemberCallExpr>(ValueOrCallID)
- ->getImplicitObjectArgument();
+ const auto *MCE =
+ Result.Nodes.getNodeAs<clang::CXXMemberCallExpr>(ValueOrCallID);
- auto *HasValueVal = getHasValue(
- State.Env, getValueBehindPossiblePointer(*ObjectArgumentExpr, State.Env));
+ auto *HasValueVal =
+ getHasValue(State.Env, getImplicitObjectLocation(*MCE, State.Env));
if (HasValueVal == nullptr)
return;
@@ -578,7 +449,9 @@ BoolValue &valueOrConversionHasValue(const FunctionDecl &F, const Expr &E,
// This is a constructor/assignment call for `optional<T>` with argument of
// type `optional<U>` such that `T` is constructible from `U`.
- if (auto *HasValueVal = getHasValue(State.Env, State.Env.getValue(E)))
+ auto *Loc =
+ cast_or_null<RecordStorageLocation>(State.Env.getStorageLocation(E));
+ if (auto *HasValueVal = getHasValue(State.Env, Loc))
return *HasValueVal;
return State.Env.makeAtomicBoolValue();
}
@@ -645,11 +518,11 @@ void transferSwap(RecordStorageLocation *Loc1, RecordStorageLocation *Loc2,
// allows for local reasoning about the value. To avoid the above, we would
// need *lazy* value allocation.
// FIXME: allocate values lazily, instead of just creating a fresh value.
- BoolValue *BoolVal1 = getHasValue(Env, Env.getValue(*Loc1));
+ BoolValue *BoolVal1 = getHasValue(Env, Loc1);
if (BoolVal1 == nullptr)
BoolVal1 = &Env.makeAtomicBoolValue();
- BoolValue *BoolVal2 = getHasValue(Env, Env.getValue(*Loc2));
+ BoolValue *BoolVal2 = getHasValue(Env, Loc2);
if (BoolVal2 == nullptr)
BoolVal2 = &Env.makeAtomicBoolValue();
@@ -712,20 +585,26 @@ void transferOptionalAndOptionalCmp(const clang::CXXOperatorCallExpr *CmpExpr,
Environment &Env = State.Env;
auto &A = Env.arena();
auto *CmpValue = &forceBoolValue(Env, *CmpExpr);
- if (auto *LHasVal = getHasValue(Env, Env.getValue(*CmpExpr->getArg(0))))
- if (auto *RHasVal = getHasValue(Env, Env.getValue(*CmpExpr->getArg(1)))) {
+ auto *Arg0Loc = cast_or_null<RecordStorageLocation>(
+ Env.getStorageLocation(*CmpExpr->getArg(0)));
+ if (auto *LHasVal = getHasValue(Env, Arg0Loc)) {
+ auto *Arg1Loc = cast_or_null<RecordStorageLocation>(
+ Env.getStorageLocation(*CmpExpr->getArg(1)));
+ if (auto *RHasVal = getHasValue(Env, Arg1Loc)) {
if (CmpExpr->getOperator() == clang::OO_ExclaimEqual)
CmpValue = &A.makeNot(*CmpValue);
Env.assume(evaluateEquality(A, *CmpValue, LHasVal->formula(),
RHasVal->formula()));
}
+ }
}
void transferOptionalAndValueCmp(const clang::CXXOperatorCallExpr *CmpExpr,
const clang::Expr *E, Environment &Env) {
auto &A = Env.arena();
auto *CmpValue = &forceBoolValue(Env, *CmpExpr);
- if (auto *HasVal = getHasValue(Env, Env.getValue(*E))) {
+ auto *Loc = cast_or_null<RecordStorageLocation>(Env.getStorageLocation(*E));
+ if (auto *HasVal = getHasValue(Env, Loc)) {
if (CmpExpr->getOperator() == clang::OO_ExclaimEqual)
CmpValue = &A.makeNot(*CmpValue);
Env.assume(
@@ -733,6 +612,19 @@ void transferOptionalAndValueCmp(const clang::CXXOperatorCallExpr *CmpExpr,
}
}
+void transferOptionalAndNulloptCmp(const clang::CXXOperatorCallExpr *CmpExpr,
+ const clang::Expr *E, Environment &Env) {
+ auto &A = Env.arena();
+ auto *CmpValue = &forceBoolValue(Env, *CmpExpr);
+ auto *Loc = cast_or_null<RecordStorageLocation>(Env.getStorageLocation(*E));
+ if (auto *HasVal = getHasValue(Env, Loc)) {
+ if (CmpExpr->getOperator() == clang::OO_ExclaimEqual)
+ CmpValue = &A.makeNot(*CmpValue);
+ Env.assume(evaluateEquality(A, *CmpValue, HasVal->formula(),
+ A.makeLiteral(false)));
+ }
+}
+
std::optional<StatementMatcher>
ignorableOptional(const UncheckedOptionalAccessModelOptions &Options) {
if (Options.IgnoreSmartPointerDereference) {
@@ -762,12 +654,6 @@ auto buildTransferMatchSwitch() {
// lot of duplicated work (e.g. string comparisons), consider providing APIs
// that avoid it through memoization.
return CFGMatchSwitchBuilder<LatticeTransferState>()
- // Attach a symbolic "has_value" state to optional values that we see for
- // the first time.
- .CaseOfCFGStmt<Expr>(
- expr(anyOf(declRefExpr(), memberExpr()), hasOptionalType()),
- initializeOptionalReference)
-
// make_optional
.CaseOfCFGStmt<CallExpr>(isMakeOptionalCall(), transferMakeOptionalCall)
@@ -779,14 +665,6 @@ auto buildTransferMatchSwitch() {
constructOptionalValue(*E, State.Env,
State.Env.getBoolLiteralValue(true));
})
- // nullopt_t::nullopt_t
- .CaseOfCFGStmt<CXXConstructExpr>(
- isNulloptConstructor(),
- [](const CXXConstructExpr *E, const MatchFinder::MatchResult &,
- LatticeTransferState &State) {
- constructOptionalValue(*E, State.Env,
- State.Env.getBoolLiteralValue(false));
- })
// optional::optional(nullopt_t)
.CaseOfCFGStmt<CXXConstructExpr>(
isOptionalNulloptConstructor(),
@@ -887,18 +765,32 @@ auto buildTransferMatchSwitch() {
// Comparisons (==, !=):
.CaseOfCFGStmt<CXXOperatorCallExpr>(
- isComparisonOperatorCall(hasAnyOptionalType(), hasAnyOptionalType()),
+ isComparisonOperatorCall(hasOptionalType(), hasOptionalType()),
transferOptionalAndOptionalCmp)
.CaseOfCFGStmt<CXXOperatorCallExpr>(
- isComparisonOperatorCall(hasOptionalType(),
- unless(hasAnyOptionalType())),
+ isComparisonOperatorCall(hasOptionalType(), hasNulloptType()),
+ [](const clang::CXXOperatorCallExpr *Cmp,
+ const MatchFinder::MatchResult &, LatticeTransferState &State) {
+ transferOptionalAndNulloptCmp(Cmp, Cmp->getArg(0), State.Env);
+ })
+ .CaseOfCFGStmt<CXXOperatorCallExpr>(
+ isComparisonOperatorCall(hasNulloptType(), hasOptionalType()),
+ [](const clang::CXXOperatorCallExpr *Cmp,
+ const MatchFinder::MatchResult &, LatticeTransferState &State) {
+ transferOptionalAndNulloptCmp(Cmp, Cmp->getArg(1), State.Env);
+ })
+ .CaseOfCFGStmt<CXXOperatorCallExpr>(
+ isComparisonOperatorCall(
+ hasOptionalType(),
+ unless(anyOf(hasOptionalType(), hasNulloptType()))),
[](const clang::CXXOperatorCallExpr *Cmp,
const MatchFinder::MatchResult &, LatticeTransferState &State) {
transferOptionalAndValueCmp(Cmp, Cmp->getArg(0), State.Env);
})
.CaseOfCFGStmt<CXXOperatorCallExpr>(
- isComparisonOperatorCall(unless(hasAnyOptionalType()),
- hasOptionalType()),
+ isComparisonOperatorCall(
+ unless(anyOf(hasOptionalType(), hasNulloptType())),
+ hasOptionalType()),
[](const clang::CXXOperatorCallExpr *Cmp,
const MatchFinder::MatchResult &, LatticeTransferState &State) {
transferOptionalAndValueCmp(Cmp, Cmp->getArg(1), State.Env);
@@ -913,8 +805,9 @@ auto buildTransferMatchSwitch() {
llvm::SmallVector<SourceLocation> diagnoseUnwrapCall(const Expr *ObjectExpr,
const Environment &Env) {
- if (auto *OptionalVal = getValueBehindPossiblePointer(*ObjectExpr, Env)) {
- auto *Prop = OptionalVal->getProperty("has_value");
+ if (auto *OptionalLoc = cast_or_null<RecordStorageLocation>(
+ getLocBehindPossiblePointer(*ObjectExpr, Env))) {
+ auto *Prop = Env.getValue(locForHasValue(*OptionalLoc));
if (auto *HasValueVal = cast_or_null<BoolValue>(Prop)) {
if (Env.proves(HasValueVal->formula()))
return {};
@@ -960,9 +853,24 @@ UncheckedOptionalAccessModel::optionalClassDecl() {
return optionalClass();
}
-UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx)
+static QualType valueTypeFromOptionalType(QualType OptionalTy) {
+ auto *CTSD =
+ cast<ClassTemplateSpecializationDecl>(OptionalTy->getAsCXXRecordDecl());
+ return CTSD->getTemplateArgs()[0].getAsType();
+}
+
+UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx,
+ Environment &Env)
: DataflowAnalysis<UncheckedOptionalAccessModel, NoopLattice>(Ctx),
- TransferMatchSwitch(buildTransferMatchSwitch()) {}
+ TransferMatchSwitch(buildTransferMatchSwitch()) {
+ Env.getDataflowAnalysisContext().setSyntheticFieldCallback(
+ [&Ctx](QualType Ty) -> llvm::StringMap<QualType> {
+ if (!isOptionalType(Ty))
+ return {};
+ return {{"value", valueTypeFromOptionalType(Ty)},
+ {"has_value", Ctx.BoolTy}};
+ });
+}
void UncheckedOptionalAccessModel::transfer(const CFGElement &Elt,
NoopLattice &L, Environment &Env) {
@@ -970,76 +878,6 @@ void UncheckedOptionalAccessModel::transfer(const CFGElement &Elt,
TransferMatchSwitch(Elt, getASTContext(), State);
}
-ComparisonResult UncheckedOptionalAccessModel::compare(
- QualType Type, const Value &Val1, const Environment &Env1,
- const Value &Val2, const Environment &Env2) {
- if (!isOptionalType(Type))
- return ComparisonResult::Unknown;
- bool MustNonEmpty1 = isNonEmptyOptional(Val1, Env1);
- bool MustNonEmpty2 = isNonEmptyOptional(Val2, Env2);
- if (MustNonEmpty1 && MustNonEmpty2)
- return ComparisonResult::Same;
- // If exactly one is true, then they're different, no reason to check whether
- // they're definitely empty.
- if (MustNonEmpty1 || MustNonEmpty2)
- return ComparisonResult::Different;
- // Check if they're both definitely empty.
- return (isEmptyOptional(Val1, Env1) && isEmptyOptional(Val2, Env2))
- ? ComparisonResult::Same
- : ComparisonResult::Different;
-}
-
-bool UncheckedOptionalAccessModel::merge(QualType Type, const Value &Val1,
- const Environment &Env1,
- const Value &Val2,
- const Environment &Env2,
- Value &MergedVal,
- Environment &MergedEnv) {
- if (!isOptionalType(Type))
- return true;
- // FIXME: uses same approach as join for `BoolValues`. Requires non-const
- // values, though, so will require updating the interface.
- auto &HasValueVal = MergedEnv.makeAtomicBoolValue();
- bool MustNonEmpty1 = isNonEmptyOptional(Val1, Env1);
- bool MustNonEmpty2 = isNonEmptyOptional(Val2, Env2);
- if (MustNonEmpty1 && MustNonEmpty2)
- MergedEnv.assume(HasValueVal.formula());
- else if (
- // Only make the costly calls to `isEmptyOptional` if we got "unknown"
- // (false) for both calls to `isNonEmptyOptional`.
- !MustNonEmpty1 && !MustNonEmpty2 && isEmptyOptional(Val1, Env1) &&
- isEmptyOptional(Val2, Env2))
- MergedEnv.assume(MergedEnv.arena().makeNot(HasValueVal.formula()));
- setHasValue(MergedVal, HasValueVal);
- return true;
-}
-
-Value *UncheckedOptionalAccessModel::widen(QualType Type, Value &Prev,
- const Environment &PrevEnv,
- Value &Current,
- Environment &CurrentEnv) {
- switch (compare(Type, Prev, PrevEnv, Current, CurrentEnv)) {
- case ComparisonResult::Same:
- return &Prev;
- case ComparisonResult::Different:
- if (auto *PrevHasVal =
- cast_or_null<BoolValue>(Prev.getProperty("has_value"))) {
- if (isa<TopBoolValue>(PrevHasVal))
- return &Prev;
- }
- if (auto *CurrentHasVal =
- cast_or_null<BoolValue>(Current.getProperty("has_value"))) {
- if (isa<TopBoolValue>(CurrentHasVal))
- return &Current;
- }
- return &createOptionalValue(cast<RecordValue>(Current).getLoc(),
- CurrentEnv.makeTopBoolValue(), CurrentEnv);
- case ComparisonResult::Unknown:
- return nullptr;
- }
- llvm_unreachable("all cases covered in switch");
-}
-
UncheckedOptionalAccessDiagnoser::UncheckedOptionalAccessDiagnoser(
UncheckedOptionalAccessModelOptions Options)
: DiagnoseMatchSwitch(buildDiagnoseMatchSwitch(Options)) {}
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 76af8baba851839..73fb4063d92be93 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -1319,8 +1319,8 @@ class UncheckedOptionalAccessTest
llvm::Error Error = checkDataflow<UncheckedOptionalAccessModel>(
AnalysisInputs<UncheckedOptionalAccessModel>(
SourceCode, std::move(FuncMatcher),
- [](ASTContext &Ctx, Environment &) {
- return UncheckedOptionalAccessModel(Ctx);
+ [](ASTContext &Ctx, Environment &Env) {
+ return UncheckedOptionalAccessModel(Ctx, Env);
})
.withPostVisitCFG(
[&Diagnostics,
>From cea4a720feab54b70db479fe34ab6f9ba15d86fd Mon Sep 17 00:00:00 2001
From: Martin Braenne <mboehme at google.com>
Date: Fri, 1 Dec 2023 13:55:40 +0000
Subject: [PATCH 4/4] fixup! [clang][dataflow] Make
UncheckedOptionalAccessModel use synthetic fields.
---
.../FlowSensitive/DataflowEnvironment.h | 16 +++--
.../Analysis/FlowSensitive/StorageLocation.h | 4 +-
.../FlowSensitive/DataflowEnvironment.cpp | 43 ++++++++++++++
.../lib/Analysis/FlowSensitive/HTMLLogger.cpp | 6 +-
.../lib/Analysis/FlowSensitive/RecordOps.cpp | 16 ++---
.../TypeErasedDataflowAnalysis.cpp | 59 ++-----------------
.../FlowSensitive/DataflowEnvironmentTest.cpp | 10 ++--
7 files changed, 78 insertions(+), 76 deletions(-)
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index 8502f4da7e5414f..d7e39ab2616fd90 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -166,9 +166,14 @@ class Environment {
/// with a symbolic representation of the `this` pointee.
Environment(DataflowAnalysisContext &DACtx, const DeclContext &DeclCtx);
- /// Assigns storage locations and values to all global variables, fields
- /// and functions referenced in `FuncDecl`. `FuncDecl` must have a body.
- void initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl);
+ /// Assigns storage locations and values to all parameters, captures, global
+ /// variables, fields and functions referenced in the function currently being
+ /// analyzed.
+ ///
+ /// Requirements:
+ ///
+ /// The function must have a body.
+ void initialize();
/// Returns a new environment that is a copy of this one.
///
@@ -645,7 +650,10 @@ class Environment {
void pushCallInternal(const FunctionDecl *FuncDecl,
ArrayRef<const Expr *> Args);
-private:
+ /// Assigns storage locations and values to all global variables, fields
+ /// and functions referenced in `FuncDecl`. `FuncDecl` must have a body.
+ void initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl);
+
// `DACtx` is not null and not owned by this object.
DataflowAnalysisContext *DACtx;
diff --git a/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h b/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
index b7203378189d6bf..8fcc6a44027a020 100644
--- a/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
+++ b/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
@@ -76,8 +76,8 @@ class ScalarStorageLocation final : public StorageLocation {
/// the base class are directly accessible as children of this location.
///
/// Record storage locations may also contain so-called synthetic fields. These
-/// are typically used to the internal state of a class (e.g. the value stored
-/// in a `std::optional`) without having to depend on that class's
+/// are typically used to model the internal state of a class (e.g. the value
+/// stored in a `std::optional`) without having to depend on that class's
/// implementation details. All `RecordStorageLocation`s of a given type should
/// have the same synthetic fields.
///
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 983375da9cc3a83..042402a129d103e 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -377,6 +377,49 @@ Environment::Environment(DataflowAnalysisContext &DACtx,
CallStack.push_back(&DeclCtx);
}
+void Environment::initialize() {
+ const DeclContext *DeclCtx = getDeclCtx();
+ if (DeclCtx == nullptr)
+ return;
+
+ if (const auto *FuncDecl = dyn_cast<FunctionDecl>(DeclCtx)) {
+ assert(FuncDecl->getBody() != nullptr);
+
+ initFieldsGlobalsAndFuncs(FuncDecl);
+
+ for (const auto *ParamDecl : FuncDecl->parameters()) {
+ assert(ParamDecl != nullptr);
+ setStorageLocation(*ParamDecl, createObject(*ParamDecl, nullptr));
+ }
+ }
+
+ if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(DeclCtx)) {
+ auto *Parent = MethodDecl->getParent();
+ assert(Parent != nullptr);
+
+ if (Parent->isLambda()) {
+ for (auto Capture : Parent->captures()) {
+ if (Capture.capturesVariable()) {
+ const auto *VarDecl = Capture.getCapturedVar();
+ assert(VarDecl != nullptr);
+ setStorageLocation(*VarDecl, createObject(*VarDecl, nullptr));
+ } else if (Capture.capturesThis()) {
+ const auto *SurroundingMethodDecl =
+ cast<CXXMethodDecl>(DeclCtx->getNonClosureAncestor());
+ QualType ThisPointeeType =
+ SurroundingMethodDecl->getFunctionObjectParameterType();
+ setThisPointeeStorageLocation(
+ cast<RecordValue>(createValue(ThisPointeeType))->getLoc());
+ }
+ }
+ } else if (MethodDecl->isImplicitObjectMemberFunction()) {
+ QualType ThisPointeeType = MethodDecl->getFunctionObjectParameterType();
+ setThisPointeeStorageLocation(
+ cast<RecordValue>(createValue(ThisPointeeType))->getLoc());
+ }
+ }
+}
+
// FIXME: Add support for resetting globals after function calls to enable
// the implementation of sound analyses.
void Environment::initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl) {
diff --git a/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp b/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
index 2cf3f321fcf4546..7430ef599e032d0 100644
--- a/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
+++ b/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
@@ -137,9 +137,9 @@ class ModelDumper {
dump(*Val);
});
- for (const auto &Prop : RLoc->synthetic_fields())
- JOS.attributeObject(("sf:" + Prop.first()).str(),
- [&] { dump(*Prop.second); });
+ for (const auto &SyntheticField : RLoc->synthetic_fields())
+ JOS.attributeObject(("sf:" + SyntheticField.first()).str(),
+ [&] { dump(*SyntheticField.second); });
}
}
diff --git a/clang/lib/Analysis/FlowSensitive/RecordOps.cpp b/clang/lib/Analysis/FlowSensitive/RecordOps.cpp
index 47a713472bc39f1..caaf443382b02cf 100644
--- a/clang/lib/Analysis/FlowSensitive/RecordOps.cpp
+++ b/clang/lib/Analysis/FlowSensitive/RecordOps.cpp
@@ -54,12 +54,12 @@ void clang::dataflow::copyRecord(RecordStorageLocation &Src,
}
}
- for (const auto &[Name, PropLocSrc] : Src.synthetic_fields()) {
- if (PropLocSrc->getType()->isRecordType()) {
- copyRecord(*cast<RecordStorageLocation>(PropLocSrc),
+ for (const auto &[Name, SynthFieldLoc] : Src.synthetic_fields()) {
+ if (SynthFieldLoc->getType()->isRecordType()) {
+ copyRecord(*cast<RecordStorageLocation>(SynthFieldLoc),
cast<RecordStorageLocation>(Dst.getSyntheticField(Name)), Env);
} else {
- if (Value *Val = Env.getValue(*PropLocSrc))
+ if (Value *Val = Env.getValue(*SynthFieldLoc))
Env.setValue(Dst.getSyntheticField(Name), *Val);
else
Env.clearValue(Dst.getSyntheticField(Name));
@@ -113,13 +113,13 @@ bool clang::dataflow::recordsEqual(const RecordStorageLocation &Loc1,
}
}
- for (const auto &[Name, PropLoc1] : Loc1.synthetic_fields()) {
- if (PropLoc1->getType()->isRecordType()) {
+ for (const auto &[Name, SynthFieldLoc1] : Loc1.synthetic_fields()) {
+ if (SynthFieldLoc1->getType()->isRecordType()) {
if (!recordsEqual(
- *cast<RecordStorageLocation>(PropLoc1), Env1,
+ *cast<RecordStorageLocation>(SynthFieldLoc1), Env1,
cast<RecordStorageLocation>(Loc2.getSyntheticField(Name)), Env2))
return false;
- } else if (Env1.getValue(*PropLoc1) !=
+ } else if (Env1.getValue(*SynthFieldLoc1) !=
Env2.getValue(Loc2.getSyntheticField(Name))) {
return false;
}
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index 462a45ef01e1775..8c9360235da7c1f 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -492,56 +492,6 @@ transferCFGBlock(const CFGBlock &Block, AnalysisContext &AC,
return State;
}
-static Environment initializeEnvironment(const Environment &InitEnv) {
- Environment ResultEnv = InitEnv.fork();
-
- const DeclContext *DeclCtx = ResultEnv.getDeclCtx();
- if (DeclCtx == nullptr)
- return ResultEnv;
-
- if (const auto *FuncDecl = dyn_cast<FunctionDecl>(DeclCtx)) {
- assert(FuncDecl->getBody() != nullptr);
-
- ResultEnv.initFieldsGlobalsAndFuncs(FuncDecl);
-
- for (const auto *ParamDecl : FuncDecl->parameters()) {
- assert(ParamDecl != nullptr);
- ResultEnv.setStorageLocation(*ParamDecl,
- ResultEnv.createObject(*ParamDecl, nullptr));
- }
- }
-
- if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(DeclCtx)) {
- auto *Parent = MethodDecl->getParent();
- assert(Parent != nullptr);
-
- if (Parent->isLambda()) {
- for (auto Capture : Parent->captures()) {
- if (Capture.capturesVariable()) {
- const auto *VarDecl = Capture.getCapturedVar();
- assert(VarDecl != nullptr);
- ResultEnv.setStorageLocation(
- *VarDecl, ResultEnv.createObject(*VarDecl, nullptr));
- } else if (Capture.capturesThis()) {
- const auto *SurroundingMethodDecl =
- cast<CXXMethodDecl>(DeclCtx->getNonClosureAncestor());
- QualType ThisPointeeType =
- SurroundingMethodDecl->getFunctionObjectParameterType();
- ResultEnv.setThisPointeeStorageLocation(
- cast<RecordValue>(ResultEnv.createValue(ThisPointeeType))
- ->getLoc());
- }
- }
- } else if (MethodDecl->isImplicitObjectMemberFunction()) {
- QualType ThisPointeeType = MethodDecl->getFunctionObjectParameterType();
- ResultEnv.setThisPointeeStorageLocation(
- cast<RecordValue>(ResultEnv.createValue(ThisPointeeType))->getLoc());
- }
- }
-
- return ResultEnv;
-}
-
llvm::Expected<std::vector<std::optional<TypeErasedDataflowAnalysisState>>>
runTypeErasedDataflowAnalysis(
const ControlFlowContext &CFCtx, TypeErasedDataflowAnalysis &Analysis,
@@ -551,10 +501,11 @@ runTypeErasedDataflowAnalysis(
PostVisitCFG) {
PrettyStackTraceAnalysis CrashInfo(CFCtx, "runTypeErasedDataflowAnalysis");
- auto MaybeStartingEnv =
- InitEnv.callStackSize() == 1
- ? std::make_optional<Environment>(initializeEnvironment(InitEnv))
- : std::nullopt;
+ std::optional<Environment> MaybeStartingEnv;
+ if (InitEnv.callStackSize() == 1) {
+ MaybeStartingEnv = InitEnv.fork();
+ MaybeStartingEnv->initialize();
+ }
const Environment &StartingEnv =
MaybeStartingEnv ? *MaybeStartingEnv : InitEnv;
diff --git a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
index 595d05e2ba5d1bb..3569b0eac7005eb 100644
--- a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
@@ -96,7 +96,7 @@ TEST_F(EnvironmentTest, CreateValueRecursiveType) {
// Verify that the struct and the field (`R`) with first appearance of the
// type is created successfully.
Environment Env(DAContext, *Fun);
- Env.initFieldsGlobalsAndFuncs(Fun);
+ Env.initialize();
auto &SLoc = cast<RecordStorageLocation>(Env.createObject(Ty));
PointerValue *PV = cast_or_null<PointerValue>(getFieldValue(&SLoc, *R, Env));
EXPECT_THAT(PV, NotNull());
@@ -176,7 +176,7 @@ TEST_F(EnvironmentTest, InitGlobalVarsFun) {
// Verify the global variable is populated when we analyze `Target`.
Environment Env(DAContext, *Fun);
- Env.initFieldsGlobalsAndFuncs(Fun);
+ Env.initialize();
EXPECT_THAT(Env.getValue(*Var), NotNull());
}
@@ -227,7 +227,7 @@ TEST_F(EnvironmentTest, IncludeFieldsFromDefaultInitializers) {
// Verify that the `X` field of `S` is populated when analyzing the
// constructor, even though it is not referenced directly in the constructor.
Environment Env(DAContext, *Constructor);
- Env.initFieldsGlobalsAndFuncs(Constructor);
+ Env.initialize();
auto &Loc = cast<RecordStorageLocation>(Env.createObject(QTy));
EXPECT_THAT(getFieldValue(&Loc, *XDecl, Env), NotNull());
}
@@ -271,7 +271,7 @@ TEST_F(EnvironmentTest, InitGlobalVarsFieldFun) {
// Verify the global variable is populated when we analyze `Target`.
Environment Env(DAContext, *Fun);
- Env.initFieldsGlobalsAndFuncs(Fun);
+ Env.initialize();
const auto *GlobalLoc =
cast<RecordStorageLocation>(Env.getStorageLocation(*GlobalDecl));
auto *BarVal = getFieldValue(GlobalLoc, *BarDecl, Env);
@@ -307,7 +307,7 @@ TEST_F(EnvironmentTest, InitGlobalVarsConstructor) {
// Verify the global variable is populated when we analyze `Target`.
Environment Env(DAContext, *Ctor);
- Env.initFieldsGlobalsAndFuncs(Ctor);
+ Env.initialize();
EXPECT_THAT(Env.getValue(*Var), NotNull());
}
More information about the cfe-commits
mailing list