This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Allow LLDB to automatically retry a failed expression with an import std C++ module
ClosedPublic

Authored by teemperor on Dec 7 2020, 12:50 PM.

Details

Summary

By now LLDB can import the 'std' C++ module to improve expression evaluation, but there are still a
few problems to solve before we can do this by default. One is that importing the C++ module is slightly slower
than normal expression evaluation (mostly because the disk access and loading the initial lookup data is quite
slow in comparison to the barebone Clang setup the rest of the LLDB expression evaluator is usually doing).
Another problem is that some complicated types in the standard library aren't fully supported yet by the ASTImporter,
so we end up types that fail to import (which usually appears to the user as if the type is empty or there is just
no result variable).

To still allow people to adopt this mode in their daily debugging, this patch adds a setting that allows LLDB to
automatically retry failed expression with a loaded C++ module. All success expressions will behave exactly as
they would do before this patch. Failed expressions get a another parse attempt if we find a usable C++ module
in the current execution context. This way we shouldn't have any performance/parsing regressions in normal debugging
workflows, while the debugging workflows involving STL containers benefit from the C++ module type info.

This setting is off by default for now with the intention to enable it by default on macOS soon-ish.

The implementation is mostly just extracting the existing parse logic into its own function and then calling the parse
function again if the first evaluation failed and we have a C++ module to retry the parsing with.

Diff Detail

Event Timeline

teemperor requested review of this revision.Dec 7 2020, 12:50 PM
teemperor created this revision.

Would it make sense to convert import-std-module to an enum with values True, False and Fallback instead of introducing a new option and then changing the default over time?

aprantl accepted this revision.Dec 7 2020, 2:20 PM
aprantl added a subscriber: aprantl.

Nice!

lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
558–561

In swift-lldb we also have a really ad-hoc retry mechanism: https://github.com/apple/llvm-project/blob/2919e1a2e5019874375b880afd473d2744b5cada/lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp#L1465

I wonder if we could/should make retrying an official part of the interface?

lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
180

nit: \see

243

\see

lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.h
77 ↗(On Diff #309997)

Is that because it isn't safe to return the reference?

This revision is now accepted and ready to land.Dec 7 2020, 2:20 PM
shafik added a comment.Dec 8 2020, 9:54 AM

LGTM but I have one questions and I am also curious about Adrian's question about returning by reference.

lldb/test/API/commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py
66

Should we try setting it to false and make sure it fails like we expect?

teemperor marked 2 inline comments as done.Dec 8 2020, 2:46 PM

Would it make sense to convert import-std-module to an enum with values True, False and Fallback instead of introducing a new option and then changing the default over time?

I actually think that makes much more sense than the current version. I changed it (but I kept the string values lowercase to be consistent with the other settings). Thanks!

lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
558–561

I can look into that, but if we unify this code we can effectively only share the if (should_retry) line. And then the interface would need to be able to express "retry with C++ modules" in a language-independent way (bool retry_harder?).

When I was implementing the retrying-fixits-multiple-times I already thought that we probably should go the other way and make the UserExpression interface much smaller. The generic UserExpression stuff knows a lot about the implementation that it really shouldn't (like, that there are Fix-Its and that they might need to be applied multiple times, etc.).

lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.h
77 ↗(On Diff #309997)

Yes, but actually that was just a WIP change that slipped into the git commit. I changed that to a vector before I realized that ArrayRef defines operator std::vector<T> to do this automatically for us. I reverted that change.

lldb/test/API/commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py
66

Makes sense, done!

teemperor updated this revision to Diff 310354.Dec 8 2020, 2:48 PM
  • Now using one setting with a backing enum (Thanks Jonas)
  • Gave up on imposing my doxygen code style on the project (Thanks Adrian).
  • Expanded test (Thanks Shafik)
JDevlieghere accepted this revision.Dec 8 2020, 4:50 PM

LGTM

lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
660

Does this add anything? Would this be a good opportunity to get rid of these weird comments/banners?

teemperor added inline comments.Dec 9 2020, 9:40 AM
lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
660

Well they're kinda useful if our functions are a 1000 lines long :) (but yes, they should be replaced by normal functions)

shafik accepted this revision.Dec 9 2020, 2:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2020, 3:29 AM