[llvm-branch-commits] [lldb] 9586082 - [lldb] Allow LLDB to automatically retry a failed expression with an imported std C++ module
Raphael Isemann via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Thu Dec 10 03:33:40 PST 2020
Author: Raphael Isemann
Date: 2020-12-10T12:29:17+01:00
New Revision: 958608285eb4d04c6e3dc7071fa429b43597585b
URL: https://github.com/llvm/llvm-project/commit/958608285eb4d04c6e3dc7071fa429b43597585b
DIFF: https://github.com/llvm/llvm-project/commit/958608285eb4d04c6e3dc7071fa429b43597585b.diff
LOG: [lldb] Allow LLDB to automatically retry a failed expression with an imported std C++ module
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.
Reviewed By: shafik, JDevlieghere, aprantl
Differential Revision: https://reviews.llvm.org/D92784
Added:
lldb/test/API/commands/expression/import-std-module/retry-with-std-module/Makefile
lldb/test/API/commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py
lldb/test/API/commands/expression/import-std-module/retry-with-std-module/main.cpp
Modified:
lldb/include/lldb/Target/Target.h
lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
lldb/source/Target/Target.cpp
lldb/source/Target/TargetProperties.td
Removed:
################################################################################
diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
index 47568c9e4429..69baefb964b0 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -65,6 +65,12 @@ enum LoadDependentFiles {
eLoadDependentsNo,
};
+enum ImportStdModule {
+ eImportStdModuleFalse,
+ eImportStdModuleFallback,
+ eImportStdModuleTrue,
+};
+
class TargetExperimentalProperties : public Properties {
public:
TargetExperimentalProperties();
@@ -135,7 +141,7 @@ class TargetProperties : public Properties {
bool GetEnableAutoImportClangModules() const;
- bool GetEnableImportStdModule() const;
+ ImportStdModule GetImportStdModule() const;
bool GetEnableAutoApplyFixIts() const;
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
index a28b4a7fb42c..9be294750fa0 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
@@ -417,7 +417,6 @@ void ClangUserExpression::CreateSourceCode(
DiagnosticManager &diagnostic_manager, ExecutionContext &exe_ctx,
std::vector<std::string> modules_to_import, bool for_completion) {
- m_filename = m_clang_state->GetNextExprFileName();
std::string prefix = m_expr_prefix;
if (m_options.GetExecutionPolicy() == eExecutionPolicyTopLevel) {
@@ -477,9 +476,6 @@ CppModuleConfiguration GetModuleConfig(lldb::LanguageType language,
if (!target)
return LogConfigError("No target");
- if (!target->GetEnableImportStdModule())
- return LogConfigError("Importing std module not enabled in settings");
-
StackFrame *frame = exe_ctx.GetFramePtr();
if (!frame)
return LogConfigError("No frame");
@@ -529,8 +525,6 @@ CppModuleConfiguration GetModuleConfig(lldb::LanguageType language,
bool ClangUserExpression::PrepareForParsing(
DiagnosticManager &diagnostic_manager, ExecutionContext &exe_ctx,
bool for_completion) {
- Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
-
InstallContext(exe_ctx);
if (!SetupPersistentState(diagnostic_manager, exe_ctx))
@@ -551,50 +545,20 @@ bool ClangUserExpression::PrepareForParsing(
SetupDeclVendor(exe_ctx, m_target, diagnostic_manager);
- CppModuleConfiguration module_config = GetModuleConfig(m_language, exe_ctx);
- llvm::ArrayRef<std::string> imported_modules =
- module_config.GetImportedModules();
- m_imported_cpp_modules = !imported_modules.empty();
- m_include_directories = module_config.GetIncludeDirs();
+ m_filename = m_clang_state->GetNextExprFileName();
- LLDB_LOG(log, "List of imported modules in expression: {0}",
- llvm::make_range(imported_modules.begin(), imported_modules.end()));
- LLDB_LOG(log, "List of include directories gathered for modules: {0}",
- llvm::make_range(m_include_directories.begin(),
- m_include_directories.end()));
+ if (m_target->GetImportStdModule() == eImportStdModuleTrue)
+ SetupCppModuleImports(exe_ctx);
- CreateSourceCode(diagnostic_manager, exe_ctx, imported_modules,
+ CreateSourceCode(diagnostic_manager, exe_ctx, m_imported_cpp_modules,
for_completion);
return true;
}
-bool ClangUserExpression::Parse(DiagnosticManager &diagnostic_manager,
- ExecutionContext &exe_ctx,
- lldb_private::ExecutionPolicy execution_policy,
- bool keep_result_in_memory,
- bool generate_debug_info) {
- Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
-
- if (!PrepareForParsing(diagnostic_manager, exe_ctx, /*for_completion*/ false))
- return false;
-
- LLDB_LOGF(log, "Parsing the following code:\n%s", m_transformed_text.c_str());
-
- ////////////////////////////////////
- // Set up the target and compiler
- //
-
- Target *target = exe_ctx.GetTargetPtr();
-
- if (!target) {
- diagnostic_manager.PutString(eDiagnosticSeverityError, "invalid target");
- return false;
- }
-
- //////////////////////////
- // Parse the expression
- //
-
+bool ClangUserExpression::TryParse(
+ DiagnosticManager &diagnostic_manager, ExecutionContextScope *exe_scope,
+ ExecutionContext &exe_ctx, lldb_private::ExecutionPolicy execution_policy,
+ bool keep_result_in_memory, bool generate_debug_info) {
m_materializer_up = std::make_unique<Materializer>();
ResetDeclMap(exe_ctx, m_result_delegate, keep_result_in_memory);
@@ -612,26 +576,16 @@ bool ClangUserExpression::Parse(DiagnosticManager &diagnostic_manager,
DeclMap()->SetLookupsEnabled(true);
}
- Process *process = exe_ctx.GetProcessPtr();
- ExecutionContextScope *exe_scope = process;
-
- if (!exe_scope)
- exe_scope = exe_ctx.GetTargetPtr();
-
- // We use a shared pointer here so we can use the original parser - if it
- // succeeds or the rewrite parser we might make if it fails. But the
- // parser_sp will never be empty.
-
- ClangExpressionParser parser(exe_scope, *this, generate_debug_info,
- m_include_directories, m_filename);
+ m_parser = std::make_unique<ClangExpressionParser>(
+ exe_scope, *this, generate_debug_info, m_include_directories, m_filename);
- unsigned num_errors = parser.Parse(diagnostic_manager);
+ unsigned num_errors = m_parser->Parse(diagnostic_manager);
// Check here for FixItHints. If there are any try to apply the fixits and
// set the fixed text in m_fixed_text before returning an error.
if (num_errors) {
if (diagnostic_manager.HasFixIts()) {
- if (parser.RewriteExpression(diagnostic_manager)) {
+ if (m_parser->RewriteExpression(diagnostic_manager)) {
size_t fixed_start;
size_t fixed_end;
m_fixed_text = diagnostic_manager.GetFixedExpression();
@@ -652,7 +606,7 @@ bool ClangUserExpression::Parse(DiagnosticManager &diagnostic_manager,
//
{
- Status jit_error = parser.PrepareForExecution(
+ Status jit_error = m_parser->PrepareForExecution(
m_jit_start_addr, m_jit_end_addr, m_execution_unit_sp, exe_ctx,
m_can_interpret, execution_policy);
@@ -666,10 +620,91 @@ bool ClangUserExpression::Parse(DiagnosticManager &diagnostic_manager,
return false;
}
}
+ return true;
+}
+
+void ClangUserExpression::SetupCppModuleImports(ExecutionContext &exe_ctx) {
+ Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
+
+ CppModuleConfiguration module_config = GetModuleConfig(m_language, exe_ctx);
+ m_imported_cpp_modules = module_config.GetImportedModules();
+ m_include_directories = module_config.GetIncludeDirs();
+
+ LLDB_LOG(log, "List of imported modules in expression: {0}",
+ llvm::make_range(m_imported_cpp_modules.begin(),
+ m_imported_cpp_modules.end()));
+ LLDB_LOG(log, "List of include directories gathered for modules: {0}",
+ llvm::make_range(m_include_directories.begin(),
+ m_include_directories.end()));
+}
+
+static bool shouldRetryWithCppModule(Target &target, ExecutionPolicy exe_policy) {
+ // Top-level expression don't yet support importing C++ modules.
+ if (exe_policy == ExecutionPolicy::eExecutionPolicyTopLevel)
+ return false;
+ return target.GetImportStdModule() == eImportStdModuleFallback;
+}
+
+bool ClangUserExpression::Parse(DiagnosticManager &diagnostic_manager,
+ ExecutionContext &exe_ctx,
+ lldb_private::ExecutionPolicy execution_policy,
+ bool keep_result_in_memory,
+ bool generate_debug_info) {
+ Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
+
+ if (!PrepareForParsing(diagnostic_manager, exe_ctx, /*for_completion*/ false))
+ return false;
+
+ LLDB_LOGF(log, "Parsing the following code:\n%s", m_transformed_text.c_str());
+
+ ////////////////////////////////////
+ // Set up the target and compiler
+ //
+
+ Target *target = exe_ctx.GetTargetPtr();
+
+ if (!target) {
+ diagnostic_manager.PutString(eDiagnosticSeverityError, "invalid target");
+ return false;
+ }
+
+ //////////////////////////
+ // Parse the expression
+ //
+
+ Process *process = exe_ctx.GetProcessPtr();
+ ExecutionContextScope *exe_scope = process;
+
+ if (!exe_scope)
+ exe_scope = exe_ctx.GetTargetPtr();
+
+ bool parse_success = TryParse(diagnostic_manager, exe_scope, exe_ctx,
+ execution_policy, keep_result_in_memory,
+ generate_debug_info);
+ // If the expression failed to parse, check if retrying parsing with a loaded
+ // C++ module is possible.
+ if (!parse_success && shouldRetryWithCppModule(*target, execution_policy)) {
+ // Load the loaded C++ modules.
+ SetupCppModuleImports(exe_ctx);
+ // If we did load any modules, then retry parsing.
+ if (!m_imported_cpp_modules.empty()) {
+ // The module imports are injected into the source code wrapper,
+ // so recreate those.
+ CreateSourceCode(diagnostic_manager, exe_ctx, m_imported_cpp_modules,
+ /*for_completion*/ false);
+ // Clear the error diagnostics from the previous parse attempt.
+ diagnostic_manager.Clear();
+ parse_success = TryParse(diagnostic_manager, exe_scope, exe_ctx,
+ execution_policy, keep_result_in_memory,
+ generate_debug_info);
+ }
+ }
+ if (!parse_success)
+ return false;
if (exe_ctx.GetProcessPtr() && execution_policy == eExecutionPolicyTopLevel) {
Status static_init_error =
- parser.RunStaticInitializers(m_execution_unit_sp, exe_ctx);
+ m_parser->RunStaticInitializers(m_execution_unit_sp, exe_ctx);
if (!static_init_error.Success()) {
const char *error_cstr = static_init_error.AsCString();
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
index f734069655ef..b628f6debf66 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
@@ -168,12 +168,23 @@ class ClangUserExpression : public LLVMUserExpression {
lldb::ExpressionVariableSP
GetResultAfterDematerialization(ExecutionContextScope *exe_scope) override;
- bool DidImportCxxModules() const { return m_imported_cpp_modules; }
+ /// Returns true iff this expression is using any imported C++ modules.
+ bool DidImportCxxModules() const { return !m_imported_cpp_modules.empty(); }
private:
/// Populate m_in_cplusplus_method and m_in_objectivec_method based on the
/// environment.
+ /// Contains the actual parsing implementation.
+ /// The parameter have the same meaning as in ClangUserExpression::Parse.
+ /// \see ClangUserExpression::Parse
+ bool TryParse(DiagnosticManager &diagnostic_manager,
+ ExecutionContextScope *exe_scope, ExecutionContext &exe_ctx,
+ lldb_private::ExecutionPolicy execution_policy, bool keep_result_in_memory,
+ bool generate_debug_info);
+
+ void SetupCppModuleImports(ExecutionContext &exe_ctx);
+
void ScanContext(ExecutionContext &exe_ctx,
lldb_private::Status &err) override;
@@ -219,6 +230,8 @@ class ClangUserExpression : public LLVMUserExpression {
ResultDelegate m_result_delegate;
ClangPersistentVariables *m_clang_state;
std::unique_ptr<ClangExpressionSourceCode> m_source_code;
+ /// The parser instance we used to parse the expression.
+ std::unique_ptr<ClangExpressionParser> m_parser;
/// File name used for the expression.
std::string m_filename;
@@ -226,8 +239,9 @@ class ClangUserExpression : public LLVMUserExpression {
/// See the comment to `UserExpression::Evaluate` for details.
ValueObject *m_ctx_obj;
- /// True iff this expression explicitly imported C++ modules.
- bool m_imported_cpp_modules = false;
+ /// A list of module names that should be imported when parsing.
+ /// \see CppModuleConfiguration::GetImportedModules
+ std::vector<std::string> m_imported_cpp_modules;
/// True if the expression parser should enforce the presence of a valid class
/// pointer in order to generate the expression as a method.
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index c6bb989c132e..bee87eb1b6c7 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -3516,6 +3516,28 @@ static constexpr OptionEnumValueElement g_x86_dis_flavor_value_types[] = {
},
};
+static constexpr OptionEnumValueElement g_import_std_module_value_types[] = {
+ {
+ eImportStdModuleFalse,
+ "false",
+ "Never import the 'std' C++ module in the expression parser.",
+ },
+ {
+ eImportStdModuleFallback,
+ "fallback",
+ "Retry evaluating expressions with an imported 'std' C++ module if they"
+ " failed to parse without the module. This allows evaluating more "
+ "complex expressions involving C++ standard library types."
+ },
+ {
+ eImportStdModuleTrue,
+ "true",
+ "Always import the 'std' C++ module. This allows evaluating more "
+ "complex expressions involving C++ standard library types. This feature"
+ " is experimental."
+ },
+};
+
static constexpr OptionEnumValueElement g_hex_immediate_style_values[] = {
{
Disassembler::eHexStyleC,
@@ -3969,10 +3991,10 @@ bool TargetProperties::GetEnableAutoImportClangModules() const {
nullptr, idx, g_target_properties[idx].default_uint_value != 0);
}
-bool TargetProperties::GetEnableImportStdModule() const {
+ImportStdModule TargetProperties::GetImportStdModule() const {
const uint32_t idx = ePropertyImportStdModule;
- return m_collection_sp->GetPropertyAtIndexAsBoolean(
- nullptr, idx, g_target_properties[idx].default_uint_value != 0);
+ return (ImportStdModule)m_collection_sp->GetPropertyAtIndexAsEnumeration(
+ nullptr, idx, g_target_properties[idx].default_uint_value);
}
bool TargetProperties::GetEnableAutoApplyFixIts() const {
diff --git a/lldb/source/Target/TargetProperties.td b/lldb/source/Target/TargetProperties.td
index 627ed087384e..2d6a4358082a 100644
--- a/lldb/source/Target/TargetProperties.td
+++ b/lldb/source/Target/TargetProperties.td
@@ -49,9 +49,11 @@ let Definition = "target" in {
def AutoImportClangModules: Property<"auto-import-clang-modules", "Boolean">,
DefaultTrue,
Desc<"Automatically load Clang modules referred to by the program.">;
- def ImportStdModule: Property<"import-std-module", "Boolean">,
- DefaultFalse,
- Desc<"Import the C++ std module to improve debugging STL containers.">;
+ def ImportStdModule: Property<"import-std-module", "Enum">,
+ DefaultEnumValue<"eImportStdModuleFalse">,
+ EnumValues<"OptionEnumValues(g_import_std_module_value_types)">,
+ Desc<"Import the 'std' C++ module to improve expression parsing involving "
+ " C++ standard library types.">;
def AutoApplyFixIts: Property<"auto-apply-fixits", "Boolean">,
DefaultTrue,
Desc<"Automatically apply fix-it hints to expressions.">;
diff --git a/lldb/test/API/commands/expression/import-std-module/retry-with-std-module/Makefile b/lldb/test/API/commands/expression/import-std-module/retry-with-std-module/Makefile
new file mode 100644
index 000000000000..f938f7428468
--- /dev/null
+++ b/lldb/test/API/commands/expression/import-std-module/retry-with-std-module/Makefile
@@ -0,0 +1,3 @@
+USE_LIBCPP := 1
+CXX_SOURCES := main.cpp
+include Makefile.rules
diff --git a/lldb/test/API/commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py b/lldb/test/API/commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py
new file mode 100644
index 000000000000..b84ab84e4b48
--- /dev/null
+++ b/lldb/test/API/commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py
@@ -0,0 +1,76 @@
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestCase(TestBase):
+
+ mydir = TestBase.compute_mydir(__file__)
+
+ @add_test_categories(["libc++"])
+ @skipIf(compiler=no_match("clang"))
+ def test(self):
+ self.build()
+
+ lldbutil.run_to_source_breakpoint(self,
+ "// Set break point at this line.",
+ lldb.SBFileSpec("main.cpp"))
+
+ # Test printing the vector before enabling any C++ module setting.
+ self.expect_expr("a", result_type="std::vector<int, std::allocator<int> >")
+
+ # Set loading the import-std-module to 'fallback' which loads the module
+ # and retries when an expression fails to parse.
+ self.runCmd("settings set target.import-std-module fallback")
+
+ # Printing the vector still works. This should return the same type
+ # as before as this shouldn't use a C++ module type (the C++ module type
+ # is hiding the second template parameter as it's equal to the default
+ # argument which the C++ module has type info for).
+ self.expect_expr("a", result_type="std::vector<int, std::allocator<int> >")
+
+ # This expression can only parse with a C++ module. LLDB should
+ # automatically fall back to import the C++ module to get this working.
+ self.expect_expr("std::max<std::size_t>(0U, a.size())", result_value="3")
+
+
+ # The 'a' and 'local' part can be parsed without loading a C++ module and will
+ # load type/runtime information. The 'std::max...' part will fail to
+ # parse without a C++ module. Make sure we reset all the relevant parts of
+ # the C++ parser so that we don't end up with for example a second
+ # definition of 'local' when retrying.
+ self.expect_expr("a; local; std::max<std::size_t>(0U, a.size())", result_value="3")
+
+
+ # Try to declare top-level declarations that require a C++ module to parse.
+ # Top-level expressions don't support importing the C++ module (yet), so
+ # this should still fail as before.
+ self.expect("expr --top-level -- int i = std::max(1, 2);", error=True,
+ substrs=["no member named 'max' in namespace 'std'"])
+
+ # Check that diagnostics from the first parse attempt don't show up
+ # in the C++ module parse attempt. In the expression below, we first
+ # fail to parse 'std::max'. Then we retry with a loaded C++ module
+ # and succeed to parse the 'std::max' part. However, the
+ # trailing 'unknown_identifier' will fail to parse even with the
+ # loaded module. The 'std::max' diagnostic from the first attempt
+ # however should not be shown to the user.
+ self.expect("expr std::max(1, 2); unknown_identifier", error=True,
+ matching=False,
+ substrs=["no member named 'max'"])
+ # The proper diagnostic however should be shown on the retry.
+ self.expect("expr std::max(1, 2); unknown_identifier", error=True,
+ substrs=["use of undeclared identifier 'unknown_identifier'"])
+
+ # Turn on the 'import-std-module' setting and make sure we import the
+ # C++ module.
+ self.runCmd("settings set target.import-std-module true")
+ # This is still expected to work.
+ self.expect_expr("std::max<std::size_t>(0U, a.size())", result_value="3")
+
+ # Turn of the 'import-std-module' setting and make sure we don't load
+ # the module (which should prevent parsing the expression involving
+ # 'std::max').
+ self.runCmd("settings set target.import-std-module false")
+ self.expect("expr std::max(1, 2);", error=True,
+ substrs=["no member named 'max' in namespace 'std'"])
diff --git a/lldb/test/API/commands/expression/import-std-module/retry-with-std-module/main.cpp b/lldb/test/API/commands/expression/import-std-module/retry-with-std-module/main.cpp
new file mode 100644
index 000000000000..b150e26d513d
--- /dev/null
+++ b/lldb/test/API/commands/expression/import-std-module/retry-with-std-module/main.cpp
@@ -0,0 +1,7 @@
+#include <vector>
+
+int main(int argc, char **argv) {
+ std::vector<int> a = {3, 1, 2};
+ int local = 3;
+ return 0; // Set break point at this line.
+}
More information about the llvm-branch-commits
mailing list