This is an archive of the discontinued LLVM Phabricator instance.

Instantiate 'std' templates explicitly in the expression evaluator
ClosedPublic

Authored by teemperor on Mar 19 2019, 2:35 AM.

Details

Summary

This patch is a follow-up for D58125. It implements the manual instantiation and merging of 'std' templates like
std::vector and std::shared_ptr with information from the debug info AST. This (finally) allows using these classes
in the expression evaluator like every other class (i.e. things like vec.size() and shared_ptr debugging now works, yay!).

The main logic is the CxxModuleHandler which intercept the ASTImporter import process and replaces any std decls
by decls from the C++ module. The decls from the C++ module are "imported" by just deserializing them directly in
the expression evaluation context. This is mostly because we don't want to rely on the ASTImporter to correctly import
these declarations, but in the future we should also move to the ASTImporter for that.

This patch doesn't contain the automatic desugaring for result variables. This means that if you call for example
size of std::vector you maybe get some very verbose typedef'd type as the variable type, e.g.
std::vector<int, std::allocator<int>>::value_type.

This is not only unreadable, it also means that our ASTImporter has to import all these types and associated
decls into the persisent variable context. This currently usually leads to some assertion getting triggered
in Clang when the ASTImporter either makes a mistake during importing or our debug info AST is inconsitent.
The current workaround I use in the tests is to just cast the result to it's actual type (e.g. size_t or int) to prevent
the ASTImporter from having to handle all these complicated decls.

The automatic desugaring will be a future patch because I'm not happy yet with the current code for that and because
I anticipate that this will be a controversial patch.

Diff Detail

Repository
rL LLVM

Event Timeline

teemperor created this revision.Mar 19 2019, 2:35 AM
balazske added inline comments.
lldb/source/Symbol/StdModuleHandler.cpp
220 ↗(On Diff #191262)

For long-term (but it should be not so long) the ASTImporter::Import_New should be used (later the Import_New will become the Import). By using the existing Import_New the code will be prepared for this change. For this to work the error handling (ignore error, return it from Import or something else) must be implemented here.

221 ↗(On Diff #191262)

I do not know if it is possible that the type to be imported here can have back-references to the template object that is currently imported (for example a vector of vectors?). If yes this case may not work correctly.

teemperor marked 2 inline comments as done.Mar 21 2019, 1:34 PM
teemperor added inline comments.
lldb/source/Symbol/StdModuleHandler.cpp
220 ↗(On Diff #191262)

Thanks, fixed! I currently just return nothing, which will bring us back to LLDB's normal importing logic (which then maybe does better).

221 ↗(On Diff #191262)

We first import the argument and then do a lookup, so that works fine. I added a test for this now. Thanks a lot for heads up!

teemperor updated this revision to Diff 191770.Mar 21 2019, 1:36 PM
  • 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.
balazske added inline comments.Mar 22 2019, 2:45 AM
lldb/source/Symbol/StdModuleHandler.cpp
220 ↗(On Diff #191262)

The TemplateArgument::Integral case is not fixed yet.

212 ↗(On Diff #191770)

This does not work correctly: llvm::consumeError(type.takeError()) should be called to handle the error, otherwise it will trigger assertion.

balazske added inline comments.Mar 22 2019, 4:59 AM
lldb/source/Symbol/StdModuleHandler.cpp
245 ↗(On Diff #191770)

This dump is not needed?

teemperor updated this revision to Diff 191876.Mar 22 2019, 6:59 AM
  • 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.
teemperor marked 5 inline comments as done.Mar 22 2019, 6:59 AM
martong added inline comments.Mar 22 2019, 8:38 AM
lldb/include/lldb/Symbol/StdModuleHandler.h
22 ↗(On Diff #191876)

I think it would worth to extend this description with something like this:

tryInstantiateStdTemplate directly *creates* (or finds existing) specializations from the std module and intercept the import from the debug info AST to avoid having broken specializations in the expression evaluation context.

martong added inline comments.Mar 25 2019, 6:47 AM
lldb/source/Symbol/StdModuleHandler.cpp
242 ↗(On Diff #191876)

Is there any guarantee that the before any subsequent clang::ASTImporter::Import call this newly created node is registered as imported (via ASTImporter::MapImported)?

The reason why I am asking this is because we have to enforce in clang::ASTImporter that after every Decl is created then immediately it is registered as an imported Decl, see ASTImporter::GetImportedOrCreateDecl. This we we make sure that no subsequent import calls will create the same node again.
The cycles in the AST are handled properly only this way.

This is one reason which makes me worried about exposing the import mechanism via ImportInternal. Would it be working if GetImportedOrCreateDecl was virtual and overridden here?

martong added inline comments.Mar 25 2019, 6:50 AM
lldb/source/Symbol/StdModuleHandler.cpp
242 ↗(On Diff #191876)

Would it be working if GetImportedOrCreateDecl was virtual and overridden here?

Sorry, that would certainly not work, but perhaps a virtual hook inside that function could work...

aprantl added inline comments.Apr 9 2019, 9:32 AM
lldb/include/lldb/Symbol/ClangASTImporter.h
240 ↗(On Diff #191876)

Bonus points for coming up with a more descriptive name for this class!

247 ↗(On Diff #191876)

Can you add a /// one-liner for this class?

lldb/packages/Python/lldbsuite/test/expression_command/import-std-module/forward_list-basic/TestBasicForwardList.py
17 ↗(On Diff #191876)

Why wouldn't this work on darwin and/or with a dsym or gmodules or dwo?

teemperor marked 2 inline comments as done.Apr 9 2019, 9:41 AM

Thanks for the review!

lldb/include/lldb/Symbol/ClangASTImporter.h
240 ↗(On Diff #191876)

Yeah, refactoring this code here is very high on my TODO list :)

lldb/packages/Python/lldbsuite/test/expression_command/import-std-module/forward_list-basic/TestBasicForwardList.py
17 ↗(On Diff #191876)

That's only because I didn't get our basic modules test case working on Darwin so far. There were some issues with include paths there I was working on, but I didn't get everything fixed yet. So that's why all tests relying on modules are currently marked as 'unsupported on dwarf'. I'll add Darwin everywhere as soon as I get he basic test case working (I don't expect any more Darwin specific issues as it's also just libc++ on both Linux and Darwin, but I was wrong about that before, so... :) ).

Same with dwo which for some reason failed for the basic test case (at least on Linux). Didn't really look into possible fixes for that yet.

Can you send me a mail with instructions to reproduce the failure? I'd like to take a look.

shafik added inline comments.Apr 9 2019, 1:25 PM
lldb/include/lldb/Symbol/ClangASTImporter.h
247 ↗(On Diff #191876)

This definitely needs a description, it took me a bit to understand what its purpose was and it feels a little subtle. Something along the lines of "RAII class to ensure that the StdModuleHandler is properly set and unset...."

teemperor updated this revision to Diff 194895.Apr 12 2019, 8:29 AM
  • Added more documentation.
teemperor marked an inline comment as done.Apr 25 2019, 2:58 AM
teemperor added inline comments.
lldb/source/Symbol/StdModuleHandler.cpp
242 ↗(On Diff #191876)

Sorry for the late reply.

So I was looking into changing the code to your suggestion, but I don't think we can get this working reliably. The problem is that if we intercept the importing in GetImportedOrCreateDecl then we need to intercept not just std::vector but also all related imports that the ASTImporter may make before importing std::vector itself, e.g. DeclContext, parent classes, return type, template args, etc. Also we constantly would need to update this list in case the standard library or the ASTImporter change internally to make sure we intercept everything.

Maybe we could instead implement an mechanism in the LLDB code that ensures we do call MapImported and add the decl to the lookup directly after creating a decl? E.g. by implementing something similar to ASTImporter's GetImportedOrCreateDecl?

martong added inline comments.Apr 26 2019, 3:18 AM
lldb/source/Symbol/StdModuleHandler.cpp
242 ↗(On Diff #191876)

Yes, I think that could work.
So, we could have a new function:

void CreateDeclPostamble(Decl *FromD, Decl* ToD) {  // better naming?
      // Keep track of imported Decls.
      Importer.MapImported(FromD, ToD);
      Importer.AddToLookupTable(ToD);
      InitializeImportedDecl(FromD, ToD);
}

And we'd call this from GetImportedOrCreateDecl and here after ::Create.

martong added inline comments.Apr 26 2019, 3:23 AM
lldb/source/Symbol/StdModuleHandler.cpp
242 ↗(On Diff #191876)

Well, the thing is a bit more complex because this is in ASTNodeImporter, so we need to wire things up to ASTImporter. But that seems doable.

teemperor updated this revision to Diff 196962.Apr 27 2019, 3:38 AM
teemperor edited the summary of this revision. (Show Details)
  • Renamed to CxxModuleHandler as I want to reuse most of code later for decl importing from generic modules.
  • Moved declaration construction into it's own templated method similar to the way the ASTImporter is handling decl construction.
  • Make sure we register all created decls with the ASTImporter to prevent triggering the new assert in ASTImporter::Import_New(Decl *d).
teemperor marked 2 inline comments as done.Apr 27 2019, 3:43 AM

@martong See D59485 for the new ASTImporter method for registering imported decls. I created the method in the ASTImporter as the only dependency on the ASTNodeImporter is InitializeImportedDecl which only imports more decl attributes/flags and doesn't update any internal ASTImporter state from what I can tell.

martong accepted this revision.Apr 29 2019, 4:59 AM

@martong See D59485 for the new ASTImporter method for registering imported decls. I created the method in the ASTImporter as the only dependency on the ASTNodeImporter is InitializeImportedDecl which only imports more decl attributes/flags and doesn't update any internal ASTImporter state from what I can tell.

Raphael, thank you for working on this. Looks good to me now.

This revision is now accepted and ready to land.Apr 29 2019, 4:59 AM
aprantl added inline comments.Apr 29 2019, 12:39 PM
lldb/include/lldb/Symbol/ClangASTContext.h
1005 ↗(On Diff #196962)

///

lldb/include/lldb/Symbol/ClangASTImporter.h
249 ↗(On Diff #196962)

I really wish we could rename Minion with something actually descriptive...

  • Fixed m_sema documentation.
teemperor marked 2 inline comments as done.Apr 29 2019, 1:12 PM
teemperor added inline comments.
lldb/include/lldb/Symbol/ClangASTImporter.h
249 ↗(On Diff #196962)

I'm still trying to come up with a good name. The whole purpose of the class is to actually extend and listen to the actual ASTImporter. So what about ASTImporterDecorator?

aprantl added inline comments.Apr 29 2019, 1:34 PM
lldb/include/lldb/Symbol/ClangASTImporter.h
249 ↗(On Diff #196962)
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2019, 1:41 AM
teemperor marked an inline comment as done.Apr 30 2019, 1:43 AM
teemperor added inline comments.
lldb/include/lldb/Symbol/ClangASTImporter.h
249 ↗(On Diff #196962)

Sounds good to me, even though we technically inherit from ASTImporter. But everything is better than Minion :)

balazske added inline comments.May 6 2019, 5:03 AM
lldb/include/lldb/Symbol/ClangASTImporter.h
249 ↗(On Diff #196962)

Yes Minion is not a good name. But Delegate in general is not much better (it may be called ASTImporterSubclass in this manner), it does not tell anything about what is does.