[PATCH] D144273: [clang][ASTImporter] Add VaList declaration to lookup table.

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

@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 <https://github.com/llvm/llvm-project/commit/87f540608106f28d9319b2148eb1b00192e020c2#diff-af1009156039bf1cca0adb74b8c38c7a4e9eacf7a570f1a47fd95261fe6dfc08> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144273