[Lldb-commits] [PATCH] D67803: [lldb] Fix that importing decls in a TagDecl end up in wrong declaration context (partly reverts D61333)

[Gabor Marton via Phabricator via lldb-commits]

martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.

Thanks for the thorough explanation about the different ASTContexts in LLDB.
The hack is indeed hideous, but seems good to me... for now until the real solution is born.

> So we go to D-AST to get the std::pair members. The ASTImporter is asked to copy over std::pair members
>  and first checks if std::pair is already in P-AST. However, it only finds the std::pair we got from E-AST, so it
>  can't use it's map of already imported declarations and does a comparison between the std::pair decls we have
>  Because the ASTImporter thinks they are different declarations, it creates a second std::pair

Note that LLDB uses the lenient ODR violation handling strategy (`ASTImporter::ODRHandlingType::Liberal`).
With the other, stricter ODR violation handling strategy, when the to be imported std::pair turns out to be nonequivalent with the existing one we would get an error instead of a new decl.
Would it make sense to change the odr handling strategy before the formatter turns to the ExternalASTSource?
It would not solve this particular issue, but perhaps could make discovering bugs like this easier.

> My preferred solution would be to complete all declarations in E-AST before they get moved to P-AST

That sounds reasonable to me.

> When doing expr m we do a minimal import of std::map from D-AST to E-AST just do the type checking/codegen.

There are some cases, when codegen do need the completed tagdecl, not just the skeleton we get via minimal import. So, there must be cases when in the E-AST we have a full, completed type, right? What would happen if we imported everything as a full declaration in the first place, instead of  using the minimal import? Could that work?
Perhaps it is even more intrusive then your preferred solution, but could greatly reduce the import related code and complexity in LLDB.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67803