[lldb] Delay removal of persistent results

[kastiglione]

Summary

Follow up to "Suppress persistent result when running po" (D144044).

This change delays removal of the persistent result until after Dump has been called.
In doing so, the persistent result is available for the purpose of getting its object
description.

In the original change, the persistent result removal happens indirectly, by setting
EvaluateExpressionOptions::SetSuppressPersistentResult. In practice this has worked,
however this exposed a latent bug in swift-lldb. The subtlety, and the bug, depend on
when the persisteted result variable is removed.

When the result is removed via SetSuppressPersistentResult, it happens within the call
to Target::EvaluateExpression. That is, by the time the call returns, the persistent
result is already removed.

The issue occurs shortly thereafter, when ValueObject::Dump is called, it cannot make
use of the persistent result variable (instead it uses the ValueObjectConstResult). In
swift-lldb, this causes an additional expression evaluation to happen. It first tries an
expression that reference $R0 etc, but that always fails because $R0 is removed. The
fallback to this failure does work most of the time, but there's at least one bug
involving imported Clang types.

rdar://108656999

[kastiglione]

rebase

[kastiglione]

This change is tested using the original tests from D144044.

[kastiglione]

check frame pointer before calling GuessLanguage

[kastiglione]

s/frame pointer/pointer to StackFrame/

[jingham]

This seems fine in general, with one quibble:

IIUC, the "indirect" removal of persistent results which you are trying to avoid happens here:

lldb::ExpressionResults
UserExpression::Execute(DiagnosticManager &diagnostic_manager,
                        ExecutionContext &exe_ctx,
                        const EvaluateExpressionOptions &options,
                        lldb::UserExpressionSP &shared_ptr_to_me,
                        lldb::ExpressionVariableSP &result_var) {
  lldb::ExpressionResults expr_result = DoExecute(
      diagnostic_manager, exe_ctx, options, shared_ptr_to_me, result_var);
  Target *target = exe_ctx.GetTargetPtr();
  if (options.GetSuppressPersistentResult() && result_var && target) {
    if (auto *persistent_state =
            target->GetPersistentExpressionStateForLanguage(m_language))
      persistent_state->RemovePersistentVariable(result_var);
  }
  return expr_result;
}

So to succeed in preserving the persistent result, your patch relies on SuppressPersistentResult being false in the EvaluateExpressionOptions you pass to the expression. However, you derive the expression options from the command options, for instance in:

const EvaluateExpressionOptions eval_options =
    m_command_options.GetEvaluateExpressionOptions(target, m_varobj_options);

so you don't actually know what the value of the SuppressPersistentResults is going to be. Since you rely on this having a particular value regardless of the user's intentions, you should do SetSuppressPersistentResults explicitly (with an appropriate comment) after you've fetch the eval_options in the dwim-print and expr commands.

A much smaller quibble is that it seems a little weird to ask the expression options or the frame which language in the PersistentExpressionResultsForLanguage map the result variable was stuffed into when you have the Result ValueObject on hand. That seems like something the ValueObject should tell you. When we Persist ValueObjects we use ValueObject::GetPreferredDisplayLanguage. That seems more trustworthy - if it isn't we should make it so...

[kastiglione]

• Explicitly control expression evaluation options for suppressing persistent result
• Use the ValueObject's display language instead of the CU or frame's language

[kastiglione]

thank you @jingham, that is great feedback!

lldb/source/Commands/CommandObjectDWIMPrint.cpp

 //===-- CommandObjectDWIMPrint.cpp ------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "CommandObjectDWIMPrint.h"
 
 #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"
 #include "lldb/Interpreter/OptionGroupFormat.h"
 #include "lldb/Interpreter/OptionGroupValueObjectDisplay.h"
 #include "lldb/Target/StackFrame.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/lldb-defines.h"
if (args.HasArgs()) {
     if (!ParseOptionsAndNotify(args.GetArgs(), result, m_option_group,
                                m_exe_ctx))
       return false;
   }
 
   // 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 (StackFrame *frame = m_exe_ctx.GetFramePtr()) {
+  if (frame) {
     auto valobj_sp = frame->FindVariable(ConstString(expr));
     if (valobj_sp && valobj_sp->GetError().Success()) {
-      if (!eval_options.GetSuppressPersistentResult()) {
+      if (!suppress_result) {
         if (auto persisted_valobj = valobj_sp->Persist())
           valobj_sp = persisted_valobj;
       }
 
       if (verbosity == eDWIMPrintVerbosityFull) {
         StringRef flags;
         if (args.HasArgs())
           flags = args.GetArgString();
if (expr_result == eExpressionCompleted) {
         StringRef flags;
         if (args.HasArgs())
           flags = args.GetArgStringWithDelimiter();
         result.AppendMessageWithFormatv("note: ran `expression {0}{1}`", flags,
                                         expr);
       }
 
       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 {
       if (valobj_sp)
         result.SetError(valobj_sp->GetError());
       else
         result.AppendErrorWithFormatv(
             "unknown error evaluating expression `{0}`", expr);
       return false;
     }
   }
 }

lldb/source/Commands/CommandObjectExpression.h

public:
     void OptionParsingStarting(ExecutionContext *execution_context) override;
 
     /// Return the appropriate expression options used for evaluating the
     /// expression in the given target.
     EvaluateExpressionOptions GetEvaluateExpressionOptions(
         const Target &target,
         const OptionGroupValueObjectDisplay &display_opts);
 
+    bool ShouldSuppressResult(
+        const OptionGroupValueObjectDisplay &display_opts) const;
+
     bool top_level;
     bool unwind_on_error;
     bool ignore_breakpoints;
     bool allow_jit;
     bool show_types;
     bool show_summary;
     bool debug;
     uint32_t timeout;

lldb/source/Commands/CommandObjectExpression.cpp

 //===-- CommandObjectExpression.cpp ---------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/StringRef.h"
 
 #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"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandOptionArgumentTable.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "lldb/Interpreter/OptionArgParser.h"
 #include "lldb/Target/Language.h"
 #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"
 
 using namespace lldb;
 using namespace lldb_private;
 
 CommandObjectExpression::CommandOptions::CommandOptions() = default;
 
 CommandObjectExpression::CommandOptions::~CommandOptions() = default;
CommandObjectExpression::CommandOptions::GetDefinitions() {
   return llvm::ArrayRef(g_expression_options);
 }
 
 EvaluateExpressionOptions
 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);
   options.SetUseDynamic(display_opts.use_dynamic);
   options.SetTryAllThreads(try_all_threads);
   options.SetDebug(debug);
   options.SetLanguage(language);
   options.SetExecutionPolicy(
CommandObjectExpression::CommandOptions::GetEvaluateExpressionOptions(
 
   if (timeout > 0)
     options.SetTimeout(std::chrono::microseconds(timeout));
   else
     options.SetTimeout(std::nullopt);
   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",
                        "Evaluate an expression on the current "
                        "thread.  Displays any returned value "
                        "with LLDB's default formatting.",
                        "",
                        eCommandProcessMustBePaused | eCommandTryTargetAPILock),
bool CommandObjectExpression::EvaluateExpression(llvm::StringRef expr,
   StackFrame *frame = exe_ctx.GetFramePtr();
 
   if (m_command_options.top_level && !m_command_options.allow_jit) {
     result.AppendErrorWithFormat(
         "Can't disable JIT compilation for top-level expressions.\n");
     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);
 
   // We only tell you about the FixIt if we applied it.  The compiler errors
   // will suggest the FixIt if it parsed.
   if (!m_fixed_expression.empty() && target.GetEnableNotifyAboutFixIts()) {
     error_stream.Printf("  Fix-it applied, fixed expression was: \n    %s\n",
                         m_fixed_expression.c_str());
if (result_valobj_sp->GetError().Success()) {
           if (error.Fail()) {
             result.AppendErrorWithFormat(
                 "expression cannot be used with --element-count %s\n",
                 error.AsCString(""));
             return false;
           }
         }
 
+        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 {
       if (result_valobj_sp->GetError().GetError() ==
           UserExpression::kNoResult) {
         if (format != eFormatVoid && GetDebugger().GetNotifyVoid()) {
           error_stream.PutCString("(void)\n");
         }