[lldb] Add option to retry Fix-Its multiple times to failed expressions

[teemperor]

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.

[shafik]

lldb/source/Expression/UserExpression.cpp

269

Why not uint32_t?

lldb/source/Target/Target.cpp

3715

Why not uint32_t?

[teemperor]

Yeah the 32/64 mix is because the options don't directly support uint32_t values while the SBAPI heavily prefers uint32_t over uint64_t parameter. But there are some parts of the SB API that use uint64_t so I think the bets way out of this is to just use uint64_t consistently everywhere.

[omjavaid]

This patch fails on lldb-aarch64-linux with following backtrace, apparently failing experession eval. I am marking it xfail for now.

Traceback (most recent call last):

File "/home/omair.javaid/work/lldb/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1751, in test_method
  return attrvalue(self)
File "/home/omair.javaid/work/lldb/llvm-project/lldb/test/API/commands/expression/fixits/TestFixIts.py", line 151, in test_with_target
  self.expect_expr("test_X(1)", result_type="int", result_value="123")
File "/home/omair.javaid/work/lldb/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2426, in expect_expr
  "Unexpected failure with msg: " + eval_result.GetError().GetCString())

AssertionError: False is not True : Unexpected failure with msg: error: Execution was interrupted, reason: signal SIGILL: illegal instruction.
The process has been returned to the state before expression evaluation.

[teemperor]

Somehow Phab forget to close this one.

lldb/bindings/interface/SBExpressionOptions.i

public:
     %feature("docstring", "Sets whether to auto-apply fix-it hints to the expression being evaluated.") SetAutoApplyFixIts;
     void
     SetAutoApplyFixIts(bool b = true);
 
     %feature("docstring", "Gets whether to auto-apply fix-it hints to an expression.") GetAutoApplyFixIts;
     bool
     GetAutoApplyFixIts();
 
+    %feature("docstring", "Sets how often LLDB should retry applying fix-its to an expression.") SetRetriesWithFixIts;
+    void
+    SetRetriesWithFixIts(uint32_t retries);
+
+    %feature("docstring", "Gets how often LLDB will retry applying fix-its to an expression.") GetRetriesWithFixIts;
+    uint32_t
+    GetRetriesWithFixIts();
+
     bool
     GetTopLevel();
 
     void
     SetTopLevel(bool b = true);
 
     %feature("docstring", "Gets whether to JIT an expression if it cannot be interpreted.") GetAllowJIT;
     bool

lldb/include/lldb/API/SBExpressionOptions.h

public:
   const char *GetPrefix() const;
 
   void SetPrefix(const char *prefix);
 
   void SetAutoApplyFixIts(bool b = true);
 
   bool GetAutoApplyFixIts();
 
+  void SetRetriesWithFixIts(uint32_t retries);
+
+  uint32_t GetRetriesWithFixIts();
+
   bool GetTopLevel();
 
   void SetTopLevel(bool b = true);
 
   // Gets whether we will JIT an expression if it cannot be interpreted
   bool GetAllowJIT();
 
   // Sets whether we will JIT an expression if it cannot be interpreted

lldb/include/lldb/Target/Target.h

public:
   FileSpecList GetClangModuleSearchPaths();
 
   bool GetEnableAutoImportClangModules() const;
 
   bool GetEnableImportStdModule() const;
 
   bool GetEnableAutoApplyFixIts() const;
 
+  uint64_t GetNumberOfRetriesWithFixits() const;
+
   bool GetEnableNotifyAboutFixIts() const;
 
   bool GetEnableSaveObjects() const;
 
   bool GetEnableSyntheticValue() const;
 
   uint32_t GetMaxZeroPaddingInFloatFormat() const;
 
#endif
   void SetResultIsInternal(bool b) { m_result_is_internal = b; }
 
   bool GetResultIsInternal() const { return m_result_is_internal; }
 
   void SetAutoApplyFixIts(bool b) { m_auto_apply_fixits = b; }
 
   bool GetAutoApplyFixIts() const { return m_auto_apply_fixits; }
 
+  void SetRetriesWithFixIts(uint32_t number_of_retries) {
+    m_retries_with_fixits = number_of_retries;
+  }
+
+  uint32_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; }
 
 private:
   ExecutionPolicy m_execution_policy = default_execution_policy;
   lldb::LanguageType m_language = lldb::eLanguageTypeUnknown;
   std::string m_prefix;
   bool m_coerce_to_id = false;
   bool m_unwind_on_error = true;
   bool m_ignore_breakpoints = false;
   bool m_keep_in_memory = false;
   bool m_try_others = true;
   bool m_stop_others = true;
   bool m_debug = false;
   bool m_trap_exceptions = true;
   bool m_repl = false;
   bool m_generate_debug_info = false;
   bool m_ansi_color_errors = false;
   bool m_result_is_internal = false;
   bool m_auto_apply_fixits = true;
+  uint32_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;
 
   lldb::DynamicValueType m_use_dynamic = lldb::eNoDynamicValues;
   Timeout<std::micro> m_timeout = default_timeout;
   Timeout<std::micro> m_one_thread_timeout = llvm::None;
   lldb::ExpressionCancelCallback m_cancel_callback = nullptr;

lldb/source/API/SBExpressionOptions.cpp

 }
 
 void SBExpressionOptions::SetAutoApplyFixIts(bool b) {
   LLDB_RECORD_METHOD(void, SBExpressionOptions, SetAutoApplyFixIts, (bool), b);
 
   return m_opaque_up->SetAutoApplyFixIts(b);
 }
 
+uint32_t SBExpressionOptions::GetRetriesWithFixIts() {
+  LLDB_RECORD_METHOD_NO_ARGS(uint32_t, SBExpressionOptions,
+                             GetRetriesWithFixIts);
+
+  return m_opaque_up->GetRetriesWithFixIts();
+}
+
+void SBExpressionOptions::SetRetriesWithFixIts(uint32_t retries) {
+  LLDB_RECORD_METHOD(void, SBExpressionOptions, SetRetriesWithFixIts,
+                     (uint32_t), retries);
+
+  return m_opaque_up->SetRetriesWithFixIts(retries);
+}
+
 bool SBExpressionOptions::GetTopLevel() {
   LLDB_RECORD_METHOD_NO_ARGS(bool, SBExpressionOptions, GetTopLevel);
 
   return m_opaque_up->GetExecutionPolicy() == eExecutionPolicyTopLevel;
 }
 
 void SBExpressionOptions::SetTopLevel(bool b) {
   LLDB_RECORD_METHOD(void, SBExpressionOptions, SetTopLevel, (bool), b);

lldb/source/Expression/UserExpression.cpp

UserExpression::Evaluate(ExecutionContext &exe_ctx,
   if (!parse_success) {
     // Delete the expression that failed to parse before attempting to parse
     // the next expression.
     user_expression_sp.reset();
 
     execution_results = lldb::eExpressionParseError;
     if (fixed_expression && !fixed_expression->empty() &&
         options.GetAutoApplyFixIts()) {
+      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));
+            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 {
-        // If the fixed expression failed to parse, don't tell the user about,
-        // that won't help.
+            // Fixed expression didn't compile without a fixit, don't retry and
+            // don't tell the user about it.
             fixed_expression->clear();
+            break;
+          }
+        }
       }
     }
 
     if (!parse_success) {
       if (!fixed_expression->empty() && target->GetEnableNotifyAboutFixIts()) {
         error.SetExpressionErrorWithFormat(
             execution_results,
             "expression failed to parse, fixed expression suggested:\n  %s",

lldb/source/Target/Target.cpp

 }
 
 bool TargetProperties::GetEnableAutoApplyFixIts() const {
   const uint32_t idx = ePropertyAutoApplyFixIts;
   return m_collection_sp->GetPropertyAtIndexAsBoolean(
       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(
       nullptr, idx, g_target_properties[idx].default_uint_value != 0);
 }
 
 bool TargetProperties::GetEnableSaveObjects() const {
   const uint32_t idx = ePropertySaveObjects;

lldb/source/Target/TargetProperties.td

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 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.">;
   def SaveObjects: Property<"save-jit-objects", "Boolean">,
     DefaultFalse,
     Desc<"Save intermediate object files generated by the LLVM JIT">;
   def MaxZeroPaddingInFloatFormat: Property<"max-zero-padding-in-float-format", "UInt64">,
     DefaultUnsignedValue<6>,

lldb/test/API/commands/expression/fixits/TestFixIts.py

def test_with_target(self):
         self.assertTrue(value.GetError().Fail())
         error_string = value.GetError().GetCString()
         self.assertTrue(
             error_string.find("fixed expression suggested:") != -1,
             "Fix was suggested")
         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")