[clang][ASTImporter] Add VaList declaration to lookup table.

[balazske]

Summary

The declaration of __builtin_va_list seems to be handled specially
by ASTContext and/or Sema. The normal AST traversal probably can
not find it, therefore it is not added to ASTImporterLookupTable.
If it is not added, errors can occur because a duplicated
VaList declaration is created at import. To fix the problem the
VaList declaration is added manually to ASTImporterLookupTable
at construction.
In special cases this declaration is inside a "invisible" 'std'
namespace that behaves the same way. This namespace must be added
to the table too.

[balazske]

removed a not needed line

[donat.nagy]

Note that the Debian x64 test failure is from the unit test

TEST(ClangdServer, MemoryUsageTest) {
  MockFS FS;
  MockCompilationDatabase CDB;
  ClangdServer Server(CDB, FS, ClangdServer::optsForTest());

  auto FooCpp = testPath("foo.cpp");
  Server.addDocument(FooCpp, "");
  ASSERT_TRUE(Server.blockUntilIdleForTest());  // THIS ASSERTION FAILED

  llvm::BumpPtrAllocator Alloc;
  MemoryTree MT(&Alloc);
  Server.profile(MT);
  ASSERT_TRUE(MT.children().count("tuscheduler"));
  EXPECT_TRUE(MT.child("tuscheduler").children().count(FooCpp));
}

(from clang-tools-extra/clangd/unittests/ClangdTests.cpp lines 1244-1258). I'm not familiar with this area but it's unrelated to the commit and words like "scheduler" and "block until idle" suggest that this unit test is not entirely deterministic, so it's plausible that its failure is not connected to the commit under review. Is there a possibility to retrigger test execution to see whether this error reappears on another run?

[donat.nagy]

@gamesh411 and I tried to understand the consequences of this change. We think it might be problematic that calling ASTContext::getVaListTagDecl [1] creates the VaList declaration (and on certain platforms, the declaration of std) even in the cases when these declarations do not appear in the translation units and wouldn't be imported. (This is analogous to the issues caused by https://reviews.llvm.org/D142822, but more limited in scope. That commit modified Sema, so it "leaked" these declarations practically everywhere; I fear that this change might analogously leak these declarations into the ASTs that are affected by import operations.)

Following the example of the testcase p2-nodef.cpp I'd suggest adding 1-2 testcases to check that the ASTImporter machinery does not leak superfluous declarations. For example, the presence of int std = 20; (in either the source or the target of the AST import) should not cause name collisions even on architectures (e.g. AArch64) where the VaList type is defined in the std namespace.

These testcases would be a valuable addition even if they'd pass with the current implementation, because they'd safeguard against leaks introduced by later changes of this logic. However, I'd guess that there is a significant chance (>30%) that this test would fail; in that case we should look for a different solution that acts in the moment when a va_list is actually imported instead of trying to "prepare the ground" by prematurely adding some declarations.

[1] Note that this isn't a pure getter, it mutates the ASTContext object in the case when __va_list_tag isn't declared yet.

[balazske]

Probably we can add a new function ASTContext::getVaListTagDeclIfExists that does not create declarations if these do not exist. These new mentioned test cases would probably fail, but with a new non-modifying get function it can work.

[donat.nagy]

@balazske Thanks for clarifying the side effects of the current solution and I support creating that side-effect-free getter function -- it sounds to be a good solution for this problem. One minor doubt: I can theoretically imagine the case when the TU object doesn't contain the VaList declaration yet when the constructor of ASTImporterLookupTable is called (so the side-effect-free call does nothing), but then it somehow creates duplicated decls by importing VaList twice (e.g. via two imports)? (I think this is unlikely, but I'm unfamiliar with the usage patterns of ASTImporterLookupTable, so it'd be easier for you to verify that this kind of failure can't happen.)

Also consider renaming the current getVaListTagDecl to e.g. getOrCreateVaListTagDecl (it wouldn't blow up the commit size, there are only six references to it in the whole repo) and reusing the name getVaListTagDecl for the side-effect-free getter variant. (The ...IfExists name variant is also acceptable and there are some precedents for it in the codebase, but I think it'd be less surprising on the long term if we reduced the amount of innocent-looking getSomething functions that are mutating our state.)

I still think it'd be good extend the patch with the "can we import int std" testcase(s), just to be sure. (Although obviously there is no need to run them on the current implementation if you think they'll fail and you already have an alternative solution.)

[vabridgers]

I tested this change in the cases that previously failed for us, and this patch addresses our problems. It would be good to note in the commit header that this patch is intended to fix the problem first described by review D136886. Finally, this should be submitted as part of a patch stack intended to address the 2 AST importer problems seen and corrected, originating from D133468.

[balazske]

Add a test.

[balazske]

A tried to find out how to add a correct test but could not check if this fails or not on AArch64 platform. The test should import the va_list declarations and then another variable std. I want to touch ASTContext only if a test failure is found on AArch64 that makes it necessary. In that case I would add a non-modifying function getVaListTagDeclIfExists, another option is to change all other simiar get functions to "getOrCreate" form to have consistent API.

[vabridgers]

LGTM. Let's accept, merge and then watch to make sure we can keep the change.

[donat.nagy]

@balazske

I tried to find out how to add a correct test but could not check if this fails or not on AArch64 platform. [...] I want to touch ASTContext only if a test failure is found on AArch64 that makes it necessary.

It's possible to "simulate" the AArch64 behavior locally by temporarily modifying ASTContext.cpp to make CreateX86_64ABIBuiltinVaListDecl create the std namespace on x86_64 (using the code fragment taken from the AArch64-specific function). I tried to do this with a simple unit test, but my results are inconclusive (my test passed, but I'm not sure that it would've reproduced the error).

Consider using this hack to test the commit instead of relying on the centrally executed AArch64 tests.

clang/test/Import/cxx-valist/Inputs/I1.cpp

1–3 ↗

(On Diff #501514)

I don't think that this "import this innocent file before importing int std" step is relevant for triggering the potentially problematic behavior that I outlined in my comments. Personally I'd remove this file from the commit (either keeping the the other two files as a simpler testcase that verifies that int std can be imported or converting that logic into a unit test); but I don't have a complete understanding of the situation, so if you feel that this is relevant, then I strongly support keeping it.

However, in that case I'd use a very simple "dummy" function like int f(int x) { return x; } instead of this use_new to keep this testcase independent of the "does the type of new leak the declaration of the std namespace?" question. (That was resolved in 2009 by commit 87f54060 but a regression on that front could cause a misleading failure of this testcase.)

[balazske]

Changed the lit test.

[balazske]

The new test should be better, it imports the f function without new and no VaList should exist in the imported translation unit. It looks like the test.cpp is appended to the AST after the import of the other files. I changed function CreateVaListDecl to always use the AArch64 create function, the test did not fail.

[donat.nagy]

LGTM, the test result sounds promising, let's hope that it'll work in the CI as well.

clang/lib/AST/ASTImporterLookupTable.cpp

 //===- ASTImporterLookupTable.cpp - ASTImporter specific lookup -----------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
struct Builder : RecursiveASTVisitor<Builder> {
   bool shouldVisitImplicitCode() const { return true; }
 };
 
 } // anonymous namespace
 
 ASTImporterLookupTable::ASTImporterLookupTable(TranslationUnitDecl &TU) {
   Builder B(*this);
   B.TraverseDecl(&TU);
+  // The VaList declaration may be created on demand only or not traversed.
+  // To ensure it is present and found during import, add it to the table now.
+  if (auto *D =
+          dyn_cast_or_null<NamedDecl>(TU.getASTContext().getVaListTagDecl())) {
+    // On some platforms (AArch64) the VaList declaration can be inside a 'std'
+    // namespace. This is handled specially and not visible by AST traversal.
+    // ASTImporter must be able to find this namespace to import the VaList
+    // declaration (and the namespace) correctly.
+    if (auto *Ns = dyn_cast<NamespaceDecl>(D->getDeclContext()))
+      add(&TU, Ns);
+    add(D->getDeclContext(), D);
+  }
 }
 
 void ASTImporterLookupTable::add(DeclContext *DC, NamedDecl *ND) {
   DeclList &Decls = LookupTable[DC][ND->getDeclName()];
   // Inserts if and only if there is no element in the container equal to it.
   Decls.insert(ND);
 }
 

clang/unittests/AST/ASTImporterTest.cpp

 //===- unittest/AST/ASTImporterTest.cpp - AST node import test ------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
TEST_P(ASTImporterOptionSpecificTestBase, isNewDecl) {
 
   auto *ToBar = FirstDeclMatcher<FunctionDecl>().match(
       ToTU, functionDecl(hasName("bar")));
 
   EXPECT_TRUE(SharedStatePtr->isNewDecl(ToOther));
   EXPECT_FALSE(SharedStatePtr->isNewDecl(ToBar));
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, VaListC) {
+  Decl *FromTU = getTuDecl(R"(typedef __builtin_va_list va_list;)", Lang_C99);
+
+  auto *FromVaList = FirstDeclMatcher<TypedefDecl>().match(
+      FromTU, typedefDecl(hasName("va_list")));
+  ASSERT_TRUE(FromVaList);
+
+  auto *ToVaList = Import(FromVaList, Lang_C99);
+  ASSERT_TRUE(ToVaList);
+
+  auto *ToBuiltinVaList = FirstDeclMatcher<TypedefDecl>().match(
+      ToAST->getASTContext().getTranslationUnitDecl(),
+      typedefDecl(hasName("__builtin_va_list")));
+
+  ASSERT_TRUE(ToAST->getASTContext().hasSameType(
+      ToVaList->getUnderlyingType(), ToBuiltinVaList->getUnderlyingType()));
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, VaListCpp) {
+  Decl *FromTU = getTuDecl(R"(typedef __builtin_va_list va_list;)", Lang_CXX03);
+
+  auto *FromVaList = FirstDeclMatcher<TypedefDecl>().match(
+      FromTU, typedefDecl(hasName("va_list")));
+  ASSERT_TRUE(FromVaList);
+
+  auto *ToVaList = Import(FromVaList, Lang_CXX03);
+  ASSERT_TRUE(ToVaList);
+
+  auto *ToBuiltinVaList = FirstDeclMatcher<TypedefDecl>().match(
+      ToAST->getASTContext().getTranslationUnitDecl(),
+      typedefDecl(hasName("__builtin_va_list")));
+
+  ASSERT_TRUE(ToAST->getASTContext().hasSameType(
+      ToVaList->getUnderlyingType(), ToBuiltinVaList->getUnderlyingType()));
+}
+
 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);