• Made empty/nullptr check more readable.
• Removed some uses of the new comparison operator for cases where we don't have a literal to compare against (which makes it hard to estimate if this is really better).
• Reworded documentation so that the length of the string doesn't really matter getting a performance benefit, only the fact that whether ConstString we pass is temporary or not.

In D60667#1467387, @amccarth wrote:

I, too, have some concern that this could have unintended side effects. To make the temporary StringRefs from the zero-terminated strings requires a strlen call each time. So we're making two passes over a string each time (once to measure and once to compare). Granted, these are mostly very short.

I don't think I can provide a test for this because I don't really know the code so well (and generating an UNPREDICTABLE seems tricky too). I'll leave this up for a bit to see if someone comes up with a test (or I'll have time to dig into the code) and then commit.

Don't have the test for this and probably can't write one for it (as I don't have access to an ARM device for testing and don't really know the code so well). If anyone feels like providing a test, feel free to link it to this revision.

• Added more documentation.

I believe this revision introduced a warning when compiling with Clang:

 98% [4004/4047] Building CXX object tool....dir/PdbFPOProgramToDWARFExpression.cpp.o
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:161:7: warning: '(anonymous namespace)::FPOProgramASTVisitor<void>' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitor {
      ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:190:51: note: in instantiation of template class '(anonymous namespace)::FPOProgramASTVisitor<void>' requested here
class FPOProgramASTVisitorMergeDependent : public FPOProgramASTVisitor<> {
                                                  ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:190:7: warning: '(anonymous namespace)::FPOProgramASTVisitorMergeDependent' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitorMergeDependent : public FPOProgramASTVisitor<> {
      ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:161:7: warning: '(anonymous namespace)::FPOProgramASTVisitor<bool>' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitor {
      ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:232:14: note: in instantiation of template class '(anonymous namespace)::FPOProgramASTVisitor<bool>' requested here
    : public FPOProgramASTVisitor<bool> {
             ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:231:7: warning: '(anonymous namespace)::FPOProgramASTVisitorResolveRegisterRefs' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitorResolveRegisterRefs
      ^
/home/teemperor/llvm/side/llvm-project/lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:310:7: warning: '(anonymous namespace)::FPOProgramASTVisitorDWARFCodegen' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class FPOProgramASTVisitorDWARFCodegen : public FPOProgramASTVisitor<> {
      ^
5 warnings generated.

LGTM. Thanks for doing this! These lines are really frustrating to maintain when writing documentation. Also this should bring us yet again a bit closer to the LLVM style, so +1 for that.

Thanks for the review!

Ah, I get it now. Fixed!

• Addressed (most of) Aleksei's comments.

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?

• Removed unused functions.
• Fixed two comments that I missed to update.

• Removed dump() that was left in there from some debugging.
• Changed the second Import call I missed to Import_New
• No longer ignoring the Errors in Expected when importing template arguments.

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?

• Addressed feedback.

• Added more tests (contents from debug info for containers, vectors of vectors)
• Import -> Import_New
• Rebased on top of the updated parent commit, which means the StdModuleHandler is now much simpler.

So I replaced the whole Strategy/Shim thing with a simple ImportInternal method that we can overwrite and is also no longer public. That also gets rid of all the boilerplate code from the first patches and also makes the follow-up LLDB patch nicer.

I think we could also refactor the strategy into a PreImport call. That way we don't break all the user-code out there and it seems less intrusive to the ASTImporter code. I'll update the patch.

Thanks for the quick review! I changed the code to also update the internal ASTImporter state like with a normal decl.

Wait, do I understand correctly that this is a commit from your personal github repo, but somehow phab finds it and thinks it is in the official LLVM repo?

• Changed to llvm::cast (even though the assert message is a bit generic now).

• Rebased patch.
• Modernized for loops in ASTUtils.

• Added safety check that we don't delete directories or files outside the llvm staging dir.

• Addressed Adrian's comments.

• Addressed Pavel's comments.

• Addressed Pavel's comments (thanks!).
• Reworked test setup code (thanks Adrian!)
• Rebased the patch.

• Refactored build flags.
• Fixed doxygen comment.

• Remove libc++ dependency from sysroot test.
• Small format fix for sysroot Makefile

• Added sysroot test.
• Moved build flags into MANDATORY_CXXMODULE_BUILD_FLAGS make variable.

LGTM, thanks!

LGTM.

I think the idea of the patch is right. Not sure tough if having Import take a second parameter is consistent with the other Import functions (and if that even matters).

That should be the right repo: https://github.com/apple/swift-lldb

@labath Thanks for the hint with the sysroot, but I'm not really sure what's the best way to test this. Making a whole fake sysroot that can be used to compile a std module seems overkill, and not sure if symlinking or so is a good approach either. I'm open to suggestions.