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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/Sema.cpp:460-462
+        Scope S(TUScope, Scope::DeclScope, getDiagnostics());
+        PushDeclContext(&S, DC);
+        PushOnScopeChains(ND, &S);
----------------
mizvekov wrote:
> aaron.ballman wrote:
> > erichkeane wrote:
> > > mizvekov wrote:
> > > > aaron.ballman wrote:
> > > > > Is it valid to use a local variable as a `Scope` object? I would assume that scope goes away at the end of this compound statement? I didn't spot other cases where we did this -- usually we call `Parser::EnterScope()` which allocates a new `Scope` that `Sema` then uses.
> > > > I don't see anything problematic about allocating it on the stack like this, implementation-wise.
> > > > 
> > > > I thought it would be a bit counter-intuitive to use parser functions here, since these declarations are entirely synthetic.
> > > I dont know much about "Scope", as it is particularly used in Parsing, but I DO see the comment says: "A scope is a transient data structure that is used while parsing the program.  It assists with resolving identifiers to the appropriate declaration."
> > > 
> > > Which means to me it doesn't need to out-live its parsing time.  I THINK it gets replaced with a CXXScope at one point, but I dont know when/where that happens.
> > > 
> > The situation I'm worried about (and maybe it's not a valid concern) is that `Parser::EnterScope` uses a cache of scopes that this bypasses and I'm not certain whether that's a good thing or not. I *think* it might be reasonable (`Scope` is just a holder for a `DeclContext` and that's the object which has all the declarations added to and removed from, and that context outlives the `Scope` object), but it's worth double-checking.
> Yeah I checked that. The scope object is fairly innocuous, the `EnterScope` function uses a cache to stack them, which seems useful in performance intensive workloads, and in case the object needs to outlive the function that creates it.
> 
> But here I see no problem just making it clear that this Scope is only used here, by simply allocating it on the stack.
Great, thank you for double-checking, I appreciate it!


================
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>
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136886