Ping @a_sidorin @shafik

• Put back the call to GetOriginalDecl

Ping @shafik @a_sidorin

In D59826#1451536, @clayborg wrote:

Any way to dump the AST in a test to verify we don't create multiple?

Ping

In D59485#1446950, @a_sidorin wrote:

Hello Raphael,
I think we should accept this change. I don't see an easy way to merge the LLDB stuff into ASTImporter; also, this patch provides a good extension point for ASTImporter since it is designed to be a parent class. @martong @shafik Gabor, Shafik, what do you think?

In D59665#1446764, @a_sidorin wrote:

Hi Shafik,
Thank you for the explanation, it is much more clear to me now. But, as I see, D59692 is going to discard the changes this patch introduces. @martong Gabor, do you expect the changes of this patch to be merged into yours, or should this patch be abandoned?

In D59485#1444673, @teemperor wrote:

In D59485#1439628, @martong wrote:

In D59485#1439570, @martong wrote:

In 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 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).

LGTM!

LGTM!

There is a lookup failure within the
ASTImporter during the expression evaluation. The clang::ASTImporter cannot
lookup previously created declarations thus it creates duplicated and redundant
declarations. These duplicated declarations then later can cause different
assertions.

• Add braces around else
• Remove falsely duplicated push_back
• Use Expected<T> in HandleNameConflict

Looks good for me now, but we should make a similar change in VisitRecordDecl too.

In D59665#1442328, @shafik wrote:

@martong your idea does not work b/c default construction DeclarationName() treats it the same as being empty. So if (!Name) is still true.

In D59485#1439570, @martong wrote:

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

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

I am aware of similar errors like this with other AST nodes. We had a patch in our fork to fix that in January (https://github.com/Ericsson/clang/pull/569/files) I am going to prepare a patch from that cause I see now this affects you guys in LLDB too.

@shafik, @a_sidorin Ping. Could you please take a look?
Guys, this and its child patch are very important patches, because without it the error handling of ASTImporter is not completed. This means we may encounter a nullptr DeclContext in the middle of the import process, etc...

Ping

@martong It's not related to lookup or anything like that, it's just that we don't have enough information in our debug info AST to allow people to use meta programming. The custom logic we have in LLDB looks into the std C++ module to fill out these gaps in the AST by hand when they are used in an expression.

The remark about the alternative being slow just means that fixing all the templates we have in our debug info AST seems like a costly approach. I'm also not sure how well it would work, as I never tried mixing a C++ module in an ASTContext that isn't currently being parsed by Clang.

In D59665#1438911, @a_sidorin wrote:

Hi Shafik,

Honestly, I was always wondering what does HandleNameConflict actually do. Its implementation in ASTImporter is trivial and I don't see any of its overrides in LLDB code too. Why do we check its result to be non-empty is a question to me as well. And the more I look at it (and the bug you pointed in it), the more I think there is something wrong with it. Maybe it is better to just remove it at all? I hope LLDB developers have more knowledge about it. Shafik, Davide @davide , do you know its actual purpose?

Hi Shafik,

LGTM!

• Rebase to master

We can't reliable import templates with the ASTImporter, so actually reconstructing the template in the debug info AST and then importing it doesn't work.

Could you please elaborate how the import of templates fails in ASTImporter? Is it because the AST you build from the DWARF is incomplete? Or is there a lookup problem? Is there an assertion in one of the CodeGen functions once the import is finished?
If we could fix that error in the ASTImporter then other clients of it (like CTU) could benefit from the fix too. Perhaps, the best would be to have a test which reproduces the template import related errors, maybe a lit test with clang-import-test?

@shafik Ping

@shafik Ping.

Ping

Ping

Ping

@a_sidorin Aleksei, If you don't mind, I am going to investigate this further with macOS/LLDB and try to answer Adrian's comments too.

Rebase to master.
There was a conflict in the tests, in ASTImporterTest.cpp.

This patch is the next in the series of patches to finish the proper error handling (i.e. using Error and Expected<T>). Please see the "Stack" column above.

@balazske Or is it superseded by this one? https://reviews.llvm.org/D53818

Ping. This patch is the next in the series of patches to finish the proper error handling (i.e. using Error and Expected<T>). Please see the "Stack" column above.

• Add asserts

Alexei, thanks for the review!

• Fix some comments

• Add FindAndMapDefinition as member fun
• Refactor CheckPreviousDecl

Alexei, thanks for the review!