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

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.


================
Comment at: clang/test/AST/ast-dump-overloaded-operators.cpp:27
 // CHECK-NEXT: | `-ParmVarDecl {{.*}} <col:18> col:19{{( imported)?}} 'E'
-// CHECK-NEXT: `-FunctionDecl {{.*}} <line:14:1, line:18:1> line:14:6{{( imported)?}} test 'void ()'
+// CHECK-NEXT:  -FunctionDecl {{.*}} <line:14:1, line:18:1> line:14:6{{( imported)?}} test 'void ()'
 // CHECK-NEXT:   `-CompoundStmt {{.*}} <col:13, line:18:1>
----------------
aaron.ballman wrote:
> mizvekov wrote:
> > aaron.ballman wrote:
> > > mizvekov wrote:
> > > > aaron.ballman wrote:
> > > > > mizvekov wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > This looks like a benign typo -- we still match the line because FileCheck will match partial lines, but I'm pretty sure nothing in your patch would have necessitated this change. Then again, you make this change in a lot of tests, so maybe I'm wrong -- in which case, what changed?
> > > > > > What is happening here (and in all the other such instances) is that on the `import` case, this declaration stops being the last one on the TU. So the beginning of the line would match on `|-` instead of ``-`, but the non-import case this remains the last one.
> > > > > > 
> > > > > > So I simply relaxed the match.
> > > > > Hmmm, I think it'd help to show what new lines are now showing up so we can validate that they make sense in context. WDYT?
> > > > It's the new lines from the synthesized `va_list_tag`. I think they would be noise on this these tests, they are testing something completely unrelated.
> > > > 
> > > > But they do show up on the ast-json tests where we are basically dumping the whole TU.
> > > Oh, so we're adding those to the *end* of the translation unit, not at the beginning? 
> > We are adding them at the beginning, but it seems we import the other stuff before adding them, so on the import tests they show up on the end.
> > 
> > The thing here is that we end up importing them, but then adding new ones from the current Sema anyway. But this is fine as most other import things, they get separate declarations but merged in any case so they have the same canonical decl.
> > 
> > The way we avoid doing that for the other synthesized builtins around it, is that we just skip creating them if we find their `Identifier` has been created somehow, which is a fairly strange way to test this.
> > 
> > This seemed even less appropriate for `va_list_tag`, since some ABIs put it into `std` namespace.
> > 
> > And it seems to me that avoiding creating them again for the current Sema is a fairly minimal optimization, and it could make we miss diagnosing imports where those things are inconsistent between contexts.
> > 
> > WDYT?
> > We are adding them at the beginning, but it seems we import the other stuff before adding them, so on the import tests they show up on the end.
> 
> That really surprises me -- I thought we would export the AST in order and import it in order. I'm a bit worried that out-of-order AST nodes may break invariants.
> 
> > This seemed even less appropriate for va_list_tag, since some ABIs put it into std namespace.
> 
> Yeah, identifying things by name in the frontend is rarely a trivial exercise, we try to avoid it whenever possible.
> 
> > And it seems to me that avoiding creating them again for the current Sema is a fairly minimal optimization, and it could make we miss diagnosing imports where those things are inconsistent between contexts.
> 
> Yeah, it doesn't strike me as a significant optimization to be worth worrying about. My concern is more that we expect import order to match export order and we're not matching up.
We are importing in the right order.

The problem is that we import the pch before we create the builtins here.

Ie look at what the test is doing, it first creates a TU, then imports it into an empty TU `/dev/null`.
In that last case, we end up adding the declaration for this Sema's `va_list_tag` after the imported stuff, but the one from the original context is still imported in the right order.

It doesn't seem problematic to me, but would you rather we changed the import order around wrt the creation of the builtins?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136886