[lldb] Make persisting result variables configurable

[kastiglione]

Summary

Context: The expression command uses artificial variables to store the expression
result. This result variable is unconditionally kept around after the expression command
has completed. These variables are known as persistent results. These are the variables
$0, $1, etc, that are displayed when running p or expression.

This change allows users to control whether result variables are persisted, by
introducing a --persistent-result flag.

This change keeps the current default behavior, persistent results are created by
default. This change gives users the ability to opt-out by re-aliasing p. For example:

command unalias p
command alias p expression --persistent-result false --

For consistency, this flag is also adopted by dwim-print. Of note, if asked,
dwim-print will create a persistent result even for frame variables.

[aprantl]

lldb/source/Commands/CommandObjectExpression.cpp

149

Do we expect people to actually use this flag or to just use an alias that has this turned on? If the latter, maybe we should only provide a long option, since the single-letter name space is precious?

[aprantl]

Did you forget to git-add the test?

[kastiglione]

Add tests

[kastiglione]

lldb/source/Commands/CommandObjectExpression.cpp

149

Initially I was going to have only the long option, but I reconsidered because I don't know how frequently persistent results are used by users. In the future, if/when we make the persistent results opt-in (instead of opt-out), will people want to use persistent results sporadically? If so maybe they'd want a short flag to quickly opt-in.

since the single-letter name space is precious

this is a compelling argument. I'll go with long option only to start. It's easier to add a short option later, than to remove it.

[kastiglione]

fix Makefile

[kastiglione]

Remove short option -C, leaving only --persistent-result

[aprantl]

lldb/source/Commands/Options.td

389

Is that the approved way of doing long options-only? What's printed in help dwim-print now?

[kastiglione]

lldb/source/Commands/Options.td

389

We don't have many long option-only flags, and those few others do it this way. If I recall, Jonas was the first to use this approach.

The relevant portion of help dwim-print (or help expression) is:

--persistent-result <boolean>

Persist expression result in a variable for subsequent use. Expression results will be
labeled with $-prefixed variables, e.g. $0, $1, etc. Defaults to true.

[kastiglione]

Improve handling of po

[aprantl]

lldb/source/Commands/Options.td

389

Okay, thanks for checking!

[kastiglione]

Fix composition of --persistent-result and -O/-v

lldb/source/Commands/CommandObjectDWIMPrint.cpp

result.AppendErrorWithFormatv("'{0}' takes a variable or expression",
                                   m_cmd_name);
     return false;
   }
 
   auto verbosity = GetDebugger().GetDWIMPrintVerbosity();
 
   DumpValueObjectOptions dump_options = m_varobj_options.GetAsDumpOptions(
       m_expr_options.m_verbosity, m_format_options.GetFormat());
+  if (m_expr_options.suppress_persistent_result)
+    dump_options.SetHideName(true);
 
   // First, try `expr` as the name of a frame variable.
   if (StackFrame *frame = m_exe_ctx.GetFramePtr()) {
     auto valobj_sp = frame->FindVariable(ConstString(expr));
     if (valobj_sp && valobj_sp->GetError().Success()) {
+      if (!m_expr_options.suppress_persistent_result)
+        valobj_sp = valobj_sp->Persist();
+
       if (verbosity == eDWIMPrintVerbosityFull) {
         StringRef flags;
         if (args.HasArgs())
           flags = args.GetArgString();
         result.AppendMessageWithFormatv("note: ran `frame variable {0}{1}`",
                                         flags, expr);
       }
+
       valobj_sp->Dump(result.GetOutputStream(), dump_options);
       result.SetStatus(eReturnStatusSuccessFinishResult);
       return true;
     }
   }
 
   // Second, also lastly, try `expr` as a source expression to evaluate.
   {
// Second, also lastly, try `expr` as a source expression to evaluate.
     if (expr_result == eExpressionCompleted) {
       if (verbosity != eDWIMPrintVerbosityNone) {
         StringRef flags;
         if (args.HasArgs())
           flags = args.GetArgStringWithDelimiter();
         result.AppendMessageWithFormatv("note: ran `expression {0}{1}`", flags,
                                         expr);
       }
+
       valobj_sp->Dump(result.GetOutputStream(), dump_options);
       result.SetStatus(eReturnStatusSuccessFinishResult);
       return true;
     } else {
       if (valobj_sp)
         result.SetError(valobj_sp->GetError());
       else
         result.AppendErrorWithFormatv(

lldb/source/Commands/CommandObjectExpression.h

public:
     bool show_types;
     bool show_summary;
     bool debug;
     uint32_t timeout;
     bool try_all_threads;
     lldb::LanguageType language;
     LanguageRuntimeDescriptionDisplayVerbosity m_verbosity;
     LazyBool auto_apply_fixits;
+    bool suppress_persistent_result;
   };
 
   CommandObjectExpression(CommandInterpreter &interpreter);
 
   ~CommandObjectExpression() override;
 
   Options *GetOptions() override;
 

lldb/source/Commands/CommandObjectExpression.cpp

if (success)
       auto_apply_fixits = tmp_value ? eLazyBoolYes : eLazyBoolNo;
     else
       error.SetErrorStringWithFormat(
           "could not convert \"%s\" to a boolean value.",
           option_arg.str().c_str());
     break;
   }
 
+  case 'C': {
+    // 'C' for "caching", since both 'P' and 'p' for persist are taken. Both 'R'
+    // flags are taken too.
+    bool success;
+    bool persist_result =
+        OptionArgParser::ToBoolean(option_arg, true, &success);
+    if (success)
+      suppress_persistent_result = !persist_result;
+    else
+      error.SetErrorStringWithFormat(
+          "could not convert \"%s\" to a boolean value.",
+          option_arg.str().c_str());
+    break;
+  }
+
   default:
     llvm_unreachable("Unimplemented option");
   }
 
   return error;
 }
 
 void CommandObjectExpression::CommandOptions::OptionParsingStarting(
void CommandObjectExpression::CommandOptions::OptionParsingStarting(
   try_all_threads = true;
   timeout = 0;
   debug = false;
   language = eLanguageTypeUnknown;
   m_verbosity = eLanguageRuntimeDescriptionDisplayVerbosityCompact;
   auto_apply_fixits = eLazyBoolCalculate;
   top_level = false;
   allow_jit = true;
+  suppress_persistent_result = false;
 }
 
 llvm::ArrayRef<OptionDefinition>
 CommandObjectExpression::CommandOptions::GetDefinitions() {
   return llvm::ArrayRef(g_expression_options);
 }
 
 EvaluateExpressionOptions
 CommandObjectExpression::CommandOptions::GetEvalateExpressionOptions(
     const Target &target, bool use_objc, lldb::DynamicValueType use_dynamic) {
   EvaluateExpressionOptions options;
   options.SetCoerceToId(use_objc);
-  if (m_verbosity == eLanguageRuntimeDescriptionDisplayVerbosityCompact)
+  if (suppress_persistent_result)
+    options.SetSuppressPersistentResult(true);
+  else if (m_verbosity == eLanguageRuntimeDescriptionDisplayVerbosityCompact)
     options.SetSuppressPersistentResult(use_objc);
   options.SetUnwindOnError(unwind_on_error);
   options.SetIgnoreBreakpoints(ignore_breakpoints);
   options.SetKeepInMemory(true);
   options.SetUseDynamic(use_dynamic);
   options.SetTryAllThreads(try_all_threads);
   options.SetDebug(debug);
   options.SetLanguage(language);
if (result_valobj_sp->GetError().Success()) {
                 "expression cannot be used with --element-count %s\n",
                 error.AsCString(""));
             return false;
           }
         }
 
         DumpValueObjectOptions options(m_varobj_options.GetAsDumpOptions(
             m_command_options.m_verbosity, format));
+        if (m_command_options.suppress_persistent_result)
+          options.SetHideName(true);
         options.SetVariableFormatDisplayLanguage(
             result_valobj_sp->GetPreferredDisplayLanguage());
 
         result_valobj_sp->Dump(output_stream, options);
 
         result.SetStatus(eReturnStatusSuccessFinishResult);
       }
     } else {

lldb/source/Commands/Options.td

let Command = "expression" in {
   def expression_options_top_level : Option<"top-level", "p">, Groups<[1,2]>,
     Desc<"Interpret the expression as a complete translation unit, without "
     "injecting it into the local context.  Allows declaration of persistent, "
     "top-level entities without a $ prefix.">;
   def expression_options_allow_jit : Option<"allow-jit", "j">, Groups<[1,2]>,
     Arg<"Boolean">,
     Desc<"Controls whether the expression can fall back to being JITted if it's "
     "not supported by the interpreter (defaults to true).">;
+  def persistent_result : Option<"persistent-result", "C">, Groups<[1,2]>,
+    Arg<"Boolean">,
+    Desc<"Persist expression result in a variable for subsequent use. "
+    "Expression results will be labeled with $-prefixed variables, e.g. $0, "
+    "$1, etc. Defaults to true.">;
 }
 
 let Command = "frame diag" in {
   def frame_diag_register : Option<"register", "r">, Group<1>,
     Arg<"RegisterName">, Desc<"A register to diagnose.">;
   def frame_diag_address : Option<"address", "a">, Group<1>, Arg<"Address">,
     Desc<"An address to diagnose.">;
   def frame_diag_offset : Option<"offset", "o">, Group<1>, Arg<"Offset">,

lldb/source/DataFormatters/ValueObjectPrinter.cpp

if (m_error.size()) {
           (entry == nullptr ||
            (entry->DoesPrintValue(m_valobj) ||
             m_options.m_format != eFormatDefault) ||
            m_summary.empty()) &&
           !m_options.m_hide_value) {
         if (m_options.m_hide_pointer_value &&
             IsPointerValue(m_valobj->GetCompilerType())) {
         } else {
-          m_stream->Printf(" %s", m_value.c_str());
+          if (!m_options.m_hide_name)
+            m_stream->Write(" ", 1);
+          m_stream->Write(m_value.c_str(), m_value.length());
           value_printed = true;
         }
       }
 
       if (m_summary.size()) {
         m_stream->Printf(" %s", m_summary.c_str());
         summary_printed = true;
       }