[clang][ASTImporter] fix variable inline of CXX17

[jcsxky]

Fix crash in the testcase from #75114 (comment)
Forget to set inline of variable declaration would make isThisDeclarationADefinition get incorrect result and didn't get imported variable. This will lead to a new VarTemplateDecl being created and call setDescribedVarTemplate again which produces the crash.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Qizhi Hu (jcsxky)

Changes

Fix crash int the testcase from #75114 (comment)
Forget to set inline of variable declaration would make isThisDeclarationADefinition get incorrect result and didn't get imported variable. This will lead to a new VarTemplateDecl being created and call setDescribedVarTemplate again which produce the crash.

---

Full diff: https://github.com/llvm/llvm-project/pull/87314.diff

2 Files Affected:

• (modified) clang/lib/AST/ASTImporter.cpp (+7-2)
• (modified) clang/unittests/AST/ASTImporterTest.cpp (+28)

diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index 94a47a8f619018..cfdb2cb5c778e3 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -4579,7 +4579,12 @@ ExpectedDecl ASTNodeImporter::VisitVarDecl(VarDecl *D) {
     if (!RedeclOrErr)
       return RedeclOrErr.takeError();
   }
-
+  if (D->isInlineSpecified()) {
+    ToVar->setInlineSpecified();
+  }
+  if (D->isInline()) {
+    ToVar->setImplicitlyInline();
+  }
   return ToVar;
 }
 
@@ -6410,7 +6415,7 @@ ExpectedDecl ASTNodeImporter::VisitVarTemplateDecl(VarTemplateDecl *D) {
     }
     ToVarTD->setPreviousDecl(Recent);
   }
-
+  // Importer.MapImported(D, ToVarTD);
   return ToVarTD;
 }
 
diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp
index 35ab7e3b7fe314..d57736830f0223 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -5317,6 +5317,34 @@ TEST_P(ASTImporterOptionSpecificTestBase,
   EXPECT_FALSE(ToX);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateDeclInlineWithCXX17) {
+  Decl *FromTU = getTuDecl(
+      R"(
+      struct S {
+        template <unsigned> static constexpr bool X = true;
+      };
+      )",
+      Lang_CXX17, "input1.cc");
+  Decl *FromTU2 = getTuDecl(
+      R"(
+      struct S {
+        template <unsigned> static constexpr bool X = true;
+        template <typename T> void get() { X<sizeof(T)>; }
+      };
+      template <typename T> T qvariant_cast(const S &v) { return v.get; }
+      )",
+      Lang_CXX17, "input2.cc");
+  auto *FromX = FirstDeclMatcher<VarTemplateDecl>().match(
+      FromTU, varTemplateDecl(hasName("X")));
+  auto *ToX = Import(FromX, Lang_CXX17);
+  EXPECT_TRUE(ToX);
+  auto *FromX2 = FirstDeclMatcher<VarTemplateDecl>().match(
+      FromTU2, varTemplateDecl(hasName("X")));
+  auto *ToX2 = Import(FromX2, Lang_CXX17);
+  EXPECT_TRUE(ToX2);
+  EXPECT_TRUE(ToX == ToX2);
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateParameterDeclContext) {
   constexpr auto Code =
       R"(

[NagyDonat]

Looks good, but I'm not familiar enough with these details of the standard to provide a definite review.

[NagyDonat]

Suggested change

	template <typename T> T qvariant_cast(const S &v) { return v.get; }
	template <typename U> U qvariant_cast(const S &v) { return v.get; }

At first I thought that the template argument typename T of this definition is intended to correspond to the same type as the typename T in the definition of get, while it seems to be a different type, and if this is true, then it'd be better to use a different name. (By the way, is it intentional that you return v.get without calling it?)

[jcsxky]

I have no idea what these code wants to do and just pick up from the comment. It can reproduce the crash and demonstrate this fix works(I think). The type name has been updated.

[steakhal]

I only had generic nits, as I'm not really in the domain. Consider my review as such.

[balazske]

I would like to move these lines to line 4513 (after ToVar->setLexicalDeclContext(LexicalDC);), and braces are not needed:

  if (D->isInlineSpecified())
    ToVar->setInlineSpecified();
  if (D->isInline())
    ToVar->setImplicitlyInline();

[jcsxky]

Seems to be more reasonable. Code has been updated. Thanks!

[balazske]

For me the test and fix looks good, test crashes without the fix (llvm-project/clang/lib/AST/ASTContext.cpp:1474: void clang::ASTContext::setTemplateOrSpecializationInfo(clang::VarDecl*, clang::ASTContext::TemplateOrSpecializationInfo): Assertion `!TemplateOrInstantiation[Inst] && "Already noted what the variable was instantiated from"' failed.).

(One very small thing is that after the second import in the test EXPECT_TRUE would be better.)

[jcsxky]

For me the test and fix looks good, test crashes without the fix (llvm-project/clang/lib/AST/ASTContext.cpp:1474: void clang::ASTContext::setTemplateOrSpecializationInfo(clang::VarDecl*, clang::ASTContext::TemplateOrSpecializationInfo): Assertion `!TemplateOrInstantiation[Inst] && "Already noted what the variable was instantiated from"' failed.).

(One very small thing is that after the second import in the test EXPECT_TRUE would be better.)

Fixed!