[clang][dataflow] Fix crash when struct with inheritance is initialized with InitExpr

[kinu]

Summary

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 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. A follow-up change can add more proper fix
around InitListExpr.

[kinu]

updated comment

clang/include/clang/Analysis/FlowSensitive/StorageLocation.h

 
 #ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_STORAGELOCATION_H
 #define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_STORAGELOCATION_H
 
 #include "clang/AST/Decl.h"
 #include "clang/AST/Type.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/Support/Debug.h"
+#include "llvm/Support/ErrorOr.h"
 #include <cassert>
 
 #define DEBUG_TYPE "dataflow"
 
 namespace clang {
 namespace dataflow {
 
 /// Base class for elements of the local variable store and of the heap.
LLVM_DEBUG({
           llvm::dbgs() << Field->getNameAsString() << "\n";
         }
       }
     });
     assert(It != Children.end());
     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 another record.
+  llvm::ErrorOr<StorageLocation *> getChildOrError(const ValueDecl &D) const {
+    auto It = Children.find(&D);
+    if (It == Children.end())
+      return std::errc::invalid_argument;
+    return It->second;
+  }
+
   /// 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.
   ///
   /// Requirements:
   ///
   ///  `D` must have reference type.
   void setChild(const ValueDecl &D, StorageLocation *Loc) {

clang/lib/Analysis/FlowSensitive/RecordOps.cpp

LLVM_DEBUG({
     if (!compatibleTypes) {
       llvm::dbgs() << "Source type " << Src.getType() << "\n";
       llvm::dbgs() << "Destination type " << Dst.getType() << "\n";
     }
   });
   assert(compatibleTypes);
 
   for (auto [Field, DstFieldLoc] : Dst.children()) {
-    StorageLocation *SrcFieldLoc = Src.getChild(*Field);
+    llvm::ErrorOr<StorageLocation *> SrcField = Src.getChildOrError(*Field);
+    if (!SrcField)
+      continue;
+    StorageLocation *SrcFieldLoc = SrcField.get();
 
     assert(Field->getType()->isReferenceType() ||
            (SrcFieldLoc != nullptr && DstFieldLoc != nullptr));
 
     if (Field->getType()->isRecordType()) {
       copyRecord(cast<RecordStorageLocation>(*SrcFieldLoc),
                  cast<RecordStorageLocation>(*DstFieldLoc), Env);
     } else if (Field->getType()->isReferenceType()) {

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

 #include "clang/AST/Decl.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Analysis/FlowSensitive/DataflowAnalysisContext.h"
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
 #include "clang/Analysis/FlowSensitive/RecordOps.h"
 #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"
 #include <optional>
 #include <string>
 #include <utility>
 
 namespace {
runDataflow(
               cast<IntegerValue>(getFieldValue(FooLoc3, *BazDecl, Env3));
           const auto *BarBazVal3 =
               cast<IntegerValue>(getFieldValue(BarLoc3, *BazDecl, Env3));
           EXPECT_NE(FooBazVal3, BarBazVal3);
         }
       });
 }
 
+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<DataflowAnalysisState<NoopLattice>> &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 {};
     A ReturnA();
 
     void target() {
       A MyA;
       MyA = ReturnA();
std::string Code = R"(
     }
   )";
   runDataflow(
       Code,
       [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
          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<DataflowAnalysisState<NoopLattice>> &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<DataflowAnalysisState<NoopLattice>> &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 {
 
     template <typename T> struct remove_reference      { using type = T; };
     template <typename T> struct remove_reference<T&>  { using type = T; };
     template <typename T> struct remove_reference<T&&> { using type = T; };
 
runDataflow(
         EXPECT_TRUE(recordsEqual(*FooLoc2, Env2, *BarLoc1, Env1));
 
         const auto *FooBazVal2 =
             cast<IntegerValue>(getFieldValue(FooLoc1, *BazDecl, Env2));
         EXPECT_EQ(FooBazVal2, BarBazVal1);
       });
 }
 
+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<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {});
+}
+
 TEST(TransferTest, BindTemporary) {
   std::string Code = R"(
     struct A {
       virtual ~A() = default;
 
       int Baz;
     };