[PATCH] D159235: Fix crash when struct with inheritance is initialized with InitExpr

[Kinuko Yasuda via Phabricator via cfe-commits]

kinu created this revision.
Herald added a reviewer: NoQ.
Herald added a project: All.
kinu requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This fix doesn't fix the problem of non-populated fields in base classes
(even if they are accessed in the code), and therefore could be a cause of
false positives, but fix most (if not all) of crashes.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159235

Files:
  clang/lib/Analysis/FlowSensitive/RecordOps.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
@@ -16,10 +16,8 @@
 #include "clang/Analysis/FlowSensitive/StorageLocation.h"
 #include "clang/Analysis/FlowSensitive/Value.h"
 #include "clang/Basic/LangStandard.h"
-#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
-#include "llvm/Support/Casting.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
@@ -2253,6 +2251,49 @@
  ASTContext &ASTCtx) {});
 }
 
+TEST(TransferTest, CopyConstructorWithInheritance) {
+  // This is a crash repro.
+  std::string Code = R"(
+struct B { int Foo; };
+struct S : public B {};
+void target() {
+  S S1 = { 1 };
+  S S2(S1);
+  // [p1]
+  S2.Foo = 2;
+  // [p2]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+// FIXME: Add tests to check if `Foo` exists both in S1 and S2 once
+// initialization with inheritance is fixed.
+  });
+}
+
+TEST(TransferTest, CopyConstructorWithBaseInitAndInheritance) {
+  // This is a crash repro.
+  std::string Code = R"(
+struct Foo { int Bar; };
+struct B { Foo F = { 1 }; };
+struct S : public B {};
+void target() {
+  S S1 = {};
+  S S2(S1);
+  // [p]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+// FIXME: Add tests to check if `Bar` exists both in S1 and S2 once
+// initialization with inheritance is fixed.
+  });
+}
+
 TEST(TransferTest, MoveConstructor) {
   std::string Code = R"(
 namespace std {
@@ -2328,6 +2369,22 @@
   });
 }
 
+TEST(TransferTest, ReturnStructWithInheritance) {
+  // This is a crash repro.
+  std::string Code = R"(
+struct B { int Foo; };
+struct S : public B { };
+S target() {
+  S S1 = { 1 };
+  return S1;
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {});
+}
+
 TEST(TransferTest, BindTemporary) {
   std::string Code = R"(
 struct A {
Index: clang/lib/Analysis/FlowSensitive/RecordOps.cpp
===
--- clang/lib/Analysis/FlowSensitive/RecordOps.cpp
+++ clang/lib/Analysis/FlowSensitive/RecordOps.cpp
@@ -35,8 +35,8 @@
   });
   assert(compatibleTypes);
 
-  for (auto [Field, DstFieldLoc] : Dst.children()) {
-StorageLocation *SrcFieldLoc = Src.getChild(*Field);
+  for (auto [Field, SrcFieldLoc] : Src.children()) {
+StorageLocation *DstFieldLoc = Dst.getChild(*Field);
 
 assert(Field->getType()->isReferenceType() ||
(SrcFieldLoc != nullptr && DstFieldLoc != nullptr));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159235: Fix crash when struct with inheritance is initialized with InitExpr

[Kinuko Yasuda via Phabricator via cfe-commits]

kinu updated this revision to Diff 554868.
kinu added a comment.

Add assignment test cases


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159235/new/

https://reviews.llvm.org/D159235

Files:
  clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
  clang/lib/Analysis/FlowSensitive/RecordOps.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
@@ -16,10 +16,8 @@
 #include "clang/Analysis/FlowSensitive/StorageLocation.h"
 #include "clang/Analysis/FlowSensitive/Value.h"
 #include "clang/Basic/LangStandard.h"
-#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
-#include "llvm/Support/Casting.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
@@ -2023,6 +2021,28 @@
   });
 }
 
+TEST(TransferTest, AssignmentOperatorWithInitAndInheritance) {
+  // This is a crash repro.
+  std::string Code = R"(
+struct B { int Foo; };
+struct S : public B {};
+void target() {
+  S S1 = { 1 };
+  S S2;
+  S S3;
+  S1 = S2;  // Only Dst has InitListExpr.
+  S3 = S1;  // Only Src has InitListExpr.
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+// FIXME: Add tests to check if `Foo` exists in S1, S2, and S3 once
+// initialization with inheritance is fixed.
+  });
+}
+
 TEST(TransferTest, AssignmentOperatorFromCallResult) {
   std::string Code = R"(
 struct A {};
@@ -2253,6 +2273,49 @@
  ASTContext &ASTCtx) {});
 }
 
+TEST(TransferTest, CopyConstructorWithInheritance) {
+  // This is a crash repro.
+  std::string Code = R"(
+struct B { int Foo; };
+struct S : public B {};
+void target() {
+  S S1 = { 1 };
+  S S2(S1);
+  // [p1]
+  S2.Foo = 2;
+  // [p2]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+// FIXME: Add tests to check if `Foo` exists both in S1 and S2 once
+// initialization with inheritance is fixed.
+  });
+}
+
+TEST(TransferTest, CopyConstructorWithBaseInitAndInheritance) {
+  // This is a crash repro.
+  std::string Code = R"(
+struct Foo { int Bar; };
+struct B { Foo F = { 1 }; };
+struct S : public B {};
+void target() {
+  S S1 = {};
+  S S2(S1);
+  // [p]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+// FIXME: Add tests to check if `Bar` exists both in S1 and S2 once
+// initialization with inheritance is fixed.
+  });
+}
+
 TEST(TransferTest, MoveConstructor) {
   std::string Code = R"(
 namespace std {
@@ -2328,6 +2391,22 @@
   });
 }
 
+TEST(TransferTest, ReturnStructWithInheritance) {
+  // This is a crash repro.
+  std::string Code = R"(
+struct B { int Foo; };
+struct S : public B { };
+S target() {
+  S S1 = { 1 };
+  return S1;
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {});
+}
+
 TEST(TransferTest, BindTemporary) {
   std::string Code = R"(
 struct A {
Index: clang/lib/Analysis/FlowSensitive/RecordOps.cpp
===
--- clang/lib/Analysis/FlowSensitive/RecordOps.cpp
+++ clang/lib/Analysis/FlowSensitive/RecordOps.cpp
@@ -36,7 +36,9 @@
   assert(compatibleTypes);
 
   for (auto [Field, DstFieldLoc] : Dst.children()) {
-StorageLocation *SrcFieldLoc = Src.getChild(*Field);
+StorageLocation *SrcFieldLoc = Src.tryGetChild(*Field);
+if (!SrcFieldLoc)
+  continue;
 
 assert(Field->getType()->isReferenceType() ||
(SrcFieldLoc != nullptr && DstFieldLoc != nullptr));
Index: clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
===
--- clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
+++ clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
@@ -135,6 +135,22 @@
 return It->second;
   }
 
+  /// Returns the child storage location for `D`.
+  ///
+  /// Same as `getChild()`, but may return nullptr if the field `D` does not
+  /// exist. Most users should use `getChild()` instead, except for the code
+  /// that performs deep copy of two records of the same class, where a field
+  /// in one record may not exist in another record.
+  ///
+  /// FIXME: Deprecate this method once we find and fix all the mismatching
+  /// cases (or update this comment if we find such cases valid).
+  StorageLocation *tryGetChild(const ValueDecl &D) const {
+auto It = Child

[PATCH] D159248: Fix crash when struct with inheritance is initialized with InitExpr

[Kinuko Yasuda via Phabricator via cfe-commits]

kinu created this revision.
Herald added a reviewer: NoQ.
Herald added a project: All.
kinu requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This fix still leaves inherited fields from base classes unpopulated
(even if they are accessed in the code), and might cause false
positives, but fix crashes that could happen because of source /
destination mismatches.

More details:
When a struct is declared, the Record value of the struct is usually
initialized with `Environment::createValue()` which internally calls
`getObjectFields()` (via filtering in `DACtx::getModeledFields()`)
to collects all fields from the current and base classes.

However, if a struct is initialized with InitListExpr, its fields are
initialized based on what is returned by `getFieldsForInitListExpr()`,
which doesn't collect fields from base classes. Moreover, if the base
classes have their own InitListExpr, those InitListExpr's are also
visited, but the field values and locations that are initialized by
them are not merged when the child class's InitListExpr is visited.

This doesn't usually result in a crash, except for the cases where the
struct is used to construct a new struct of the same type with copy/move
constructor: in such a case the destination struct is
created with a regular `createValue()`, and therefore also have
fields from the base classes, but the source struct does not.
And when `copyRecord()` tries to copy the fields by looping over
the destination fields, it crashes because the source struct may
not have the fields.

This change adds a new `getChildOrError()` method in `StorageLocation`
and uses it in `copyRecord()`, so that only the fields that exist in
both record can be copied.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159248

Files:
  clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
  clang/lib/Analysis/FlowSensitive/RecordOps.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
@@ -16,10 +16,8 @@
 #include "clang/Analysis/FlowSensitive/StorageLocation.h"
 #include "clang/Analysis/FlowSensitive/Value.h"
 #include "clang/Basic/LangStandard.h"
-#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
-#include "llvm/Support/Casting.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
@@ -2023,6 +2021,28 @@
   });
 }
 
+TEST(TransferTest, AssignmentOperatorWithInitAndInheritance) {
+  // This is a crash repro.
+  std::string Code = R"(
+struct B { int Foo; };
+struct S : public B {};
+void target() {
+  S S1 = { 1 };
+  S S2;
+  S S3;
+  S1 = S2;  // Only Dst has InitListExpr.
+  S3 = S1;  // Only Src has InitListExpr.
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+// FIXME: Add tests to check if `Foo` exists in S1, S2, and S3 once
+// initialization with inheritance is fixed.
+  });
+}
+
 TEST(TransferTest, AssignmentOperatorFromCallResult) {
   std::string Code = R"(
 struct A {};
@@ -2253,6 +2273,49 @@
  ASTContext &ASTCtx) {});
 }
 
+TEST(TransferTest, CopyConstructorWithInheritance) {
+  // This is a crash repro.
+  std::string Code = R"(
+struct B { int Foo; };
+struct S : public B {};
+void target() {
+  S S1 = { 1 };
+  S S2(S1);
+  // [p1]
+  S2.Foo = 2;
+  // [p2]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+// FIXME: Add tests to check if `Foo` exists both in S1 and S2 once
+// initialization with inheritance is fixed.
+  });
+}
+
+TEST(TransferTest, CopyConstructorWithBaseInitAndInheritance) {
+  // This is a crash repro.
+  std::string Code = R"(
+struct Foo { int Bar; };
+struct B { Foo F = { 1 }; };
+struct S : public B {};
+void target() {
+  S S1 = {};
+  S S2(S1);
+  // [p]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+// FIXME: Add tests to check if `Bar` exists both in S1 and S2 once
+// initialization with inheritance is fixed.
+  });
+}
+
 TEST(TransferTest, MoveConstructor) {
   std::string Code = R"(
 namespace std {
@@ -2328,6 +2391,22 @@
   });
 }
 
+TEST(TransferTest, ReturnStructWithInheritance) {
+  // This is a crash repro.
+  std::string Code = R"(
+struct B { int Foo; };
+struct S : public B { };
+S target() {
+  S S1 = { 1 };
+  return S1;
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext

[PATCH] D159248: Fix crash when struct with inheritance is initialized with InitExpr

[Kinuko Yasuda via Phabricator via cfe-commits]

kinu updated this revision to Diff 554906.
kinu added a comment.

updated comment


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159248/new/

https://reviews.llvm.org/D159248

Files:
  clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
  clang/lib/Analysis/FlowSensitive/RecordOps.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
@@ -16,10 +16,8 @@
 #include "clang/Analysis/FlowSensitive/StorageLocation.h"
 #include "clang/Analysis/FlowSensitive/Value.h"
 #include "clang/Basic/LangStandard.h"
-#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
-#include "llvm/Support/Casting.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
@@ -2023,6 +2021,28 @@
   });
 }
 
+TEST(TransferTest, AssignmentOperatorWithInitAndInheritance) {
+  // This is a crash repro.
+  std::string Code = R"(
+struct B { int Foo; };
+struct S : public B {};
+void target() {
+  S S1 = { 1 };
+  S S2;
+  S S3;
+  S1 = S2;  // Only Dst has InitListExpr.
+  S3 = S1;  // Only Src has InitListExpr.
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+// FIXME: Add tests to check if `Foo` exists in S1, S2, and S3 once
+// initialization with inheritance is fixed.
+  });
+}
+
 TEST(TransferTest, AssignmentOperatorFromCallResult) {
   std::string Code = R"(
 struct A {};
@@ -2253,6 +2273,49 @@
  ASTContext &ASTCtx) {});
 }
 
+TEST(TransferTest, CopyConstructorWithInheritance) {
+  // This is a crash repro.
+  std::string Code = R"(
+struct B { int Foo; };
+struct S : public B {};
+void target() {
+  S S1 = { 1 };
+  S S2(S1);
+  // [p1]
+  S2.Foo = 2;
+  // [p2]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+// FIXME: Add tests to check if `Foo` exists both in S1 and S2 once
+// initialization with inheritance is fixed.
+  });
+}
+
+TEST(TransferTest, CopyConstructorWithBaseInitAndInheritance) {
+  // This is a crash repro.
+  std::string Code = R"(
+struct Foo { int Bar; };
+struct B { Foo F = { 1 }; };
+struct S : public B {};
+void target() {
+  S S1 = {};
+  S S2(S1);
+  // [p]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+// FIXME: Add tests to check if `Bar` exists both in S1 and S2 once
+// initialization with inheritance is fixed.
+  });
+}
+
 TEST(TransferTest, MoveConstructor) {
   std::string Code = R"(
 namespace std {
@@ -2328,6 +2391,22 @@
   });
 }
 
+TEST(TransferTest, ReturnStructWithInheritance) {
+  // This is a crash repro.
+  std::string Code = R"(
+struct B { int Foo; };
+struct S : public B { };
+S target() {
+  S S1 = { 1 };
+  return S1;
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {});
+}
+
 TEST(TransferTest, BindTemporary) {
   std::string Code = R"(
 struct A {
Index: clang/lib/Analysis/FlowSensitive/RecordOps.cpp
===
--- clang/lib/Analysis/FlowSensitive/RecordOps.cpp
+++ clang/lib/Analysis/FlowSensitive/RecordOps.cpp
@@ -36,7 +36,10 @@
   assert(compatibleTypes);
 
   for (auto [Field, DstFieldLoc] : Dst.children()) {
-StorageLocation *SrcFieldLoc = Src.getChild(*Field);
+llvm::ErrorOr SrcField = Src.getChildOrError(*Field);
+if (!SrcField)
+  continue;
+StorageLocation *SrcFieldLoc = SrcField.get();
 
 assert(Field->getType()->isReferenceType() ||
(SrcFieldLoc != nullptr && DstFieldLoc != nullptr));
Index: clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
===
--- clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
+++ clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
@@ -18,6 +18,7 @@
 #include "clang/AST/Type.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/Support/Debug.h"
+#include "llvm/Support/ErrorOr.h"
 #include 
 
 #define DEBUG_TYPE "dataflow"
@@ -135,6 +136,19 @@
 return It->second;
   }
 
+  /// Returns the child storage location for `D` if it exists.
+  ///
+  /// Same as `getChild()`, but returns llvm::Error if the field `D` does not
+  /// exist. Most users should use `getChild()` instead, except for the code
+  /// that performs deep copy of two records of the same class, where a field
+  /// in one record may not exist in anot

[PATCH] D159284: Fix Record initialization with InitListExpr and inheritances

[Kinuko Yasuda via Phabricator via cfe-commits]

kinu created this revision.
Herald added a reviewer: NoQ.
Herald added a project: All.
kinu requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Usually RecordValues for record objects (e.g. struct) are initialized with
`Environment::createValue()` which internally calls `getObjectFields()` to
collects all fields from the current and base classes, and then filter them
with `ModeledValues` via `DACtx::getModeledFields()` so that the fields that
are actually referenced are modeled.

The consistent set of fields should be initialized when a record is initialized
with an initializer list (InitListExpr), however the existing code's behavior
was different.

Before this patch:

- When a struct is initialized with InitListExpr, its fields are initialized 
based on what is returned by `getFieldsForInitListExpr()`, which only collects 
the direct fields in the current class, but not from the base classes. 
Moreover, if the base classes have their own InitListExpr, values that are 
initialized by their InitListExpr's weren't merged into the child objects.

After this patch:

- When a struct is initialized with InitListExpr, it collects and merges the 
fields in the base classes that were initialized by their InitListExpr's. The 
code also asserts that the consistent set of fields are initialized with the 
ModeledFields.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159284

Files:
  clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
  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
@@ -1438,6 +1438,150 @@
   llvm::Succeeded());
 }
 
+TEST(TransferTest, StructModeledFieldsWithComplicatedInheritance) {
+  std::string Code = R"(
+struct Base3 {
+  int x7;
+  int x8;
+};
+struct Base2 : Base3 {
+  int x5;
+  int x6;
+};
+struct Base {
+  int x3;
+  int x4;
+};
+struct Foo : public Base, Base2 {
+  int x1;
+  int x2;
+};
+
+void target() {
+  Foo F;
+  F.x4 = F.x2;
+  F.x6 = 1;
+  F.x8 = 1;
+  // [[p]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Env =
+  getEnvironmentAtAnnotation(Results, "p");
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "F");
+const ValueDecl *X1Decl = findValueDecl(ASTCtx, "x1");
+const ValueDecl *X2Decl = findValueDecl(ASTCtx, "x2");
+const ValueDecl *X3Decl = findValueDecl(ASTCtx, "x3");
+const ValueDecl *X4Decl = findValueDecl(ASTCtx, "x4");
+const ValueDecl *X5Decl = findValueDecl(ASTCtx, "x5");
+const ValueDecl *X6Decl = findValueDecl(ASTCtx, "x6");
+const ValueDecl *X7Decl = findValueDecl(ASTCtx, "x7");
+const ValueDecl *X8Decl = findValueDecl(ASTCtx, "x8");
+ASSERT_THAT(FooDecl, NotNull());
+
+// Only "x2", "x4", "x6", and "x8" are accessed and exist in the model.
+const auto *FooLoc =
+cast(Env.getStorageLocation(*FooDecl));
+llvm::DenseSet Fields;
+for (auto [Field, _] : FooLoc->children()) {
+  Fields.insert(Field);
+}
+ASSERT_EQ(Fields.size(), 4);
+ASSERT_TRUE(Fields.contains(X2Decl));
+ASSERT_TRUE(Fields.contains(X4Decl));
+ASSERT_TRUE(Fields.contains(X6Decl));
+ASSERT_TRUE(Fields.contains(X8Decl));
+
+// "x1", "x3", "x5", "x7" are not used therefore are filtered out.
+ASSERT_FALSE(Fields.contains(X1Decl));
+ASSERT_FALSE(Fields.contains(X3Decl));
+ASSERT_FALSE(Fields.contains(X5Decl));
+ASSERT_FALSE(Fields.contains(X7Decl));
+  });
+}
+
+TEST(TransferTest, StructInitializerListWithComplicatedInheritance) {
+  std::string Code = R"(
+struct Base3 {
+  int x7;
+  int x8;
+};
+struct Base2 : Base3 {
+  int x5;
+  int x6;
+};
+struct Base {
+  int x3;
+  int x4;
+};
+struct Foo : public Base, Base2 {
+  int x1;
+  int x2;
+};
+
+void target() {
+  Foo F = {};
+  F.x4 = F.x2;
+  F.x6 = 1;
+  F.x8 = 1;
+  // [[p]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Env =
+  getEnvironmentAtAnnotation(Results, "p");
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "F");
+const ValueDecl *X1Decl = findValueDecl(ASTCtx, "x1");
+const ValueDecl *X2Decl = findValueDecl(ASTCtx, "x2");
+const ValueDecl *X3Decl = findValueDecl(ASTCtx, "x3");
+const ValueDecl *X4

[PATCH] D159284: [clang][dataflow] Fix Record initialization with InitListExpr and inheritances

[Kinuko Yasuda via Phabricator via cfe-commits]

kinu added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:131
+// initialized with InitListExpr.
 assert(&RecordVal1->getLoc() == &RecordVal2->getLoc());
 

Note, this hits regardless of my change, but not very often.  I haven't looked 
into details yet


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159284/new/

https://reviews.llvm.org/D159284

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159284: [clang][dataflow] Fix Record initialization with InitListExpr and inheritances

[Kinuko Yasuda via Phabricator via cfe-commits]

kinu updated this revision to Diff 555166.
kinu added a comment.

Exclude lambdas


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159284/new/

https://reviews.llvm.org/D159284

Files:
  clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
  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
@@ -1438,6 +1438,150 @@
   llvm::Succeeded());
 }
 
+TEST(TransferTest, StructModeledFieldsWithComplicatedInheritance) {
+  std::string Code = R"(
+struct Base3 {
+  int x7;
+  int x8;
+};
+struct Base2 : Base3 {
+  int x5;
+  int x6;
+};
+struct Base {
+  int x3;
+  int x4;
+};
+struct Foo : public Base, Base2 {
+  int x1;
+  int x2;
+};
+
+void target() {
+  Foo F;
+  F.x4 = F.x2;
+  F.x6 = 1;
+  F.x8 = 1;
+  // [[p]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Env =
+  getEnvironmentAtAnnotation(Results, "p");
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "F");
+const ValueDecl *X1Decl = findValueDecl(ASTCtx, "x1");
+const ValueDecl *X2Decl = findValueDecl(ASTCtx, "x2");
+const ValueDecl *X3Decl = findValueDecl(ASTCtx, "x3");
+const ValueDecl *X4Decl = findValueDecl(ASTCtx, "x4");
+const ValueDecl *X5Decl = findValueDecl(ASTCtx, "x5");
+const ValueDecl *X6Decl = findValueDecl(ASTCtx, "x6");
+const ValueDecl *X7Decl = findValueDecl(ASTCtx, "x7");
+const ValueDecl *X8Decl = findValueDecl(ASTCtx, "x8");
+ASSERT_THAT(FooDecl, NotNull());
+
+// Only "x2", "x4", "x6", and "x8" are accessed and exist in the model.
+const auto *FooLoc =
+cast(Env.getStorageLocation(*FooDecl));
+llvm::DenseSet Fields;
+for (auto [Field, _] : FooLoc->children()) {
+  Fields.insert(Field);
+}
+ASSERT_EQ(Fields.size(), 4);
+ASSERT_TRUE(Fields.contains(X2Decl));
+ASSERT_TRUE(Fields.contains(X4Decl));
+ASSERT_TRUE(Fields.contains(X6Decl));
+ASSERT_TRUE(Fields.contains(X8Decl));
+
+// "x1", "x3", "x5", "x7" are not used therefore are filtered out.
+ASSERT_FALSE(Fields.contains(X1Decl));
+ASSERT_FALSE(Fields.contains(X3Decl));
+ASSERT_FALSE(Fields.contains(X5Decl));
+ASSERT_FALSE(Fields.contains(X7Decl));
+  });
+}
+
+TEST(TransferTest, StructInitializerListWithComplicatedInheritance) {
+  std::string Code = R"(
+struct Base3 {
+  int x7;
+  int x8;
+};
+struct Base2 : Base3 {
+  int x5;
+  int x6;
+};
+struct Base {
+  int x3;
+  int x4;
+};
+struct Foo : public Base, Base2 {
+  int x1;
+  int x2;
+};
+
+void target() {
+  Foo F = {};
+  F.x4 = F.x2;
+  F.x6 = 1;
+  F.x8 = 1;
+  // [[p]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Env =
+  getEnvironmentAtAnnotation(Results, "p");
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "F");
+const ValueDecl *X1Decl = findValueDecl(ASTCtx, "x1");
+const ValueDecl *X2Decl = findValueDecl(ASTCtx, "x2");
+const ValueDecl *X3Decl = findValueDecl(ASTCtx, "x3");
+const ValueDecl *X4Decl = findValueDecl(ASTCtx, "x4");
+const ValueDecl *X5Decl = findValueDecl(ASTCtx, "x5");
+const ValueDecl *X6Decl = findValueDecl(ASTCtx, "x6");
+const ValueDecl *X7Decl = findValueDecl(ASTCtx, "x7");
+const ValueDecl *X8Decl = findValueDecl(ASTCtx, "x8");
+ASSERT_THAT(FooDecl, NotNull());
+
+// When a struct is initialized with a initializer list, all the
+// fields are considered "accessed", and therefore do exist.
+// (Without the initializer, we should only see "x2", "x4", "x6",
+// and "x8")
+const auto *FooLoc =
+cast(Env.getStorageLocation(*FooDecl));
+const auto *X1Val =
+cast(getFieldValue(FooLoc, *X1Decl, Env));
+const auto *X2Val =
+cast(getFieldValue(FooLoc, *X2Decl, Env));
+const auto *X3Val =
+cast(getFieldValue(FooLoc, *X3Decl, Env));
+const auto *X4Val =
+cast(getFieldValue(FooLoc, *X4Decl, Env));
+const auto *X5Val =
+cast(getFieldValue(FooLoc, *X5Decl, Env));
+const auto *X6Val =
+cast(getFieldValue(FooLoc, *X6Decl, Env));
+const auto *X7Val =
+cast(getFieldValue(FooLo

[PATCH] D159284: [clang][dataflow] Fix Record initialization with InitListExpr and inheritances

[Kinuko Yasuda via Phabricator via cfe-commits]

kinu updated this revision to Diff 555676.
kinu added a comment.

reverted wrong changes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159284/new/

https://reviews.llvm.org/D159284

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
@@ -1438,6 +1438,150 @@
   llvm::Succeeded());
 }
 
+TEST(TransferTest, StructModeledFieldsWithComplicatedInheritance) {
+  std::string Code = R"(
+struct Base3 {
+  int x7;
+  int x8;
+};
+struct Base2 : Base3 {
+  int x5;
+  int x6;
+};
+struct Base {
+  int x3;
+  int x4;
+};
+struct Foo : public Base, Base2 {
+  int x1;
+  int x2;
+};
+
+void target() {
+  Foo F;
+  F.x4 = F.x2;
+  F.x6 = 1;
+  F.x8 = 1;
+  // [[p]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Env =
+  getEnvironmentAtAnnotation(Results, "p");
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "F");
+const ValueDecl *X1Decl = findValueDecl(ASTCtx, "x1");
+const ValueDecl *X2Decl = findValueDecl(ASTCtx, "x2");
+const ValueDecl *X3Decl = findValueDecl(ASTCtx, "x3");
+const ValueDecl *X4Decl = findValueDecl(ASTCtx, "x4");
+const ValueDecl *X5Decl = findValueDecl(ASTCtx, "x5");
+const ValueDecl *X6Decl = findValueDecl(ASTCtx, "x6");
+const ValueDecl *X7Decl = findValueDecl(ASTCtx, "x7");
+const ValueDecl *X8Decl = findValueDecl(ASTCtx, "x8");
+ASSERT_THAT(FooDecl, NotNull());
+
+// Only "x2", "x4", "x6", and "x8" are accessed and exist in the model.
+const auto *FooLoc =
+cast(Env.getStorageLocation(*FooDecl));
+llvm::DenseSet Fields;
+for (auto [Field, _] : FooLoc->children()) {
+  Fields.insert(Field);
+}
+ASSERT_EQ(Fields.size(), 4);
+ASSERT_TRUE(Fields.contains(X2Decl));
+ASSERT_TRUE(Fields.contains(X4Decl));
+ASSERT_TRUE(Fields.contains(X6Decl));
+ASSERT_TRUE(Fields.contains(X8Decl));
+
+// "x1", "x3", "x5", "x7" are not used therefore are filtered out.
+ASSERT_FALSE(Fields.contains(X1Decl));
+ASSERT_FALSE(Fields.contains(X3Decl));
+ASSERT_FALSE(Fields.contains(X5Decl));
+ASSERT_FALSE(Fields.contains(X7Decl));
+  });
+}
+
+TEST(TransferTest, StructInitializerListWithComplicatedInheritance) {
+  std::string Code = R"(
+struct Base3 {
+  int x7;
+  int x8;
+};
+struct Base2 : Base3 {
+  int x5;
+  int x6;
+};
+struct Base {
+  int x3;
+  int x4;
+};
+struct Foo : public Base, Base2 {
+  int x1;
+  int x2;
+};
+
+void target() {
+  Foo F = {};
+  F.x4 = F.x2;
+  F.x6 = 1;
+  F.x8 = 1;
+  // [[p]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Env =
+  getEnvironmentAtAnnotation(Results, "p");
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "F");
+const ValueDecl *X1Decl = findValueDecl(ASTCtx, "x1");
+const ValueDecl *X2Decl = findValueDecl(ASTCtx, "x2");
+const ValueDecl *X3Decl = findValueDecl(ASTCtx, "x3");
+const ValueDecl *X4Decl = findValueDecl(ASTCtx, "x4");
+const ValueDecl *X5Decl = findValueDecl(ASTCtx, "x5");
+const ValueDecl *X6Decl = findValueDecl(ASTCtx, "x6");
+const ValueDecl *X7Decl = findValueDecl(ASTCtx, "x7");
+const ValueDecl *X8Decl = findValueDecl(ASTCtx, "x8");
+ASSERT_THAT(FooDecl, NotNull());
+
+// When a struct is initialized with a initializer list, all the
+// fields are considered "accessed", and therefore do exist.
+// (Without the initializer, we should only see "x2", "x4", "x6",
+// and "x8")
+const auto *FooLoc =
+cast(Env.getStorageLocation(*FooDecl));
+const auto *X1Val =
+cast(getFieldValue(FooLoc, *X1Decl, Env));
+const auto *X2Val =
+cast(getFieldValue(FooLoc, *X2Decl, Env));
+const auto *X3Val =
+cast(getFieldValue(FooLoc, *X3Decl, Env));
+const auto *X4Val =
+cast(getFieldValue(FooLoc, *X4Decl, Env));
+const auto *X5Val =
+cast(getFieldValue(FooLoc, *X5Decl, Env));
+const auto *X6Val =
+cast(getFieldValue(FooLoc, *X6Decl, Env));
+const auto *X7Val =
+cast(getFieldValue(FooLoc, *X7Decl, Env));
+const auto *X8Val =
+   

[PATCH] D159284: [clang][dataflow] Fix Record initialization with InitListExpr and inheritances

[Kinuko Yasuda via Phabricator via cfe-commits]

kinu updated this revision to Diff 555777.
kinu marked 19 inline comments as done.
kinu added a comment.

Addressed comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159284/new/

https://reviews.llvm.org/D159284

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
@@ -1438,6 +1438,116 @@
   llvm::Succeeded());
 }
 
+TEST(TransferTest, StructModeledFieldsWithComplicatedInheritance) {
+  std::string Code = R"(
+struct Base1 {
+  int base1_1;
+  int base1_2;
+};
+struct Intermediate : Base1 {
+  int intermediate_1;
+  int intermediate_2;
+};
+struct Base2 {
+  int base2_1;
+  int base2_2;
+};
+struct MostDerived : public Intermediate, Base2 {
+  int most_derived_1;
+  int most_derived_2;
+};
+
+void target() {
+  MostDerived MD;
+  MD.base1_2 = 1;
+  MD.intermediate_2 = 1;
+  MD.base2_2 = 1;
+  MD.most_derived_2 = 1;
+  // [[p]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Env =
+  getEnvironmentAtAnnotation(Results, "p");
+
+// Only the accessed fields should exist in the model.
+auto &MDLoc = getLocForDecl(ASTCtx, Env, "MD");
+std::vector Fields;
+for (auto [Field, _] : MDLoc.children()) {
+  Fields.push_back(Field);
+}
+ASSERT_THAT(Fields, UnorderedElementsAre(
+findValueDecl(ASTCtx, "base1_2"),
+findValueDecl(ASTCtx, "intermediate_2"),
+findValueDecl(ASTCtx, "base2_2"),
+findValueDecl(ASTCtx, "most_derived_2")));
+  });
+}
+
+TEST(TransferTest, StructInitializerListWithComplicatedInheritance) {
+  std::string Code = R"(
+struct Base1 {
+  int base1_1;
+  int base1_2;
+};
+struct Intermediate : Base1 {
+  int intermediate_1;
+  int intermediate_2;
+};
+struct Base2 {
+  int base2_1;
+  int base2_2;
+};
+struct MostDerived : public Intermediate, Base2 {
+  int most_derived_1;
+  int most_derived_2;
+};
+
+void target() {
+  MostDerived MD = {};
+  // [[p]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Env =
+  getEnvironmentAtAnnotation(Results, "p");
+
+// When a struct is initialized with a initializer list, all the
+// fields are considered "accessed", and therefore do exist.
+auto &MD = getLocForDecl(ASTCtx, Env, "MD");
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "base1_1"), Env)),
+NotNull());
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "base1_2"), Env)),
+NotNull());
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "intermediate_1"), Env)),
+NotNull());
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "intermediate_2"), Env)),
+NotNull());
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "base2_1"), Env)),
+NotNull());
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "base2_1"), Env)),
+NotNull());
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "most_derived_1"), Env)),
+NotNull());
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "most_derived_2"), Env)),
+NotNull());
+  });
+}
+
 TEST(TransferTest, ReferenceMember) {
   std::string Code = R"(
 struct A {
@@ -2043,6 +2153,26 @@
   });
 }
 
+TEST(TransferTest, AssignmentOperatorWithInitAndInheritance) {
+  // This is a crash repro.
+  std::string Code = R"(
+struct B { int Foo; };
+struct S : public B {};
+void target() {
+  S S1 = { 1 };
+  S S2;
+  S S3;
+  S1 = S2;  // Only Dst has InitListExpr.
+  S3 = S1;  // Only Src has InitListExpr.
+  // [[p]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {});
+}
+
 TEST(TransferTest, CopyConstructor) {
   std::string Code = R"(
 struct A {
@@ -2253,6 +2383,76 @@
  ASTContext &ASTCtx) {});
 }
 
+TEST(TransferTest, CopyConstructorWithInitializerAndInheritance) {
+  // This is a crash repro.
+  std::string Code = R"(
+struct B { int Foo; };
+struct S : public B {};
+void target() {
+  S S1 = { 1 };
+  S S2(S1);
+  //

[PATCH] D159284: [clang][dataflow] Fix Record initialization with InitListExpr and inheritances

[Kinuko Yasuda via Phabricator via cfe-commits]

kinu added a comment.

Thanks!  Updated the patch.




Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:650
+  assert(FieldsForInit.size() + R->getNumBases() == Inits.size());
+  for ([[maybe_unused]] const CXXBaseSpecifier &Base : R->bases()) {
+assert(InitIdx < Inits.size());

mboehme wrote:
> This `[[maybe_unused]]` seems unnecesary, as `Base` is definitely used in the 
> `isLambda()` check?
I removed the isLambda check below from this patch, let me keep this line for 
now



Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:655
+   GetCanonicalType(Init->getType()));
+if (Base.getType()->getAsCXXRecordDecl()->isLambda()) {
+  // FIXME: Figure out what to do for lambdas.

mboehme wrote:
> - Can this actually happen? (How do you make a lambda a base class?)
> - Why does this need to be handled separately? (Isn't a lambda just a normal 
> class, modulo sugar? What is different about lambdas that means we can't 
> handle them like other classes?)
> 
> Depending on the answers to the above, would also be useful to capture them 
> as comments in the source code.
> 
> I assume you encountered this scenario somewhere. Can you add a test (with 
> incorrect behavior marked as a FIXME) that exercises this?
I hit this case in one of real code so added this afterwards...

but actually can I drop this part from this patch but come back in a separate 
patch once I clarify the behavior?  I somehow lost the note about in which file 
this happened, and I need to confirm a few things before I can add a test :(



Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:683-686
+  // `ModeledFields` also contains from all the bases, but only the modeled
+  // ones. Having an initializer list for a struct basically populates all
+  // the fields for the struct, so the fields set that is populated here
+  // should match the modeled fields without additional filtering.

mboehme wrote:
> Rewrote this a bit with the aim of making it clearer -- WDYT?
Much better, applied!



Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:2191
+TEST(TransferTest, AssignmentOperatorWithInitAndInheritance) {
+  // This is a crash repro.
+  std::string Code = R"(

mboehme wrote:
> IIUC, the crash was happening because `copyRecord()` assumes that the source 
> and destination have the same fields. More specifically, there was an 
> unspoken invariant in the framework that a `RecordStorageLocation` should 
> always contain exactly the fields returned by 
> `DataflowAnalysisContext::getModeledFields()`), but our treatment of 
> `InitListExpr` was violating this invariant.
> 
> IOW, this wasn't really an issue with the way we treat copy/move operations 
> but with `InitlistExpr`.
> 
> I understand that we want to have a test the reproduces the exact 
> circumstances that led to the crash, but maybe it's enough to have one of 
> these, and have the focus of the testing be on testing the actual invariant 
> that we want to maintain? IOW, maybe keep this test but delete the additional 
> tests below?
Dropped the additional tests below.



Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:2208-2220
+const ValueDecl *S1Decl = findValueDecl(ASTCtx, "S1");
+const ValueDecl *S2Decl = findValueDecl(ASTCtx, "S2");
+const ValueDecl *S3Decl = findValueDecl(ASTCtx, "S3");
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+
+const Environment &Env =
+  getEnvironmentAtAnnotation(Results, "p");

mboehme wrote:
> 
(Now this part is removed)



Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:2498
+TEST(TransferTest, ReturnStructWithInheritance) {
+  // This is a crash repro. (Returning a struct involves copy/move constructor)
+  std::string Code = R"(

mboehme wrote:
> I don't think this is true -- the `return S1` in `target()` will use NRVO.
> 
> But I also don't think it's relevant -- I believe the crash this is 
> reproducing was triggered by the `InitListExpr`, not the return statement?
> 
> Given the other tests added in this patch, do we need this test?
It will use NRVO but in AST this still seems to generate CXXConstructExpr with 
isCopyOrMoveConstructor() == true because omitting the ctor is not mandatory in 
compilers.

I can drop this one, but I'm also a bit torn because this was the original 
crash repro that puzzled me a bit.

I refined the comment to explain it a bit better; how does it look now?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159284/new/

https://reviews.llvm.org/D159284

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/l

[PATCH] D159284: [clang][dataflow] Fix Record initialization with InitListExpr and inheritances

[Kinuko Yasuda via Phabricator via cfe-commits]

kinu updated this revision to Diff 555838.
kinu marked an inline comment as done and an inline comment as not done.
kinu added a comment.

Updated


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159284/new/

https://reviews.llvm.org/D159284

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
@@ -1436,6 +1436,116 @@
   llvm::Succeeded());
 }
 
+TEST(TransferTest, StructModeledFieldsWithComplicatedInheritance) {
+  std::string Code = R"(
+struct Base1 {
+  int base1_1;
+  int base1_2;
+};
+struct Intermediate : Base1 {
+  int intermediate_1;
+  int intermediate_2;
+};
+struct Base2 {
+  int base2_1;
+  int base2_2;
+};
+struct MostDerived : public Intermediate, Base2 {
+  int most_derived_1;
+  int most_derived_2;
+};
+
+void target() {
+  MostDerived MD;
+  MD.base1_2 = 1;
+  MD.intermediate_2 = 1;
+  MD.base2_2 = 1;
+  MD.most_derived_2 = 1;
+  // [[p]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Env =
+  getEnvironmentAtAnnotation(Results, "p");
+
+// Only the accessed fields should exist in the model.
+auto &MDLoc = getLocForDecl(ASTCtx, Env, "MD");
+std::vector Fields;
+for (auto [Field, _] : MDLoc.children()) {
+  Fields.push_back(Field);
+}
+ASSERT_THAT(Fields, UnorderedElementsAre(
+findValueDecl(ASTCtx, "base1_2"),
+findValueDecl(ASTCtx, "intermediate_2"),
+findValueDecl(ASTCtx, "base2_2"),
+findValueDecl(ASTCtx, "most_derived_2")));
+  });
+}
+
+TEST(TransferTest, StructInitializerListWithComplicatedInheritance) {
+  std::string Code = R"(
+struct Base1 {
+  int base1_1;
+  int base1_2;
+};
+struct Intermediate : Base1 {
+  int intermediate_1;
+  int intermediate_2;
+};
+struct Base2 {
+  int base2_1;
+  int base2_2;
+};
+struct MostDerived : public Intermediate, Base2 {
+  int most_derived_1;
+  int most_derived_2;
+};
+
+void target() {
+  MostDerived MD = {};
+  // [[p]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Env =
+  getEnvironmentAtAnnotation(Results, "p");
+
+// When a struct is initialized with a initializer list, all the
+// fields are considered "accessed", and therefore do exist.
+auto &MD = getLocForDecl(ASTCtx, Env, "MD");
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "base1_1"), Env)),
+NotNull());
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "base1_2"), Env)),
+NotNull());
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "intermediate_1"), Env)),
+NotNull());
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "intermediate_2"), Env)),
+NotNull());
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "base2_1"), Env)),
+NotNull());
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "base2_1"), Env)),
+NotNull());
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "most_derived_1"), Env)),
+NotNull());
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "most_derived_2"), Env)),
+NotNull());
+  });
+}
+
 TEST(TransferTest, ReferenceMember) {
   std::string Code = R"(
 struct A {
@@ -2041,6 +2151,26 @@
   });
 }
 
+TEST(TransferTest, AssignmentOperatorWithInitAndInheritance) {
+  // This is a crash repro.
+  std::string Code = R"(
+struct B { int Foo; };
+struct S : public B {};
+void target() {
+  S S1 = { 1 };
+  S S2;
+  S S3;
+  S1 = S2;  // Only Dst has InitListExpr.
+  S3 = S1;  // Only Src has InitListExpr.
+  // [[p]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {});
+}
+
 TEST(TransferTest, CopyConstructor) {
   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
@@ -27,12 +27,12 @@
 #include "clang/Analysis/FlowSensitive/Value.h"

[PATCH] D159284: [clang][dataflow] Fix Record initialization with InitListExpr and inheritances

[Kinuko Yasuda via Phabricator via cfe-commits]

kinu updated this revision to Diff 555840.
kinu marked 2 inline comments as done.
kinu added a comment.

More fixes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159284/new/

https://reviews.llvm.org/D159284

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
@@ -1436,6 +1436,116 @@
   llvm::Succeeded());
 }
 
+TEST(TransferTest, StructModeledFieldsWithComplicatedInheritance) {
+  std::string Code = R"(
+struct Base1 {
+  int base1_1;
+  int base1_2;
+};
+struct Intermediate : Base1 {
+  int intermediate_1;
+  int intermediate_2;
+};
+struct Base2 {
+  int base2_1;
+  int base2_2;
+};
+struct MostDerived : public Intermediate, Base2 {
+  int most_derived_1;
+  int most_derived_2;
+};
+
+void target() {
+  MostDerived MD;
+  MD.base1_2 = 1;
+  MD.intermediate_2 = 1;
+  MD.base2_2 = 1;
+  MD.most_derived_2 = 1;
+  // [[p]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Env =
+  getEnvironmentAtAnnotation(Results, "p");
+
+// Only the accessed fields should exist in the model.
+auto &MDLoc = getLocForDecl(ASTCtx, Env, "MD");
+std::vector Fields;
+for (auto [Field, _] : MDLoc.children()) {
+  Fields.push_back(Field);
+}
+ASSERT_THAT(Fields, UnorderedElementsAre(
+findValueDecl(ASTCtx, "base1_2"),
+findValueDecl(ASTCtx, "intermediate_2"),
+findValueDecl(ASTCtx, "base2_2"),
+findValueDecl(ASTCtx, "most_derived_2")));
+  });
+}
+
+TEST(TransferTest, StructInitializerListWithComplicatedInheritance) {
+  std::string Code = R"(
+struct Base1 {
+  int base1_1;
+  int base1_2;
+};
+struct Intermediate : Base1 {
+  int intermediate_1;
+  int intermediate_2;
+};
+struct Base2 {
+  int base2_1;
+  int base2_2;
+};
+struct MostDerived : public Intermediate, Base2 {
+  int most_derived_1;
+  int most_derived_2;
+};
+
+void target() {
+  MostDerived MD = {};
+  // [[p]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Env =
+  getEnvironmentAtAnnotation(Results, "p");
+
+// When a struct is initialized with a initializer list, all the
+// fields are considered "accessed", and therefore do exist.
+auto &MD = getLocForDecl(ASTCtx, Env, "MD");
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "base1_1"), Env)),
+NotNull());
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "base1_2"), Env)),
+NotNull());
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "intermediate_1"), Env)),
+NotNull());
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "intermediate_2"), Env)),
+NotNull());
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "base2_1"), Env)),
+NotNull());
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "base2_1"), Env)),
+NotNull());
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "most_derived_1"), Env)),
+NotNull());
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "most_derived_2"), Env)),
+NotNull());
+  });
+}
+
 TEST(TransferTest, ReferenceMember) {
   std::string Code = R"(
 struct A {
@@ -2041,6 +2151,26 @@
   });
 }
 
+TEST(TransferTest, AssignmentOperatorWithInitAndInheritance) {
+  // This is a crash repro.
+  std::string Code = R"(
+struct B { int Foo; };
+struct S : public B {};
+void target() {
+  S S1 = { 1 };
+  S S2;
+  S S3;
+  S1 = S2;  // Only Dst has InitListExpr.
+  S3 = S1;  // Only Src has InitListExpr.
+  // [[p]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {});
+}
+
 TEST(TransferTest, CopyConstructor) {
   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
@@ -27,12 +27,12 @@
 #include "clang/Analysis/FlowSensitive/Value.h"
 #include "clang/Basic/Builtin

[PATCH] D159284: [clang][dataflow] Fix Record initialization with InitListExpr and inheritances

[Kinuko Yasuda via Phabricator via cfe-commits]

kinu updated this revision to Diff 555842.
kinu marked 5 inline comments as done.
kinu added a comment.

Updated again... very easy to overlook unaddressed comments!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159284/new/

https://reviews.llvm.org/D159284

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
@@ -1436,6 +1436,99 @@
   llvm::Succeeded());
 }
 
+TEST(TransferTest, StructModeledFieldsWithComplicatedInheritance) {
+  std::string Code = R"(
+struct Base1 {
+  int base1_1;
+  int base1_2;
+};
+struct Intermediate : Base1 {
+  int intermediate_1;
+  int intermediate_2;
+};
+struct Base2 {
+  int base2_1;
+  int base2_2;
+};
+struct MostDerived : public Intermediate, Base2 {
+  int most_derived_1;
+  int most_derived_2;
+};
+
+void target() {
+  MostDerived MD;
+  MD.base1_2 = 1;
+  MD.intermediate_2 = 1;
+  MD.base2_2 = 1;
+  MD.most_derived_2 = 1;
+  // [[p]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Env =
+  getEnvironmentAtAnnotation(Results, "p");
+
+// Only the accessed fields should exist in the model.
+auto &MDLoc = getLocForDecl(ASTCtx, Env, "MD");
+std::vector Fields;
+for (auto [Field, _] : MDLoc.children())
+  Fields.push_back(Field);
+ASSERT_THAT(Fields, UnorderedElementsAre(
+findValueDecl(ASTCtx, "base1_2"),
+findValueDecl(ASTCtx, "intermediate_2"),
+findValueDecl(ASTCtx, "base2_2"),
+findValueDecl(ASTCtx, "most_derived_2")));
+  });
+}
+
+TEST(TransferTest, StructInitializerListWithComplicatedInheritance) {
+  std::string Code = R"(
+struct Base1 {
+  int base1;
+};
+struct Intermediate : Base1 {
+  int intermediate;
+};
+struct Base2 {
+  int base2;
+};
+struct MostDerived : public Intermediate, Base2 {
+  int most_derived;
+};
+
+void target() {
+  MostDerived MD = {};
+  // [[p]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+const Environment &Env =
+  getEnvironmentAtAnnotation(Results, "p");
+
+// When a struct is initialized with a initializer list, all the
+// fields are considered "accessed", and therefore do exist.
+auto &MD = getLocForDecl(ASTCtx, Env, "MD");
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "base1"), Env)),
+NotNull());
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "intermediate"), Env)),
+NotNull());
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "base2"), Env)),
+NotNull());
+ASSERT_THAT(cast(
+getFieldValue(&MD, *findValueDecl(ASTCtx, "most_derived"), Env)),
+NotNull());
+  });
+}
+
 TEST(TransferTest, ReferenceMember) {
   std::string Code = R"(
 struct A {
@@ -2041,6 +2134,26 @@
   });
 }
 
+TEST(TransferTest, AssignmentOperatorWithInitAndInheritance) {
+  // This is a crash repro.
+  std::string Code = R"(
+struct B { int Foo; };
+struct S : public B {};
+void target() {
+  S S1 = { 1 };
+  S S2;
+  S S3;
+  S1 = S2;  // Only Dst has InitListExpr.
+  S3 = S1;  // Only Src has InitListExpr.
+  // [[p]]
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {});
+}
+
 TEST(TransferTest, CopyConstructor) {
   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
@@ -27,12 +27,12 @@
 #include "clang/Analysis/FlowSensitive/Value.h"
 #include "clang/Basic/Builtins.h"
 #include "clang/Basic/OperatorKinds.h"
-#include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Casting.h"
-#include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/Debug.h"
+#include 
 #include 
-#include 
-#include 
+
+#define DEBUG_TYPE "dataflow"
 
 namespace clang {
 namespace dataflow {
@@ -629,17 +629,66 @@
   return;
 }
 
-std::vector Fields =
-getFieldsForInitListExpr(Type->getAsRecordDecl());
 llvm::DenseMap FieldLocs;
 
-for (auto [Field, Init] : llvm::zip(Fields, S->inits())) {
-  assert(Field != nullptr);
-  assert(Init != n

[PATCH] D159284: [clang][dataflow] Fix Record initialization with InitListExpr and inheritances

[Kinuko Yasuda via Phabricator via cfe-commits]

kinu added a comment.

Thanks, addressed the comments, PTAL~




Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:688-690
+  for ([[maybe_unused]] auto [Field, Loc] : FieldLocs) {
+assert(ModeledFields.contains(cast_or_null(Field)));
+  }

mboehme wrote:
> https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
Does it work even if assert() is dropped? ... looks like it does.  Ok, applied.



Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:672-673
+  // The types are same, or
+  GetCanonicalType(Field->getType()) ==
+  GetCanonicalType(Init->getType()) ||
+  // The field's type is T&, and initializer is T

mboehme wrote:
> mboehme wrote:
> > Again, this is the prvalue case, so I think you should simply be able to 
> > use `QualType::getCanonicalType()`.
> Thinking about this again -- the field can, of course, have qualifiers, while 
> the `Init` as a prvalue should not, so I think this should read
> 
> ```
> Field->getType().getCanonicalType().getUnqualifiedType() == 
> Init->getType().getCanonicalType()
> ```
> 
> Sorry for the back and forth!
No problem, thanks for clarifying the details, done.



Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:1528-1530
+  F.x4 = F.x2;
+  F.x6 = 1;
+  F.x8 = 1;

mboehme wrote:
> mboehme wrote:
> > These lines seem unnecessary. The test only wants to check whether the 
> > fields are modeled, not what their values are.
> > 
> > If anything, leaving out these operations makes the test stronger: It then 
> > tests whether the fields are modeled even if they are only initialized with 
> > an empty `{}`.
> > 
> > Then, once we leave out these operations, it's probably sufficient to give 
> > each class in this test just a single member variable (which is another 
> > nice simplification).
> Reopening the comment to discuss this line:
> 
> > Then, once we leave out these operations, it's probably sufficient to give 
> > each class in this test just a single member variable (which is another 
> > nice simplification).
> 
> I see that giving each class two member variables makes this test consistent 
> with the test above -- but OTOH, we're testing exactly the same thing for 
> both member variables, so maybe it's better on the whole to simplify the test 
> by putting just one member variable in each class?
That's true for this test... now I made each class have only one member.



Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:2191
+TEST(TransferTest, AssignmentOperatorWithInitAndInheritance) {
+  // This is a crash repro.
+  std::string Code = R"(

mboehme wrote:
> kinu wrote:
> > mboehme wrote:
> > > IIUC, the crash was happening because `copyRecord()` assumes that the 
> > > source and destination have the same fields. More specifically, there was 
> > > an unspoken invariant in the framework that a `RecordStorageLocation` 
> > > should always contain exactly the fields returned by 
> > > `DataflowAnalysisContext::getModeledFields()`), but our treatment of 
> > > `InitListExpr` was violating this invariant.
> > > 
> > > IOW, this wasn't really an issue with the way we treat copy/move 
> > > operations but with `InitlistExpr`.
> > > 
> > > I understand that we want to have a test the reproduces the exact 
> > > circumstances that led to the crash, but maybe it's enough to have one of 
> > > these, and have the focus of the testing be on testing the actual 
> > > invariant that we want to maintain? IOW, maybe keep this test but delete 
> > > the additional tests below?
> > Dropped the additional tests below.
> I still see more tests below? Suggest dropping these.
Ah I see what you mean.  I dropped more!



Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:2498
+TEST(TransferTest, ReturnStructWithInheritance) {
+  // This is a crash repro. (Returning a struct involves copy/move constructor)
+  std::string Code = R"(

mboehme wrote:
> kinu wrote:
> > mboehme wrote:
> > > I don't think this is true -- the `return S1` in `target()` will use NRVO.
> > > 
> > > But I also don't think it's relevant -- I believe the crash this is 
> > > reproducing was triggered by the `InitListExpr`, not the return statement?
> > > 
> > > Given the other tests added in this patch, do we need this test?
> > It will use NRVO but in AST this still seems to generate CXXConstructExpr 
> > with isCopyOrMoveConstructor() == true because omitting the ctor is not 
> > mandatory in compilers.
> > 
> > I can drop this one, but I'm also a bit torn because this was the original 
> > crash repro that puzzled me a bit.
> > 
> > I refined the comment to explain it a bit better; how does it look now?
> > It will use NRVO but in AST this still seems to generate

[PATCH] D149640: [clang][dataflow] Change PruneTriviallyFalseEdges for building CFG

[Kinuko Yasuda via Phabricator via cfe-commits]

kinu created this revision.
Herald added subscribers: martong, xazax.hun.
Herald added a reviewer: NoQ.
Herald added a project: All.
kinu requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

It looks like keeping this false could end up with extra iterations
on a lot of loops that aren't real ones (e.g. they could be a
do-while-false for macros).

This patch changes the default for
CFG::BuildOptions.PruneTriviallyFalseEdges to true to avoid it.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149640

Files:
  clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp


Index: clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
===
--- clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
+++ clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
@@ -70,7 +70,7 @@
 llvm::Expected
 ControlFlowContext::build(const Decl *D, Stmt &S, ASTContext &C) {
   CFG::BuildOptions Options;
-  Options.PruneTriviallyFalseEdges = false;
+  Options.PruneTriviallyFalseEdges = true;
   Options.AddImplicitDtors = true;
   Options.AddTemporaryDtors = true;
   Options.AddInitializers = true;


Index: clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
===
--- clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
+++ clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
@@ -70,7 +70,7 @@
 llvm::Expected
 ControlFlowContext::build(const Decl *D, Stmt &S, ASTContext &C) {
   CFG::BuildOptions Options;
-  Options.PruneTriviallyFalseEdges = false;
+  Options.PruneTriviallyFalseEdges = true;
   Options.AddImplicitDtors = true;
   Options.AddTemporaryDtors = true;
   Options.AddInitializers = true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149640: [clang][dataflow] Change PruneTriviallyFalseEdges for building CFG

[Kinuko Yasuda via Phabricator via cfe-commits]

kinu updated this revision to Diff 518676.
kinu added a comment.

Add a while-true test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149640/new/

https://reviews.llvm.org/D149640

Files:
  clang/lib/Analysis/FlowSensitive/ControlFlowContext.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
@@ -5271,4 +5271,22 @@
   });
 }
 
+TEST(TransferTest, UnreachedAfterWhileTrue) {
+  // PruneTriviallyFalseEdges should have pruned the unreached node.
+  std::string Code = R"(
+void target() {
+  while (true) {}
+  (void)0;
+  /*[[p]]*/
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+ASSERT_TRUE(Results.empty());
+  });
+}
+
+
 } // namespace
Index: clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
===
--- clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
+++ clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
@@ -70,7 +70,7 @@
 llvm::Expected
 ControlFlowContext::build(const Decl *D, Stmt &S, ASTContext &C) {
   CFG::BuildOptions Options;
-  Options.PruneTriviallyFalseEdges = false;
+  Options.PruneTriviallyFalseEdges = true;
   Options.AddImplicitDtors = true;
   Options.AddTemporaryDtors = true;
   Options.AddInitializers = true;


Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -5271,4 +5271,22 @@
   });
 }
 
+TEST(TransferTest, UnreachedAfterWhileTrue) {
+  // PruneTriviallyFalseEdges should have pruned the unreached node.
+  std::string Code = R"(
+void target() {
+  while (true) {}
+  (void)0;
+  /*[[p]]*/
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+ASSERT_TRUE(Results.empty());
+  });
+}
+
+
 } // namespace
Index: clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
===
--- clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
+++ clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
@@ -70,7 +70,7 @@
 llvm::Expected
 ControlFlowContext::build(const Decl *D, Stmt &S, ASTContext &C) {
   CFG::BuildOptions Options;
-  Options.PruneTriviallyFalseEdges = false;
+  Options.PruneTriviallyFalseEdges = true;
   Options.AddImplicitDtors = true;
   Options.AddTemporaryDtors = true;
   Options.AddInitializers = true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149640: [clang][dataflow] Change PruneTriviallyFalseEdges for building CFG

[Kinuko Yasuda via Phabricator via cfe-commits]

kinu added a comment.

In D149640#4311889 , @gribozavr2 
wrote:

> Is it testable? For example, can we show that we don't compute the program 
> state for the program point "p" here:

I added a simple test, how does it look?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149640/new/

https://reviews.llvm.org/D149640

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149640: [clang][dataflow] Change PruneTriviallyFalseEdges for building CFG

[Kinuko Yasuda via Phabricator via cfe-commits]

kinu updated this revision to Diff 518702.
kinu added a comment.

Changing the existing test to reflect the flag change


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149640/new/

https://reviews.llvm.org/D149640

Files:
  clang/lib/Analysis/FlowSensitive/ControlFlowContext.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
@@ -2652,25 +2652,8 @@
   Code,
   [](const llvm::StringMap> &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());
-
-const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
-ASSERT_THAT(BarDecl, NotNull());
-
-const auto *FooVal =
-cast(Env.getValue(*FooDecl, SkipPast::None));
-const auto *FooPointeeVal =
-cast(Env.getValue(FooVal->getPointeeLoc()));
-
-const auto *BarVal = dyn_cast_or_null(
-Env.getValue(*BarDecl, SkipPast::None));
-ASSERT_THAT(BarVal, NotNull());
-
-EXPECT_EQ(BarVal, FooPointeeVal);
+// PruneTriviallyFalseEdges prunes the unreachable node.
+ASSERT_TRUE(Results.empty());
   });
 }
 
Index: clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
===
--- clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
+++ clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
@@ -70,7 +70,7 @@
 llvm::Expected
 ControlFlowContext::build(const Decl *D, Stmt &S, ASTContext &C) {
   CFG::BuildOptions Options;
-  Options.PruneTriviallyFalseEdges = false;
+  Options.PruneTriviallyFalseEdges = true;
   Options.AddImplicitDtors = true;
   Options.AddTemporaryDtors = true;
   Options.AddInitializers = true;


Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -2652,25 +2652,8 @@
   Code,
   [](const llvm::StringMap> &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());
-
-const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
-ASSERT_THAT(BarDecl, NotNull());
-
-const auto *FooVal =
-cast(Env.getValue(*FooDecl, SkipPast::None));
-const auto *FooPointeeVal =
-cast(Env.getValue(FooVal->getPointeeLoc()));
-
-const auto *BarVal = dyn_cast_or_null(
-Env.getValue(*BarDecl, SkipPast::None));
-ASSERT_THAT(BarVal, NotNull());
-
-EXPECT_EQ(BarVal, FooPointeeVal);
+// PruneTriviallyFalseEdges prunes the unreachable node.
+ASSERT_TRUE(Results.empty());
   });
 }
 
Index: clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
===
--- clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
+++ clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
@@ -70,7 +70,7 @@
 llvm::Expected
 ControlFlowContext::build(const Decl *D, Stmt &S, ASTContext &C) {
   CFG::BuildOptions Options;
-  Options.PruneTriviallyFalseEdges = false;
+  Options.PruneTriviallyFalseEdges = true;
   Options.AddImplicitDtors = true;
   Options.AddTemporaryDtors = true;
   Options.AddInitializers = true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149640: [clang][dataflow] Change PruneTriviallyFalseEdges for building CFG

[Kinuko Yasuda via Phabricator via cfe-commits]

kinu added a comment.

(comment only)




Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:2673
-
-EXPECT_EQ(BarVal, FooPointeeVal);
   });

mboehme wrote:
> It's unfortuante that all of these checks have gone away. I think the test 
> was actually trying to test something.
> 
> I'd suggest checking the environment at two different places:
> 
> ```
> void target(int *Foo) {
>   do {
> int Bar = *Foo;
> // [[in_loop]]
>   } while (true);
>   (void)0;
>   // [[after_loop]]
> }
> ```
> 
> You can keep the existing checks for the `in_loop` environment and verify 
> that `Results` doesn't actually contain an environment for `after_loop`.
Wdyt if we change this to exercise `do { } while (false)` instead (with the 
checks that we already have), and add a simple while (true) {}?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149640/new/

https://reviews.llvm.org/D149640

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149640: [clang][dataflow] Change PruneTriviallyFalseEdges for building CFG

[Kinuko Yasuda via Phabricator via cfe-commits]

kinu updated this revision to Diff 518762.
kinu added a comment.

Updated changes further not to lose the existing checks with the do-while test.

Also restored the simplest test that exercises the while-true one.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149640/new/

https://reviews.llvm.org/D149640

Files:
  clang/lib/Analysis/FlowSensitive/ControlFlowContext.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
@@ -2643,7 +2643,7 @@
 void target(int *Foo) {
   do {
 int Bar = *Foo;
-  } while (true);
+  } while (false);
   (void)0;
   /*[[p]]*/
 }
@@ -2674,6 +2674,24 @@
   });
 }
 
+TEST(TransferTest, UnreachableAfterWhileTrue) {
+  std::string Code = R"(
+void target() {
+  while (true) {}
+  (void)0;
+  /*[[p]]*/
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+// `after_loop` is pruned by PruneTriviallyFalseEdges, so we only 
should
+// have `in_loop`.
+ASSERT_TRUE(Results.empty());
+  });
+}
+
 TEST(TransferTest, AggregateInitialization) {
   std::string BracesCode = R"(
 struct A {
Index: clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
===
--- clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
+++ clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
@@ -70,7 +70,7 @@
 llvm::Expected
 ControlFlowContext::build(const Decl *D, Stmt &S, ASTContext &C) {
   CFG::BuildOptions Options;
-  Options.PruneTriviallyFalseEdges = false;
+  Options.PruneTriviallyFalseEdges = true;
   Options.AddImplicitDtors = true;
   Options.AddTemporaryDtors = true;
   Options.AddInitializers = true;


Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -2643,7 +2643,7 @@
 void target(int *Foo) {
   do {
 int Bar = *Foo;
-  } while (true);
+  } while (false);
   (void)0;
   /*[[p]]*/
 }
@@ -2674,6 +2674,24 @@
   });
 }
 
+TEST(TransferTest, UnreachableAfterWhileTrue) {
+  std::string Code = R"(
+void target() {
+  while (true) {}
+  (void)0;
+  /*[[p]]*/
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+// `after_loop` is pruned by PruneTriviallyFalseEdges, so we only should
+// have `in_loop`.
+ASSERT_TRUE(Results.empty());
+  });
+}
+
 TEST(TransferTest, AggregateInitialization) {
   std::string BracesCode = R"(
 struct A {
Index: clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
===
--- clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
+++ clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
@@ -70,7 +70,7 @@
 llvm::Expected
 ControlFlowContext::build(const Decl *D, Stmt &S, ASTContext &C) {
   CFG::BuildOptions Options;
-  Options.PruneTriviallyFalseEdges = false;
+  Options.PruneTriviallyFalseEdges = true;
   Options.AddImplicitDtors = true;
   Options.AddTemporaryDtors = true;
   Options.AddInitializers = true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149640: [clang][dataflow] Change PruneTriviallyFalseEdges for building CFG

[Kinuko Yasuda via Phabricator via cfe-commits]

kinu updated this revision to Diff 519029.
kinu added a comment.

Updated the stale comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149640/new/

https://reviews.llvm.org/D149640

Files:
  clang/lib/Analysis/FlowSensitive/ControlFlowContext.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
@@ -2643,7 +2643,7 @@
 void target(int *Foo) {
   do {
 int Bar = *Foo;
-  } while (true);
+  } while (false);
   (void)0;
   /*[[p]]*/
 }
@@ -2674,6 +2674,24 @@
   });
 }
 
+TEST(TransferTest, UnreachableAfterWhileTrue) {
+  std::string Code = R"(
+void target() {
+  while (true) {}
+  (void)0;
+  /*[[p]]*/
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+// The node after the while-true is pruned because it is trivially
+// known to be unreachable.
+ASSERT_TRUE(Results.empty());
+  });
+}
+
 TEST(TransferTest, AggregateInitialization) {
   std::string BracesCode = R"(
 struct A {
Index: clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
===
--- clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
+++ clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
@@ -70,7 +70,7 @@
 llvm::Expected
 ControlFlowContext::build(const Decl *D, Stmt &S, ASTContext &C) {
   CFG::BuildOptions Options;
-  Options.PruneTriviallyFalseEdges = false;
+  Options.PruneTriviallyFalseEdges = true;
   Options.AddImplicitDtors = true;
   Options.AddTemporaryDtors = true;
   Options.AddInitializers = true;


Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -2643,7 +2643,7 @@
 void target(int *Foo) {
   do {
 int Bar = *Foo;
-  } while (true);
+  } while (false);
   (void)0;
   /*[[p]]*/
 }
@@ -2674,6 +2674,24 @@
   });
 }
 
+TEST(TransferTest, UnreachableAfterWhileTrue) {
+  std::string Code = R"(
+void target() {
+  while (true) {}
+  (void)0;
+  /*[[p]]*/
+}
+  )";
+  runDataflow(
+  Code,
+  [](const llvm::StringMap> &Results,
+ ASTContext &ASTCtx) {
+// The node after the while-true is pruned because it is trivially
+// known to be unreachable.
+ASSERT_TRUE(Results.empty());
+  });
+}
+
 TEST(TransferTest, AggregateInitialization) {
   std::string BracesCode = R"(
 struct A {
Index: clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
===
--- clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
+++ clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
@@ -70,7 +70,7 @@
 llvm::Expected
 ControlFlowContext::build(const Decl *D, Stmt &S, ASTContext &C) {
   CFG::BuildOptions Options;
-  Options.PruneTriviallyFalseEdges = false;
+  Options.PruneTriviallyFalseEdges = true;
   Options.AddImplicitDtors = true;
   Options.AddTemporaryDtors = true;
   Options.AddInitializers = true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149640: [clang][dataflow] Change PruneTriviallyFalseEdges for building CFG

[Kinuko Yasuda via Phabricator via cfe-commits]

kinu marked an inline comment as done.
kinu added inline comments.



Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:2690
+// `after_loop` is pruned by PruneTriviallyFalseEdges, so we only 
should
+// have `in_loop`.
+ASSERT_TRUE(Results.empty());

mboehme wrote:
> This comment looks stale?
Good catch, updated!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149640/new/

https://reviews.llvm.org/D149640

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152732: [clang][dataflow] Support limits on the SAT solver to force timeouts.

[Kinuko Yasuda via Phabricator via cfe-commits]

kinu added a comment.

LGTM as well, I was initially thinking about having a local limit per query 
(which could be easier to pinpoint the particular query that explodes), but 
per-solver instance limit could make sense as a starter too.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152732/new/

https://reviews.llvm.org/D152732

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits