[clang][ASTImporter] Fix a bug when importing CXXDefaultInitExpr.

[balazske]

Summary

The "in-class initializer" expression should be set in the field of a
default initialization expression before this expression node is created.
The CXXDefaultInitExpr objects are created after the AST is loaded and
at import not present in the "To" AST. And the in-class initializers of
the used fields can be missing too, these must be set at import.

This fixes a github issue #54061.

[balazske]

Improved the test to fix failure on Windows.

[steakhal]

Looks good to me.

[martong]

So, my understanding is that the issue stems from the fact that hasInClassInitializer() gave inconsistent results with getInClassInitializer() for previously imported nodes. Please confirm. For that, the solution you provide seems reasonable and the test cases gives a proper coverage.

clang/lib/AST/ASTImporter.cpp

3649–3650

It is unfortunate to have such an API. Anyway, the assertions you placed seems to address this mess. Good.

clang/unittests/AST/ASTImporterTest.cpp

536–547

So these tests failed with the baseline?

[balazske]

In D120824#3369407, @martong wrote:

So, my understanding is that the issue stems from the fact that hasInClassInitializer() gave inconsistent results with getInClassInitializer() for previously imported nodes.

Not really the API is the problem. The real problem was that to create a CXXDefaultInitExpr the field should have a "in-class initializer". CXXDefaultInitExpr has a pointer to the field that is initialized at that place. The field has an "in-class initializer", this is the used expression to initialize the field (CXXDefaultInitExpr is a separate object that is replicated for every initialization in constructors and initializer-list). The in-class initializer expression is not always stored in the AST, in the ToTU it is missing initially. The field has the flag set that it contains in-class initializer but the actual expression is not set yet (probably because the code parser works this way). This expression should be imported before a CXXDefaultInitExpr can be created.

The other code change (at lines 3650-60) is needed because setInClassInitializer can be called only if the value is not set already, otherwise it will assert.

clang/unittests/AST/ASTImporterTest.cpp

536–547

This test was added only because there was no test for CXXDefaultInitExpr. Probably this test does not fail without the fix. The failing test is only the added lit test.

[martong]

Okay, thanks for the explanation.

The in-class initializer expression is not always stored in the AST, in the ToTU it is missing initially. The field has the flag set that it contains in-class initializer but the actual expression is not set yet (probably because the code parser works this way).

So, the parser can really create such a node when the expression is not set? That seems like a bug in the parser. Anyway, would be nice to see a test case for it and perhaps fix in the parser.
But I have doubts that it would be a parser issue. I wonder whether it is rather this case: first we do an import that fails to set the expression and then the subsequent import crashes?

[balazske]

I think that the problem may be related to the fact that the in-class initializer is not used by the code in the "To" AST (in ctu-cxxdefaultinitexpr.cpp the problem is with QMultiHash::d field). Probably the expression node in the field is set only if it is used by any CXXDefaultInitExpr, but the hasInClassInitializer value is set always if the code contains in-class initializer. I can try to make a unit test for this case (but the lit test is good to have because it is more complex and can reveal other problems).

[martong]

In D120824#3369563, @balazske wrote:

I think that the problem may be related to the fact that the in-class initializer is not used by the code in the "To" AST (in ctu-cxxdefaultinitexpr.cpp the problem is with QMultiHash::d field). Probably the expression node in the field is set only if it is used by any CXXDefaultInitExpr, but the hasInClassInitializer value is set always if the code contains in-class initializer. I can try to make a unit test for this case ...

Thanks, that'd be great.

... (but the lit test is good to have because it is more complex and can reveal other problems).

I agree.

[balazske]

Added unit tests.

[balazske]

The last unit test is not much different from the lit test, I could not find more simple code to get the wanted result. Is it still good to have the lit test too?

[balazske]

Ping
I had a question about if the lit-test is needed with the new unit tests (that contain essentially similar code). But if there is no answer I will commit it with all tests, more test is always better.

clang/lib/AST/ASTImporter.cpp

if (FieldDecl *FoundField = dyn_cast<FieldDecl>(FoundDecl)) {
 
       if (Importer.IsStructurallyEquivalent(D->getType(),
                                             FoundField->getType())) {
         Importer.MapImported(D, FoundField);
         // In case of a FieldDecl of a ClassTemplateSpecializationDecl, the
         // initializer of a FieldDecl might not had been instantiated in the
         // "To" context.  However, the "From" context might instantiated that,
         // thus we have to merge that.
+        // Note: `hasInClassInitializer()` is not the same as non-null
+        // `getInClassInitializer()` value.
         if (Expr *FromInitializer = D->getInClassInitializer()) {
-          // We don't have yet the initializer set.
-          if (FoundField->hasInClassInitializer() &&
-              !FoundField->getInClassInitializer()) {
-            if (ExpectedExpr ToInitializerOrErr = import(FromInitializer))
+          if (ExpectedExpr ToInitializerOrErr = import(FromInitializer)) {
+            // Import of the FromInitializer may result in the setting of
+            // InClassInitializer. If not, set it here.
+            assert(FoundField->hasInClassInitializer() &&
+                   "Field should have an in-class initializer if it has an "
+                   "expression for it.");
+            if (!FoundField->getInClassInitializer())
               FoundField->setInClassInitializer(*ToInitializerOrErr);
-            else {
-              // We can't return error here,
-              // since we already mapped D as imported.
-              // FIXME: warning message?
-              consumeError(ToInitializerOrErr.takeError());
-              return FoundField;
-            }
-          }
-        }
+          } else {
         return FoundField;
       }
 
       // FIXME: Why is this case not handled with calling HandleNameConflict?
       Importer.ToDiag(Loc, diag::warn_odr_field_type_inconsistent)
         << Name << D->getType() << FoundField->getType();
       Importer.ToDiag(FoundField->getLocation(), diag::note_odr_value_here)
         << FoundField->getType();
ExpectedStmt ASTNodeImporter::VisitCXXDefaultInitExpr(CXXDefaultInitExpr *E) {
   auto ToFieldOrErr = import(E->getField());
   if (!ToFieldOrErr)
     return ToFieldOrErr.takeError();
 
   auto UsedContextOrErr = Importer.ImportContext(E->getUsedContext());
   if (!UsedContextOrErr)
     return UsedContextOrErr.takeError();
 
-  return CXXDefaultInitExpr::Create(
-      Importer.getToContext(), *ToBeginLocOrErr, *ToFieldOrErr, *UsedContextOrErr);
+  FieldDecl *ToField = *ToFieldOrErr;
+  assert(ToField->hasInClassInitializer() &&
+         "Field should have in-class initializer if there is a default init "
+         "expression that uses it.");
+  if (!ToField->getInClassInitializer()) {
+    // The in-class initializer may be not yet set in "To" AST even if the
+    // field is already there. This must be set here to make construction of
+    // CXXDefaultInitExpr work.
+    auto ToInClassInitializerOrErr =
+        import(E->getField()->getInClassInitializer());
+    if (!ToInClassInitializerOrErr)
+      return ToInClassInitializerOrErr.takeError();
+    ToField->setInClassInitializer(*ToInClassInitializerOrErr);
+  }
+
+  return CXXDefaultInitExpr::Create(Importer.getToContext(), *ToBeginLocOrErr,
+                                    ToField, *UsedContextOrErr);
 }
 
 ExpectedStmt ASTNodeImporter::VisitCXXNamedCastExpr(CXXNamedCastExpr *E) {
   Error Err = Error::success();
   auto ToType = importChecked(Err, E->getType());
   auto ToSubExpr = importChecked(Err, E->getSubExpr());
   auto ToTypeInfoAsWritten = importChecked(Err, E->getTypeInfoAsWritten());
   auto ToOperatorLoc = importChecked(Err, E->getOperatorLoc());

clang/test/Analysis/Inputs/ctu-cxxdefaultinitexpr-import.cpp

+namespace QHashPrivate {
+template <typename> int b;
+struct Data;
+} // namespace QHashPrivate
+
+struct QDomNodePrivate {};
+template <typename = struct QString> struct QMultiHash {
+  QHashPrivate::Data *d = nullptr;
+};
+
+struct QDomNamedNodeMapPrivate {
+  QMultiHash<> map;
+};
+struct QDomElementPrivate : QDomNodePrivate {
+  QDomElementPrivate();
+  void importee();
+  QMultiHash<> *m_attr = nullptr;
+};
+// --------- common part end ---------
+
+QDomElementPrivate::QDomElementPrivate() : m_attr{new QMultiHash<>} {}
+void QDomElementPrivate::importee() { (void)QMultiHash<>{}; }
+struct foo {
+  QDomElementPrivate m = {};
+  static const int value = (QHashPrivate::b<foo>, 22);
+};

clang/test/Analysis/Inputs/ctu-cxxdefaultinitexpr-import.cpp.externalDefMap.ast-dump.txt

+14:c:@S@foo@value ctu-cxxdefaultinitexpr-import.cpp.ast
+35:c:@S@QDomElementPrivate@F@importee# ctu-cxxdefaultinitexpr-import.cpp.ast
+45:c:@S@QDomElementPrivate@F@QDomElementPrivate# ctu-cxxdefaultinitexpr-import.cpp.ast
+39:c:@S@QDomNodePrivate@F@QDomNodePrivate# ctu-cxxdefaultinitexpr-import.cpp.ast

clang/test/Analysis/ctu-cxxdefaultinitexpr.cpp

+// RUN: rm -rf %t && mkdir %t
+// RUN: mkdir -p %t/ctudir
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -std=c++17 \
+// RUN:   -emit-pch -o %t/ctudir/ctu-cxxdefaultinitexpr-import.cpp.ast %S/Inputs/ctu-cxxdefaultinitexpr-import.cpp
+// RUN: cp %S/Inputs/ctu-cxxdefaultinitexpr-import.cpp.externalDefMap.ast-dump.txt %t/ctudir/externalDefMap.txt
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -std=c++17 -analyze \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config experimental-enable-naive-ctu-analysis=true \
+// RUN:   -analyzer-config ctu-dir=%t/ctudir \
+// RUN:   -verify %s
+
+// Check that importing this code does not cause crash.
+// expected-no-diagnostics
+
+namespace QHashPrivate {
+template <typename> int b;
+struct Data;
+} // namespace QHashPrivate
+
+struct QDomNodePrivate {};
+template <typename = struct QString> struct QMultiHash {
+  QHashPrivate::Data *d = nullptr;
+};
+
+struct QDomNamedNodeMapPrivate {
+  QMultiHash<> map;
+};
+struct QDomElementPrivate : QDomNodePrivate {
+  QDomElementPrivate();
+  void importee();
+  QMultiHash<> *m_attr = nullptr;
+};
+// --------- common part end ---------
+
+void importer(QDomElementPrivate x) { x.importee(); }

clang/unittests/AST/ASTImporterTest.cpp

testImport(
           has(initListExpr(
               hasType(asString("struct point")), has(floatLiteral(equals(1.0))),
               has(implicitValueInitExpr(hasType(asString("double")))))),
           has(initListExpr(hasType(asString("struct point")),
                            has(floatLiteral(equals(2.0))),
                            has(floatLiteral(equals(1.0)))))))));
 }
 
+const internal::VariadicDynCastAllOfMatcher<Expr, CXXDefaultInitExpr>
+    cxxDefaultInitExpr;
+
+TEST_P(ImportExpr, ImportCXXDefaultInitExpr) {
+  MatchVerifier<Decl> Verifier;
+  testImport("class declToImport { int DefInit = 5; }; declToImport X;",
+             Lang_CXX11, "", Lang_CXX11, Verifier,
+             cxxRecordDecl(hasDescendant(cxxConstructorDecl(
+                 hasAnyConstructorInitializer(cxxCtorInitializer(
+                     withInitializer(cxxDefaultInitExpr())))))));
+  testImport(
+      "struct X { int A = 5; }; X declToImport{};", Lang_CXX17, "", Lang_CXX17,
+      Verifier,
+      varDecl(hasInitializer(initListExpr(hasInit(0, cxxDefaultInitExpr())))));
+}
 
 const internal::VariadicDynCastAllOfMatcher<Expr, VAArgExpr> vaArgExpr;
 
 TEST_P(ImportExpr, ImportVAArgExpr) {
   MatchVerifier<Decl> Verifier;
   testImport("void declToImport(__builtin_va_list list, ...) {"
              "  (void)__builtin_va_arg(list, int); }",
              Lang_CXX03, "", Lang_CXX03, Verifier,
TEST_P(ASTImporterOptionSpecificTestBase,
   // Test that during import of 'foo' the record layout (of 'A') can be obtained
   // without crash. It is not possible to have all of the fields of 'A' imported
   // at that time (without big code changes).
   auto *ToA = Import(FromA, Lang_CXX11);
   EXPECT_TRUE(ToA);
   EXPECT_TRUE(ToA->isCompleteDefinition());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportInClassInitializerFromField) {
+  // Encounter import of a field when the field already exists but has the
+  // in-class initializer expression not yet set. Such case can occur in the AST
+  // of generated template specializations.
+  // The first code forces to create a template specialization of
+  // `A<int>` but without implicit constructors.
+  // The second ("From") code contains a variable of type `A<int>`, this
+  // results in a template specialization that has constructors and
+  // CXXDefaultInitExpr nodes.
+  Decl *ToTU = getToTuDecl(
+      R"(
+      void f();
+      template<typename> struct A { int X = 1; };
+      struct B { A<int> Y; };
+      )",
+      Lang_CXX11);
+  auto *ToX = FirstDeclMatcher<FieldDecl>().match(
+      ToTU,
+      fieldDecl(hasName("X"), hasParent(classTemplateSpecializationDecl())));
+  ASSERT_TRUE(ToX->hasInClassInitializer());
+  ASSERT_FALSE(ToX->getInClassInitializer());
+
+  Decl *FromTU = getTuDecl(
+      R"(
+      void f();
+      template<typename> struct A { int X = 1; };
+      struct B { A<int> Y; };
+      //
+      A<int> Z;
+      )",
+      Lang_CXX11, "input1.cc");
+  auto *FromX = FirstDeclMatcher<FieldDecl>().match(
+      FromTU,
+      fieldDecl(hasName("X"), hasParent(classTemplateSpecializationDecl())));
+
+  auto *ToXImported = Import(FromX, Lang_CXX11);
+  EXPECT_EQ(ToXImported, ToX);
+  EXPECT_TRUE(ToX->getInClassInitializer());
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase,
+       ImportInClassInitializerFromCXXDefaultInitExpr) {
+  // Encounter AST import of a CXXDefaultInitExpr where the "to-field"
+  // of it exists but has the in-class initializer not set yet.
+  Decl *ToTU = getToTuDecl(
+      R"(
+      namespace N {
+        template<typename> int b;
+        struct X;
+      }
+      template<typename> struct A { N::X *X = nullptr; };
+      struct B { A<int> Y; };
+      )",
+      Lang_CXX14);
+  auto *ToX = FirstDeclMatcher<FieldDecl>().match(
+      ToTU,
+      fieldDecl(hasName("X"), hasParent(classTemplateSpecializationDecl())));
+  ASSERT_TRUE(ToX->hasInClassInitializer());
+  ASSERT_FALSE(ToX->getInClassInitializer());
+
+  Decl *FromTU = getTuDecl(
+      R"(
+      namespace N {
+        template<typename> int b;
+        struct X;
+      }
+      template<typename> struct A { N::X *X = nullptr; };
+      struct B { A<int> Y; };
+      //
+      void f() {
+        (void)A<int>{};
+      }
+      struct C {
+        C(): attr(new A<int>{}){}
+        A<int> *attr;
+        const int value = N::b<C>;
+      };
+      )",
+      Lang_CXX14, "input1.cc");
+  auto *FromF = FirstDeclMatcher<FunctionDecl>().match(
+      FromTU, functionDecl(hasName("f"), isDefinition()));
+  auto *ToF = Import(FromF, Lang_CXX11);
+  EXPECT_TRUE(ToF);
+  EXPECT_TRUE(ToX->getInClassInitializer());
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
                          DefaultTestValuesForRunOptions);
 
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportPath,
                          ::testing::Values(std::vector<std::string>()));
 
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportExpr,
                          DefaultTestValuesForRunOptions);