[Lldb-commits] [lldb] 203a8ad - [lldb] Add option to retry Fix-Its multiple times to failed expressions
Raphael Isemann via lldb-commits
lldb-commits at lists.llvm.org
Mon Apr 6 02:26:01 PDT 2020
Author: Raphael Isemann
Date: 2020-04-06T11:25:36+02:00
New Revision: 203a8adb65429ec6b8989afdf5956564972b9703
URL: https://github.com/llvm/llvm-project/commit/203a8adb65429ec6b8989afdf5956564972b9703
DIFF: https://github.com/llvm/llvm-project/commit/203a8adb65429ec6b8989afdf5956564972b9703.diff
LOG: [lldb] Add option to retry Fix-Its multiple times to failed expressions
Summary:
Usually when Clang emits an error Fix-It it does two things. It emits the diagnostic and then it fixes the
currently generated AST to reflect the applied Fix-It. While emitting the diagnostic is easy to implement,
fixing the currently generated AST is often tricky. That causes that some Fix-Its just keep the AST as-is or
abort the parsing process entirely. Once the parser stopped, any Fix-Its for the rest of the expression are
not detected and when the user manually applies the Fix-It, the next expression will just produce a new
Fix-It.
This is often occurring with quickly made Fix-Its that are just used to bridge temporary API changes
and that often are not worth implementing a proper API fixup in addition to the diagnostic. To still
give some kind of reasonable user-experience for users that have these Fix-Its and rely on them to
fix their expressions, this patch adds the ability to retry parsing with applied Fix-Its multiple time to
give the normal Fix-It experience where things Clang knows how to fix are not causing actual expression
error (at least when automatically applying Fix-Its is activated).
The way this is implemented is just by having another setting in the expression options that specify how
often we should try applying Fix-Its and then reparse the expression. The default setting is still 1 for everyone
so this should not affect the speed in which we fail to parse expressions.
Reviewers: jingham, JDevlieghere, friss, shafik
Reviewed By: shafik
Subscribers: shafik, abidh
Differential Revision: https://reviews.llvm.org/D77214
Added:
Modified:
lldb/bindings/interface/SBExpressionOptions.i
lldb/include/lldb/API/SBExpressionOptions.h
lldb/include/lldb/Target/Target.h
lldb/source/API/SBExpressionOptions.cpp
lldb/source/Commands/CommandObjectExpression.cpp
lldb/source/Expression/UserExpression.cpp
lldb/source/Target/Target.cpp
lldb/source/Target/TargetProperties.td
lldb/test/API/commands/expression/fixits/TestFixIts.py
Removed:
################################################################################
diff --git a/lldb/bindings/interface/SBExpressionOptions.i b/lldb/bindings/interface/SBExpressionOptions.i
index 5dbd7007c01c..33a6ed745a8d 100644
--- a/lldb/bindings/interface/SBExpressionOptions.i
+++ b/lldb/bindings/interface/SBExpressionOptions.i
@@ -126,6 +126,14 @@ public:
bool
GetAutoApplyFixIts();
+ %feature("docstring", "Sets how often LLDB should retry applying fix-its to an expression.") SetRetriesWithFixIts;
+ void
+ SetRetriesWithFixIts(uint64_t retries);
+
+ %feature("docstring", "Gets how often LLDB will retry applying fix-its to an expression.") GetRetriesWithFixIts;
+ uint64_t
+ GetRetriesWithFixIts();
+
bool
GetTopLevel();
diff --git a/lldb/include/lldb/API/SBExpressionOptions.h b/lldb/include/lldb/API/SBExpressionOptions.h
index 14b52684ddce..9fc6e9ea957e 100644
--- a/lldb/include/lldb/API/SBExpressionOptions.h
+++ b/lldb/include/lldb/API/SBExpressionOptions.h
@@ -86,6 +86,10 @@ class LLDB_API SBExpressionOptions {
bool GetAutoApplyFixIts();
+ void SetRetriesWithFixIts(uint64_t retries);
+
+ uint64_t GetRetriesWithFixIts();
+
bool GetTopLevel();
void SetTopLevel(bool b = true);
diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
index f0a57b8f3827..27997e5ea76d 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -135,6 +135,8 @@ class TargetProperties : public Properties {
bool GetEnableAutoApplyFixIts() const;
+ uint64_t GetNumberOfRetriesWithFixits() const;
+
bool GetEnableNotifyAboutFixIts() const;
bool GetEnableSaveObjects() const;
@@ -385,6 +387,12 @@ class EvaluateExpressionOptions {
bool GetAutoApplyFixIts() const { return m_auto_apply_fixits; }
+ void SetRetriesWithFixIts(uint64_t number_of_retries) {
+ m_retries_with_fixits = number_of_retries;
+ }
+
+ uint64_t GetRetriesWithFixIts() const { return m_retries_with_fixits; }
+
bool IsForUtilityExpr() const { return m_running_utility_expression; }
void SetIsForUtilityExpr(bool b) { m_running_utility_expression = b; }
@@ -406,6 +414,7 @@ class EvaluateExpressionOptions {
bool m_ansi_color_errors = false;
bool m_result_is_internal = false;
bool m_auto_apply_fixits = true;
+ uint64_t m_retries_with_fixits = 1;
/// True if the executed code should be treated as utility code that is only
/// used by LLDB internally.
bool m_running_utility_expression = false;
diff --git a/lldb/source/API/SBExpressionOptions.cpp b/lldb/source/API/SBExpressionOptions.cpp
index 2c92d090a40f..bbe7cba7012c 100644
--- a/lldb/source/API/SBExpressionOptions.cpp
+++ b/lldb/source/API/SBExpressionOptions.cpp
@@ -237,6 +237,20 @@ void SBExpressionOptions::SetAutoApplyFixIts(bool b) {
return m_opaque_up->SetAutoApplyFixIts(b);
}
+uint64_t SBExpressionOptions::GetRetriesWithFixIts() {
+ LLDB_RECORD_METHOD_NO_ARGS(uint64_t, SBExpressionOptions,
+ GetRetriesWithFixIts);
+
+ return m_opaque_up->GetRetriesWithFixIts();
+}
+
+void SBExpressionOptions::SetRetriesWithFixIts(uint64_t retries) {
+ LLDB_RECORD_METHOD(void, SBExpressionOptions, SetRetriesWithFixIts,
+ (uint64_t), retries);
+
+ return m_opaque_up->SetRetriesWithFixIts(retries);
+}
+
bool SBExpressionOptions::GetTopLevel() {
LLDB_RECORD_METHOD_NO_ARGS(bool, SBExpressionOptions, GetTopLevel);
diff --git a/lldb/source/Commands/CommandObjectExpression.cpp b/lldb/source/Commands/CommandObjectExpression.cpp
index 9a314c251475..7cec714a0e17 100644
--- a/lldb/source/Commands/CommandObjectExpression.cpp
+++ b/lldb/source/Commands/CommandObjectExpression.cpp
@@ -385,6 +385,7 @@ CommandObjectExpression::GetEvalOptions(const Target &target) {
auto_apply_fixits = m_command_options.auto_apply_fixits == eLazyBoolYes;
options.SetAutoApplyFixIts(auto_apply_fixits);
+ options.SetRetriesWithFixIts(target.GetNumberOfRetriesWithFixits());
if (m_command_options.top_level)
options.SetExecutionPolicy(eExecutionPolicyTopLevel);
diff --git a/lldb/source/Expression/UserExpression.cpp b/lldb/source/Expression/UserExpression.cpp
index 5bd2321e48dd..47d13f052bfb 100644
--- a/lldb/source/Expression/UserExpression.cpp
+++ b/lldb/source/Expression/UserExpression.cpp
@@ -266,22 +266,33 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx,
execution_results = lldb::eExpressionParseError;
if (fixed_expression && !fixed_expression->empty() &&
options.GetAutoApplyFixIts()) {
- lldb::UserExpressionSP fixed_expression_sp(
- target->GetUserExpressionForLanguage(fixed_expression->c_str(),
- full_prefix, language,
- desired_type, options, ctx_obj,
- error));
- DiagnosticManager fixed_diagnostic_manager;
- parse_success = fixed_expression_sp->Parse(
- fixed_diagnostic_manager, exe_ctx, execution_policy,
- keep_expression_in_memory, generate_debug_info);
- if (parse_success) {
- diagnostic_manager.Clear();
- user_expression_sp = fixed_expression_sp;
- } else {
- // If the fixed expression failed to parse, don't tell the user about,
- // that won't help.
- fixed_expression->clear();
+ const uint64_t max_fix_retries = options.GetRetriesWithFixIts();
+ for (uint64_t i = 0; i < max_fix_retries; ++i) {
+ // Try parsing the fixed expression.
+ lldb::UserExpressionSP fixed_expression_sp(
+ target->GetUserExpressionForLanguage(
+ fixed_expression->c_str(), full_prefix, language, desired_type,
+ options, ctx_obj, error));
+ DiagnosticManager fixed_diagnostic_manager;
+ parse_success = fixed_expression_sp->Parse(
+ fixed_diagnostic_manager, exe_ctx, execution_policy,
+ keep_expression_in_memory, generate_debug_info);
+ if (parse_success) {
+ diagnostic_manager.Clear();
+ user_expression_sp = fixed_expression_sp;
+ break;
+ } else {
+ // The fixed expression also didn't parse. Let's check for any new
+ // Fix-Its we could try.
+ if (fixed_expression_sp->GetFixedText()) {
+ *fixed_expression = fixed_expression_sp->GetFixedText();
+ } else {
+ // Fixed expression didn't compile without a fixit, don't retry and
+ // don't tell the user about it.
+ fixed_expression->clear();
+ break;
+ }
+ }
}
}
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index bdfd314695dc..9c61ddeadcc8 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -3740,6 +3740,12 @@ bool TargetProperties::GetEnableAutoApplyFixIts() const {
nullptr, idx, g_target_properties[idx].default_uint_value != 0);
}
+uint64_t TargetProperties::GetNumberOfRetriesWithFixits() const {
+ const uint32_t idx = ePropertyRetriesWithFixIts;
+ return m_collection_sp->GetPropertyAtIndexAsUInt64(
+ nullptr, idx, g_target_properties[idx].default_uint_value);
+}
+
bool TargetProperties::GetEnableNotifyAboutFixIts() const {
const uint32_t idx = ePropertyNotifyAboutFixIts;
return m_collection_sp->GetPropertyAtIndexAsBoolean(
diff --git a/lldb/source/Target/TargetProperties.td b/lldb/source/Target/TargetProperties.td
index fa4f4bdd9bd6..a73f1a3b695c 100644
--- a/lldb/source/Target/TargetProperties.td
+++ b/lldb/source/Target/TargetProperties.td
@@ -55,6 +55,9 @@ let Definition = "target" in {
def AutoApplyFixIts: Property<"auto-apply-fixits", "Boolean">,
DefaultTrue,
Desc<"Automatically apply fix-it hints to expressions.">;
+ def RetriesWithFixIts: Property<"retries-with-fixits", "UInt64">,
+ DefaultUnsignedValue<1>,
+ Desc<"Maximum number of attempts to fix an expression with Fix-Its">;
def NotifyAboutFixIts: Property<"notify-about-fixits", "Boolean">,
DefaultTrue,
Desc<"Print the fixed expression text.">;
diff --git a/lldb/test/API/commands/expression/fixits/TestFixIts.py b/lldb/test/API/commands/expression/fixits/TestFixIts.py
index 8ccb08ebbaa2..44d3ddd34068 100644
--- a/lldb/test/API/commands/expression/fixits/TestFixIts.py
+++ b/lldb/test/API/commands/expression/fixits/TestFixIts.py
@@ -81,3 +81,71 @@ def test_with_target(self):
self.assertTrue(
error_string.find("my_pointer->second.a") != -1,
"Fix was right")
+
+
+ # Test repeatedly applying Fix-Its to expressions and reparsing them.
+ multiple_runs_options = lldb.SBExpressionOptions()
+ multiple_runs_options.SetAutoApplyFixIts(True)
+ multiple_runs_options.SetTopLevel(True)
+
+ # An expression that needs two parse attempts with one Fix-It each
+ # to be successfully parsed.
+ two_runs_expr = """
+ struct Data { int m; };
+
+ template<typename T>
+ struct S1 : public T {
+ using T::TypeDef;
+ int f() {
+ Data d;
+ d.m = 123;
+ // The first error as the using above requires a 'typename '.
+ // Will trigger a Fix-It that puts 'typename' in the right place.
+ typename S1<T>::TypeDef i = &d;
+ // i has the type "Data *", so this should be i.m.
+ // The second run will change the . to -> via the Fix-It.
+ return i.m;
+ }
+ };
+
+ struct ClassWithTypeDef {
+ typedef Data *TypeDef;
+ };
+
+ int test_X(int i) {
+ S1<ClassWithTypeDef> s1;
+ return s1.f();
+ }
+ """
+
+ # Disable retries which will fail.
+ multiple_runs_options.SetRetriesWithFixIts(0)
+ value = frame.EvaluateExpression(two_runs_expr, multiple_runs_options)
+ self.assertIn("expression failed to parse, fixed expression suggested:",
+ value.GetError().GetCString())
+ self.assertIn("using typename T::TypeDef",
+ value.GetError().GetCString())
+ # The second Fix-It shouldn't be suggested here as Clang should have
+ # aborted the parsing process.
+ self.assertNotIn("i->m",
+ value.GetError().GetCString())
+
+ # Retry once, but the expression needs two retries.
+ multiple_runs_options.SetRetriesWithFixIts(1)
+ value = frame.EvaluateExpression(two_runs_expr, multiple_runs_options)
+ self.assertIn("expression failed to parse, fixed expression suggested:",
+ value.GetError().GetCString())
+ # Both our fixed expressions should be in the suggested expression.
+ self.assertIn("using typename T::TypeDef",
+ value.GetError().GetCString())
+ self.assertIn("i->m",
+ value.GetError().GetCString())
+
+ # Retry twice, which will get the expression working.
+ multiple_runs_options.SetRetriesWithFixIts(2)
+ value = frame.EvaluateExpression(two_runs_expr, multiple_runs_options)
+ # This error signals success for top level expressions.
+ self.assertEquals(value.GetError().GetCString(), "error: No value")
+
+ # Test that the code above compiles to the right thing.
+ self.expect_expr("test_X(1)", result_type="int", result_value="123")
More information about the lldb-commits
mailing list