ymandel created this revision.
ymandel added reviewers: xazax.hun, sgatev, gribozavr2.
Herald added subscribers: martong, rnkovacs.
Herald added a reviewer: NoQ.
Herald added a project: All.
ymandel requested review of this revision.
Herald added a project: clang.
This patch fixes a subtle bug in how we create lvalues to reference-typed
fields. In the rare case that the field is umodeled because of the depth limit
on field modeling, the lvalue created can be malformed. This patch prevents that
and adds some related assertions to other code dealing with lvalues for
references.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D142468
Files:
clang/lib/Analysis/FlowSensitive/Transfer.cpp
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===================================================================
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -156,6 +156,89 @@
});
}
+TEST(TransferTest, StructIncomplete) {
+ std::string Code = R"(
+ struct A;
+
+ void target() {
+ A* Foo;
+ // [[p]]
+ }
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {
+ ASSERT_THAT(Results.keys(), UnorderedElementsAre("p"));
+ const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+ const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ ASSERT_THAT(FooDecl, NotNull());
+ auto *FooValue = dyn_cast_or_null<PointerValue>(
+ Env.getValue(*FooDecl, SkipPast::None));
+ ASSERT_THAT(FooValue, NotNull());
+
+ EXPECT_TRUE(isa<AggregateStorageLocation>(FooValue->getPointeeLoc()));
+ auto *FooPointeeValue = Env.getValue(FooValue->getPointeeLoc());
+ ASSERT_THAT(FooPointeeValue, NotNull());
+ EXPECT_TRUE(isa<StructValue>(FooPointeeValue));
+ });
+}
+
+// As a memory optimization, we prevent modeling fields nested below a certain
+// level (currently, depth 3). This test verifies this lack of modeling. We also
+// include a regression test for the case that the unmodeled field is a
+// reference to a struct; previously, we crashed when accessing such a field.
+TEST(TransferTest, StructFieldUnmodeled) {
+ std::string Code = R"(
+ struct S { int X; };
+ S GlobalS;
+ struct A { S &Unmodeled = GlobalS; };
+ struct B { A F3; };
+ struct C { B F2; };
+ struct D { C F1; };
+
+ void target() {
+ D Bar;
+ A Foo = Bar.F1.F2.F3;
+ int Zab = Foo.Unmodeled.X;
+ // [[p]]
+ }
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {
+ ASSERT_THAT(Results.keys(), UnorderedElementsAre("p"));
+ const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+ const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ ASSERT_THAT(FooDecl, NotNull());
+ ASSERT_TRUE(FooDecl->getType()->isStructureType());
+ auto FooFields = FooDecl->getType()->getAsRecordDecl()->fields();
+
+ FieldDecl *UnmodeledDecl = nullptr;
+ for (FieldDecl *Field : FooFields) {
+ if (Field->getNameAsString() == "Unmodeled") {
+ UnmodeledDecl = Field;
+ } else {
+ FAIL() << "Unexpected field: " << Field->getNameAsString();
+ }
+ }
+ ASSERT_THAT(UnmodeledDecl, NotNull());
+
+ const auto *FooLoc = cast<AggregateStorageLocation>(
+ Env.getStorageLocation(*FooDecl, SkipPast::None));
+ const auto *UnmodeledLoc = &FooLoc->getChild(*UnmodeledDecl);
+ ASSERT_TRUE(isa<ScalarStorageLocation>(UnmodeledLoc));
+ ASSERT_THAT(Env.getValue(*UnmodeledLoc), IsNull());
+
+ const ValueDecl *ZabDecl = findValueDecl(ASTCtx, "Zab");
+ ASSERT_THAT(ZabDecl, NotNull());
+ EXPECT_THAT(Env.getValue(*ZabDecl, SkipPast::None), NotNull());
+ });
+}
+
TEST(TransferTest, StructVarDecl) {
std::string Code = R"(
struct A {
Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -128,6 +128,15 @@
return &UnpackedVal;
}
+// `isValidReferenceLoc` is only called inside `assert`, so we wrap it in
+// #ifndef to prevent warnings in opt builds.
+#ifndef NDEBUG
+static bool isValidReferenceLoc(StorageLocation &Loc, Environment &Env) {
+ auto *V = Env.getValue(Loc);
+ return V != nullptr && isa<ReferenceValue>(V);
+}
+#endif
+
class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
public:
TransferVisitor(const StmtToEnvMap &StmtToEnv, Environment &Env)
@@ -197,6 +206,8 @@
return;
if (VD->getType()->isReferenceType()) {
+ assert(isValidReferenceLoc(*DeclLoc, Env) &&
+ "reference-typed declarations map to `ReferenceValue`s");
Env.setStorageLocation(*S, *DeclLoc);
} else {
auto &Loc = Env.createStorageLocation(*S);
@@ -474,6 +485,8 @@
return;
if (VarDeclLoc->getType()->isReferenceType()) {
+ assert(isValidReferenceLoc(*VarDeclLoc, Env) &&
+ "reference-typed declarations map to `ReferenceValue`s");
Env.setStorageLocation(*S, *VarDeclLoc);
} else {
auto &Loc = Env.createStorageLocation(*S);
@@ -494,7 +507,22 @@
auto &MemberLoc = BaseLoc->getChild(*Member);
if (MemberLoc.getType()->isReferenceType()) {
- Env.setStorageLocation(*S, MemberLoc);
+ // Based on its type, `MemberLoc` must be mapped either to nothing or to a
+ // `ReferenceValue`. For the former, we won't set a storage location for
+ // this expression, so as to maintain an invariant lvalue expressions;
+ // namely, that their location maps to a `ReferenceValue`. In this,
+ // lvalues are unlike other expressions, where it is valid for their
+ // location to map to nothing (because they are not modeled).
+ //
+ // Note: we need this invariant for lvalues so that, when accessing a
+ // value, we can distinguish an rvalue from an lvalue. An alternative
+ // design, which takes the expression's value category into account, would
+ // avoid the need for this invariant.
+ if (auto *V = Env.getValue(MemberLoc)) {
+ assert(isa<ReferenceValue>(V) &&
+ "reference-typed declarations map to `ReferenceValue`s");
+ Env.setStorageLocation(*S, MemberLoc);
+ }
} else {
auto &Loc = Env.createStorageLocation(*S);
Env.setStorageLocation(*S, Loc);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
