[PATCH] D59485: [ASTImporter] Add an ImportInternal method to allow customizing Import behavior.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D59485#1444673 <https://reviews.llvm.org/D59485#1444673>, @teemperor wrote:

> In D59485#1439628 <https://reviews.llvm.org/D59485#1439628>, @martong wrote:
>
> > In D59485#1439570 <https://reviews.llvm.org/D59485#1439570>, @martong wrote:
> >
> > > In D59485#1439390 <https://reviews.llvm.org/D59485#1439390>, @teemperor wrote:
> > >
> > > > > Well, I still don't understand how LLDB synthesis the AST. 
> > > > >  So you have a C++ module in a .pcm file. This means you could create an AST from that with ASTReader from it's .clang_ast section, right? In that case the AST should be complete with all type information. If this was the case then I don't see why it is not possible to use clang::ASTImporter to import templates and specializations, since we do exactly that in CTU.
> > > > > 
> > > > > Or do you guys create the AST from the debug info, from the .debug_info section of .pcm module file? And this is why AST is incomplete? I've got this from https://youtu.be/EgkZ8PTNSHQ?list=PL85Cf-ok7HpppFWlp2wX_-A1dkyFVlaEB&t=1565
> > > > >  If this is the case, then comes my naiive quiestion: Why don't you use the ASTReader?
> > > >
> > > > There are no C++ modules involved in our use case beside the generic `std` module. No user-written code is read from a module and we don't have any PCM file beside the `std` module we build for the expression evaluator.
> > > >
> > > > We use the ASTReader to directly read the `std` module contents into the expression evaluation ASTContext, but this doesn't give us the decls for the instantiations the user has made (e.g. `std::vector<Foo>`). We only have these user instantiations in the 'normal' debug info where we have a rather minimal AST (again, no modules are not involved in building this debug info AST). The `InternalImport` in the LLDB patch just stitches the module AST and the debug info AST together when we import something that we also have (in better quality from the C++ module) in the target ASTContext.
> > >
> > >
> > > Thank you for the explanation.
> > >  Now I understand you directly create specializations from the std module and intercept the import to avoid importing broken specializations from the debug info, this makes sense.
> >
> >
> > Really, just one last question to see the whole picture: If clang::ASTImporter were capable of handling a specialization/instantiation with missing info then we could use that. So the reason for this interception is that clang::ASTImporter::VisitClassTemplateSpecializationDecl cannot handle the specialization it receives because that or its dependent nodes has too many missing data, right?
>
>
> Feel free to ask! I'm just traveling so my internet access (and my reply rate) is a bit low at the moment.
>
> Not sure if we can easily merge the logic of D59537 <https://reviews.llvm.org/D59537> into the ASTImporter. It has no logic at all to actually determine if a source AST node has missing data but solely relies on our knowledge that our AST in LLDB isn't very useful for std templates. Also instantiating a template instead of importing an existing instantiation seems like a very rare scenario. I'm not even sure how we would test having a broken AST with Clang (the parser won't produce one, so we would have to hack our own AST builder for broken nodes).
>
> If there is a reasonable way to get this logic into the ASTImporter I have no objection against doing so. The only problem I see is that I can't come up with a reasonable way of merging/testing the logic in the ASTImporter (and what other application would benefit from it).


Raphael, thank you for your answer. This explanation makes so much more easier for me to understand the need for this patch.


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

https://reviews.llvm.org/D59485