Merge pull request #6887 from apple/dl/lldb-delay-removal-of-persistent-results

[lldb] Delay removal of persistent results

Author: adrian-prantl

lldb/source/Commands/CommandObjectDWIMPrint.cpp

@@ -10,6 +10,7 @@
 
 #include "lldb/Core/ValueObject.h"
 #include "lldb/DataFormatters/DumpValueObjectOptions.h"
+#include "lldb/Expression/ExpressionVariable.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandObject.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
@@ -78,28 +79,34 @@ bool CommandObjectDWIMPrint::DoExecute(StringRef command,
   // If the user has not specified, default to disabling persistent results.
   if (m_expr_options.suppress_persistent_result == eLazyBoolCalculate)
     m_expr_options.suppress_persistent_result = eLazyBoolYes;
+  bool suppress_result = m_expr_options.ShouldSuppressResult(m_varobj_options);
 
   auto verbosity = GetDebugger().GetDWIMPrintVerbosity();
 
   Target *target_ptr = m_exe_ctx.GetTargetPtr();
   // Fallback to the dummy target, which can allow for expression evaluation.
   Target &target = target_ptr ? *target_ptr : GetDummyTarget();
 
-  const EvaluateExpressionOptions eval_options =
+  EvaluateExpressionOptions eval_options =
       m_expr_options.GetEvaluateExpressionOptions(target, m_varobj_options);
+  // This command manually removes the result variable, make sure expression
+  // evaluation doesn't do it first.
+  eval_options.SetSuppressPersistentResult(false);
 
   DumpValueObjectOptions dump_options = m_varobj_options.GetAsDumpOptions(
       m_expr_options.m_verbosity, m_format_options.GetFormat());
-  dump_options.SetHideRootName(eval_options.GetSuppressPersistentResult());
+  dump_options.SetHideRootName(suppress_result);
 
   StackFrame *frame = m_exe_ctx.GetFramePtr();
 
   // First, try `expr` as the name of a frame variable.
   if (frame) {
     auto valobj_sp = frame->FindVariable(ConstString(expr));
     if (valobj_sp && valobj_sp->GetError().Success()) {
-      if (!eval_options.GetSuppressPersistentResult())
-        valobj_sp = valobj_sp->Persist();
+      if (!suppress_result) {
+        if (auto persisted_valobj = valobj_sp->Persist())
+          valobj_sp = persisted_valobj;
+      }
 
       if (verbosity == eDWIMPrintVerbosityFull) {
         StringRef flags;
@@ -172,6 +179,16 @@ bool CommandObjectDWIMPrint::DoExecute(StringRef command,
       }
 
       valobj_sp->Dump(result.GetOutputStream(), dump_options);
+
+      if (suppress_result)
+        if (auto result_var_sp =
+                target.GetPersistentVariable(valobj_sp->GetName())) {
+          auto language = valobj_sp->GetPreferredDisplayLanguage();
+          if (auto *persistent_state =
+                  target.GetPersistentExpressionStateForLanguage(language))
+            persistent_state->RemovePersistentVariable(result_var_sp);
+        }
+
       result.SetStatus(eReturnStatusSuccessFinishResult);
       return true;
     } else {

lldb/source/Commands/CommandObjectExpression.cpp

@@ -10,6 +10,7 @@
 
 #include "CommandObjectExpression.h"
 #include "lldb/Core/Debugger.h"
+#include "lldb/Expression/ExpressionVariable.h"
 #include "lldb/Expression/REPL.h"
 #include "lldb/Expression/UserExpression.h"
 #include "lldb/Host/OptionParser.h"
@@ -21,6 +22,7 @@
 #include "lldb/Target/Process.h"
 #include "lldb/Target/StackFrame.h"
 #include "lldb/Target/Target.h"
+#include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-private-enumerations.h"
 
 // BEGIN SWIFT
@@ -218,13 +220,6 @@ CommandObjectExpression::CommandOptions::GetEvaluateExpressionOptions(
     const Target &target, const OptionGroupValueObjectDisplay &display_opts) {
   EvaluateExpressionOptions options;
   options.SetCoerceToId(display_opts.use_objc);
-  // Explicitly disabling persistent results takes precedence over the
-  // m_verbosity/use_objc logic.
-  if (suppress_persistent_result != eLazyBoolCalculate)
-    options.SetSuppressPersistentResult(suppress_persistent_result ==
-                                        eLazyBoolYes);
-  else if (m_verbosity == eLanguageRuntimeDescriptionDisplayVerbosityCompact)
-    options.SetSuppressPersistentResult(display_opts.use_objc);
   options.SetUnwindOnError(unwind_on_error);
   options.SetIgnoreBreakpoints(ignore_breakpoints);
   options.SetKeepInMemory(true);
@@ -272,6 +267,17 @@ CommandObjectExpression::CommandOptions::GetEvaluateExpressionOptions(
   return options;
 }
 
+bool CommandObjectExpression::CommandOptions::ShouldSuppressResult(
+    const OptionGroupValueObjectDisplay &display_opts) const {
+  // Explicitly disabling persistent results takes precedence over the
+  // m_verbosity/use_objc logic.
+  if (suppress_persistent_result != eLazyBoolCalculate)
+    return suppress_persistent_result == eLazyBoolYes;
+
+  return display_opts.use_objc &&
+         m_verbosity == eLanguageRuntimeDescriptionDisplayVerbosityCompact;
+}
+
 CommandObjectExpression::CommandObjectExpression(
     CommandInterpreter &interpreter)
     : CommandObjectRaw(interpreter, "expression",
@@ -456,8 +462,12 @@ bool CommandObjectExpression::EvaluateExpression(llvm::StringRef expr,
     return false;
   }
 
-  const EvaluateExpressionOptions eval_options =
+  EvaluateExpressionOptions eval_options =
       m_command_options.GetEvaluateExpressionOptions(target, m_varobj_options);
+  // This command manually removes the result variable, make sure expression
+  // evaluation doesn't do it first.
+  eval_options.SetSuppressPersistentResult(false);
+
   ExpressionResults success = target.EvaluateExpression(
       expr, frame, result_valobj_sp, eval_options, &m_fixed_expression);
 
@@ -486,14 +496,25 @@ bool CommandObjectExpression::EvaluateExpression(llvm::StringRef expr,
           }
         }
 
+        bool suppress_result =
+            m_command_options.ShouldSuppressResult(m_varobj_options);
+
         DumpValueObjectOptions options(m_varobj_options.GetAsDumpOptions(
             m_command_options.m_verbosity, format));
-        options.SetHideRootName(eval_options.GetSuppressPersistentResult());
+        options.SetHideRootName(suppress_result);
         options.SetVariableFormatDisplayLanguage(
             result_valobj_sp->GetPreferredDisplayLanguage());
 
         result_valobj_sp->Dump(output_stream, options);
 
+        if (suppress_result)
+          if (auto result_var_sp =
+                  target.GetPersistentVariable(result_valobj_sp->GetName())) {
+            auto language = result_valobj_sp->GetPreferredDisplayLanguage();
+            if (auto *persistent_state =
+                    target.GetPersistentExpressionStateForLanguage(language))
+              persistent_state->RemovePersistentVariable(result_var_sp);
+          }
         result.SetStatus(eReturnStatusSuccessFinishResult);
       }
     } else {

lldb/source/Commands/CommandObjectExpression.h

@@ -41,6 +41,9 @@ class CommandObjectExpression : public CommandObjectRaw,
         const Target &target,
         const OptionGroupValueObjectDisplay &display_opts);
 
+    bool ShouldSuppressResult(
+        const OptionGroupValueObjectDisplay &display_opts) const;
+
     bool top_level;
     bool unwind_on_error;
     bool ignore_breakpoints;

lldb/test/API/commands/expression/persistent_result/swift/Makefile

@@ -0,0 +1,3 @@
+SWIFT_SOURCES := main.swift
+
+include Makefile.rules

lldb/test/API/commands/expression/persistent_result/swift/TestSwiftPersistentResult.py

@@ -0,0 +1,37 @@
+"""
+Test controlling `expression` result variables are persistent.
+"""
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from lldbsuite.test import lldbutil
+
+
+class TestCase(TestBase):
+    def setUp(self):
+        TestBase.setUp(self)
+        self.build()
+        lldbutil.run_to_source_breakpoint(self, "break here", lldb.SBFileSpec("main.swift"))
+
+    def test_enable_persistent_result(self):
+        """Test explicitly enabling result variables persistence."""
+        self.expect("expression --persistent-result on -- i", startstr="(Int) $R0 = 30")
+        # Verify the lifetime of $R0 extends beyond the `expression` it was created in.
+        self.expect("expression $R0", startstr="(Int) $R1 = 30")
+
+    def test_disable_persistent_result(self):
+        """Test explicitly disabling persistent result variables."""
+        self.expect("expression --persistent-result off -- i", startstr="(Int) 30")
+        # Verify a persistent result was not silently created.
+        self.expect("expression $R0", error=True)
+
+    def test_expression_persists_result(self):
+        """Test `expression`'s default behavior is to persist a result variable."""
+        self.expect("expression i", startstr="(Int) $R0 = 30")
+        self.expect("expression $R0", startstr="(Int) $R1 = 30")
+
+    def test_p_does_not_persist_results(self):
+        """Test `p` does not persist a result variable."""
+        self.expect("p i", startstr="(Int) 30")
+        self.expect("p $R0", error=True)

lldb/test/API/commands/expression/persistent_result/swift/main.swift

@@ -0,0 +1,6 @@
+func main() {
+  var i = 30
+  print("break here")
+}
+
+main()