[clang] 71f2ec2 - [clang][dataflow] Add synthetic fields to `RecordStorageLocation` (#73860)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 4 00:29:26 PST 2023
Author: martinboehme
Date: 2023-12-04T09:29:22+01:00
New Revision: 71f2ec2db1295462d61e1407fcc1e715ba5d458b
URL: https://github.com/llvm/llvm-project/commit/71f2ec2db1295462d61e1407fcc1e715ba5d458b
DIFF: https://github.com/llvm/llvm-project/commit/71f2ec2db1295462d61e1407fcc1e715ba5d458b.diff
LOG: [clang][dataflow] Add synthetic fields to `RecordStorageLocation` (#73860)
Synthetic fields are intended 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.
Today, this is typically done with properties on `RecordValue`s, but
these have several drawbacks:
* Care must be taken to call `refreshRecordValue()` before modifying a
property so that the modified property values aren’t seen by other
environments that may have access to the same `RecordValue`.
* Properties aren’t associated with a storage location. If an analysis
needs to associate a location with the value stored in a property (e.g.
to model the reference returned by `std::optional::value()`), it needs
to manually add an indirection using a `PointerValue`. (See for example
the way this is done in UncheckedOptionalAccessModel.cpp, specifically
in `maybeInitializeOptionalValueMember()`.)
* Properties don’t participate in the builtin compare, join, and widen
operations. If an analysis needs to apply these operations to
properties, it needs to override the corresponding methods of
`ValueModel`.
* Longer-term, we plan to eliminate `RecordValue`, as by-value
operations on records aren’t really “a thing” in C++ (see
https://discourse.llvm.org/t/70086#changed-structvalue-api-14). This
would obviously eliminate the ability to set properties on
`RecordValue`s.
To demonstrate the advantages of synthetic fields, this patch converts
UncheckedOptionalAccessModel.cpp to synthetic fields. This greatly
simplifies the implementation of the check.
This PR is pretty big; to make it easier to review, I have broken it
down into a stack of three commits, each of which contains a set of
logically related changes. I considered submitting each of these as a
separate PR, but the commits only really make sense when taken together.
To review, I suggest first looking at the changes in
UncheckedOptionalAccessModel.cpp. This gives a flavor for how the
various API changes work together in the context of an analysis. Then,
review the rest of the changes.
Added:
Modified:
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
clang/include/clang/Analysis/FlowSensitive/RecordOps.h
clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
clang/lib/Analysis/FlowSensitive/RecordOps.cpp
clang/lib/Analysis/FlowSensitive/Transfer.cpp
clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp
clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
clang/unittests/Analysis/FlowSensitive/TestingSupport.h
clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
index c5f14cfcd4f7b..1dffbe8a09c36 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
diff erent 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/DataflowAnalysisContext.h b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
index c1aa8484817a9..20e45cc27b01f 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 7c1f549109632..d7e39ab2616fd 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -166,6 +166,15 @@ class Environment {
/// with a symbolic representation of the `this` pointee.
Environment(DataflowAnalysisContext &DACtx, const DeclContext &DeclCtx);
+ /// 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.
///
/// The state of the program is initially the same, but can be mutated without
@@ -283,7 +292,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.
///
@@ -367,7 +384,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()`).
///
@@ -570,6 +588,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`.
diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
index aeaf75b215422..09eb8b9382261 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/include/clang/Analysis/FlowSensitive/RecordOps.h b/clang/include/clang/Analysis/FlowSensitive/RecordOps.h
index f007a947a82f1..7b87840d626b4 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 89d2bbfbb69f9..8fcc6a44027a0 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 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.
///
/// 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 0a2fcd4ad695a..fa114979c8e32 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 525ab188b01b8..042402a129d10 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -367,6 +367,59 @@ 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);
+}
+
+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) {
@@ -416,59 +469,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 +733,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());
@@ -852,8 +854,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 8329367098b1d..7430ef599e032 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 &SyntheticField : RLoc->synthetic_fields())
+ JOS.attributeObject(("sf:" + SyntheticField.first()).str(),
+ [&] { dump(*SyntheticField.second); });
}
}
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 55d0713639d90..69ac2c2b82cff 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
diff erent 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
diff erent, 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/lib/Analysis/FlowSensitive/RecordOps.cpp b/clang/lib/Analysis/FlowSensitive/RecordOps.cpp
index 38638f8a9e589..caaf443382b02 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, SynthFieldLoc] : Src.synthetic_fields()) {
+ if (SynthFieldLoc->getType()->isRecordType()) {
+ copyRecord(*cast<RecordStorageLocation>(SynthFieldLoc),
+ cast<RecordStorageLocation>(Dst.getSyntheticField(Name)), Env);
+ } else {
+ if (Value *Val = Env.getValue(*SynthFieldLoc))
+ 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, SynthFieldLoc1] : Loc1.synthetic_fields()) {
+ if (SynthFieldLoc1->getType()->isRecordType()) {
+ if (!recordsEqual(
+ *cast<RecordStorageLocation>(SynthFieldLoc1), Env1,
+ cast<RecordStorageLocation>(Loc2.getSyntheticField(Name)), Env2))
+ return false;
+ } else if (Env1.getValue(*SynthFieldLoc1) !=
+ 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 4343af7900813..bbf5f12359bc7 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/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index ade8c84f19366..8c9360235da7c 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -501,6 +501,14 @@ runTypeErasedDataflowAnalysis(
PostVisitCFG) {
PrettyStackTraceAnalysis CrashInfo(CFCtx, "runTypeErasedDataflowAnalysis");
+ std::optional<Environment> MaybeStartingEnv;
+ if (InitEnv.callStackSize() == 1) {
+ MaybeStartingEnv = InitEnv.fork();
+ MaybeStartingEnv->initialize();
+ }
+ const Environment &StartingEnv =
+ MaybeStartingEnv ? *MaybeStartingEnv : InitEnv;
+
const clang::CFG &CFG = CFCtx.getCFG();
PostOrderCFGView POV(&CFG);
ForwardDataflowWorklist Worklist(CFG, &POV);
@@ -511,10 +519,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 ff6cbec5d20b7..3569b0eac7005 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.initialize();
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.initialize();
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.initialize();
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.initialize();
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.initialize();
EXPECT_THAT(Env.getValue(*Var), NotNull());
}
diff --git a/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp b/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp
index 6ba1c2ebd833d..84fe675c32c2d 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
diff erent 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
diff erent 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 e24ff25cb8292..3726f56fc824d 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 100d78378695d..95ffcbd6f322e 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.
///
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 76af8baba8518..73fb4063d92be 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,
More information about the cfe-commits
mailing list