[PATCH] D142822: [clang] ASTImporter: Fix importing of va_list types and declarations

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

With this fix the `std::__va_list` is created at initialization of Sema, previously it was not there. Function `CreateAArch64ABIBuiltinVaListDecl` (ASTContext.cpp) makes the namespace `std` and `__va_list` record. This change fixes the problem with `ASTImporter` probably because the __va_list record exists before start of AST import, this way it is found and added to `ASTImporterLookupTable` at initialization (the original problem was caused because `std::__va_list` is missing from `ASTImporterLookupTable`). But I was thinking of other ways to fix the problem. My old fix for the problem may still work without any test failures:

In D136886#3892261 <https://reviews.llvm.org/D136886#3892261>, @balazske wrote:

> `ASTImporterLookupTable` do not contain an entry for `__va_list_tag`, I do not know why it is missing. If it is added "manually" the crash disappears (without fix in `VisitTypedefType`). Following code was used to add VaListTagDecl:
>
>   ASTImporterLookupTable::ASTImporterLookupTable(TranslationUnitDecl &TU) {
>     Builder B(*this);
>     B.TraverseDecl(&TU);
>     // Add __va_list_tag to the table, it is not visited by the builder.
>     if (NamedDecl *D = dyn_cast_or_null<NamedDecl>(TU.getASTContext().getVaListTagDecl()))
>       add(&TU, D);
>   }
>
> The problem probably existed before but did not have visible effect until the new assertion was added.

It can happen (without any of the new fixes) that `std::__va_list` is created during the AST import process, it is created at first access. This can be the reason why it is missing from the lookup table: It is created implicitly by `ASTContext` code in the "to" context during import. To fix this problem, I think the above quoted code is a good solution: The va_list declaration is created at start of AST import (if it did not exist yet) (by the get function) and added to the lookup table. Probably it can be a problem that the va_list declaration and std namespace appears in a TU by just importing any code into it, but probably no tests will fail for this case. Another way to fix the problem is to add special handling of `std::__va_list` at least on the affected platforms to `ASTImporterLookupTable` but I did not check of this can be done. I do not like to change function `ASTImporter::VisitTypedefType` like in the very first fix of @vabridgers because it affects how every typedef is imported, and that can be incorrect. Probably some special handling of a record called `__va_list` at AST import can be a good fix too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142822