[lldb] Change dwim-print to default to disabled persistent results

[kastiglione]

Summary

Change dwim-print to now disable persistent results by default, unless requested by
the user with the --persistent-result flag.

Ex:

(lldb) dwim-print 1 + 1
(int) 2
(lldb) dwim-print --persistent-result on -- 1 + 1
(int) $0 = 2

Users who wish to enable persistent results can make and use an alias that includes
--persistent-result on.

[kastiglione]

Cleanup TestDWIMPrint

[jgorbe]

What was the rationale for this change? It changes the output format of a common command (given that print is now an alias for dwim-print) and it breaks basically anything that examines debugger output.

Also --persistent-result on can't be passed to print, it only works for me if I run specifically dwim-print. Is that intended?

[kastiglione]

Also --persistent-result on can't be passed to print, it only works for me if I run specifically dwim-print. Is that intended?

The intension is that for users who want persistent results enabled can do so by customizing their p or print alias, like so:

command unalias print
command alias print dwim-print --persistent-result on --

What was the rationale for this change? It changes the output format of a common command (given that print is now an alias for dwim-print) and it breaks basically anything that examines debugger output.

Are you using persistent results? If not, how much effort is it to either 1) change the tools/code that examine the output to not look for $\d+, or 2) use a custom print/p alias? Honest question.

The rationale is: dwim-print doesn't always use expression evaluation, it prefers to use frame variable where possible. In the future it could be expanded, for example to print register as well. Because dwim-print doesn't always use expression, there isn't always a persistent result. To make dwim-print output consistent, and because it's presumed most users don't use persistent results, we changed dwim-print to default to no persistent results. By consistent output, I mean that if a user runs print someVar and then follows that by running print someVar.dump(), it could be confusing if the first command doesn't have a persistent result, but the second one does.

[aprantl]

breaks basically anything that examines debugger output

I'd be curious to learn about some example you have in mind here.

[dblaikie]

The rationale is: dwim-print doesn't always use expression evaluation, it prefers to use frame variable where possible. In the future it could be expanded, for example to print register as well. Because dwim-print doesn't always use expression, there isn't always a persistent result.

Oh, so in the cases where dwim-print delegates to frame variable you can't then refer to that result in another dwim-print command/other place where you'd like to reference a previously printed value?

That seems like a significant regression over existing print functionality and compared to gdb. At least for me that'd be annoying/inconvenient to have to think about which kind of printing I'm doing (& have to explicitly use the less stable full expression based printing) when I want to reuse a value.

To make dwim-print output consistent, and because it's presumed most users don't use persistent results

Got data on that? I'd /guess/ I use it maybe in single digit % of my prints, but probably at least 1%, though no idea if that's representative - even if it is, the inconsistency feels counter to the goals of dwim-print & the convenience provided by gdb's consistent value handling.

[kastiglione]

Oh, so in the cases where dwim-print delegates to frame variable you can't then refer to that result in another dwim-print command/other place where you'd like to reference a previously printed value?

If the variable is in scope, you can use the variable. If it's not in scope, can you tell me about the workflows that you'd use it in?

That seems like a significant regression over existing print functionality and compared to gdb. At least for me that'd be annoying/inconvenient to have to think about which kind of printing I'm doing (& have to explicitly use the less stable full expression based printing) when I want to reuse a value.

Our expectation is that for users such as yourself, the solution is to customize the p alias, or alternatively create a separate alias, like pr (print w/ result). Does that seem reasonable? My plan is to use e for the cases where I want persistent results, since expression still defaults to persisting result variables.

Got data on that? I'd /guess/ I use it maybe in single digit % of my prints, but probably at least 1%, though no idea if that's representative - even if it is, the inconsistency feels counter to the goals of dwim-print & the convenience provided by gdb's consistent value handling.

Re data: only anecdotal. I expect that those of us who work on lldb represent the biggest users of persistent results, but how much is that? You estimate 1%. I use it less than that, maybe one out of a thousand (0.01%), but probably less. @aprantl has mentioned he's more likely to hit the up arrow and edit a previous command than use persistent results, which I can see being true for most users.

It's not that the ability is taken away, only that the default is being set to match the majority of cases – the 99+% of prints don't need a persistent result.

[jgorbe]

Are you using persistent results? If not, how much effort is it to either 1) change the tools/code that examine the output to not look for $\d+, or 2) use a custom print/p alias? Honest question.

I'm not using them myself, and it's not much effort to fix the problems I've seen because of this. It just seemed weird to have a user-facing behavior change like this being committed without a rationale given.

I assumed switching to dwim-print would be transparent for users, and I'm not sure how a user that relies on these convenience variables is supposed to discover what happened when print stops producing them.

I'd be curious to learn about some example you have in mind here.

We have a python test harness that drives a debugger session that we mainly use for prettyprinter tests. Then we have tests that look like:

self.ContinueTo('StopHereForDebugger')
self.TestPrintOutputMatches('my_variable', 'Cord of length 4: "foo\\n"')

when I ported this from gdb I couldn't find an equivalent to gdb's output command (that prints just the value), so I reached for print which printed (Type) $0 = value instead, and kept everything after the ' = '. It wasn't even using the persistent values, but the change in the output format broke it.

I also found a failure in a one-off test that worked as a minimal sanity check for debug builds: build a debug binary, then run the debugger on it and try to print some variables in order to verify that debug builds do have debug info that the debugger can use (i.e. debug info is not completely missing, or corrupted). This is a shell script that basically does:

OUTPUT=$("$LLDB" -b -s "$SCRIPT_FILE" "$TEST_PROGRAM")

and then used the conveniently sequentially-numbered persistent results to grep the output:

# Check that `argc` is 1 and has type `int`.
(echo "$OUTPUT" | grep "(int).*[$]0 = 1") || die

I know they're not the most robust tests in the world, and as I said, it was not much effort to fix (they're already fixed!). But it still feels like a user-facing regression, and the consistency argument can go both ways: you could similarly argue that dwim-print should put the result of frame variable into a convenience variable to keep it consistent with the cases where the expression evaluator is used. So the real argument is that most users don't need it and it shouldn't be enabled by default. Which is a fine argument to make (although it's sad that we don't have data to support it one way or the other), but I had to ask to know because the commit description didn't mention any reasons at all.

[jgorbe]

By the way, I just bisected another problem to this commit: I'm seeing in some cases that prettyprinted values with synthetic children don't show child names when using dwim-print. So print used to show:

(lldb) p c
(MyClass) $0 = MyClass object with children: {
  [0] = 0x0000555555556004 "hello"
}
(lldb) p m
(MyMap) $1 = MyMap object with children: {
  ["my_key"] = 0x0000555555556011 "my_value"
}

but now it shows

(lldb) p c
(MyClass)  MyClass object with children: {
  0x0000555555556004 "hello"
}
(lldb) p m
(MyMap)  MyMap object with children: {
  0x0000555555556011 "my_value"
}

I don't have a repro test case to share right away but I though I'd give you a heads-up in case the problem is easy to find from the description. Or maybe it's the intended behavior? I get the same behavior with expr --persistent-result off -- m so I can't tell if it's a bug or if it's just the expected behavior when disabling persistent results. If it's the latter, that's a strong argument for me to unalias print and restore the old behavior in the lldbinit file we ship to our internal users.

I'll work on a small shareable test case tomorrow anyway.

[kastiglione]

@jgorbe thank you for letting me know! It's not intended that the names of child values are not printed. I have a fix ready for review here D146783.

lldb/source/Commands/CommandObjectDWIMPrint.cpp

if (!ParseOptionsAndNotify(args.GetArgs(), result, m_option_group,
                                m_exe_ctx))
       return false;
   } else if (command.empty()) {
     result.AppendErrorWithFormatv("'{0}' takes a variable or expression",
                                   m_cmd_name);
     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;
+
   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 =
       m_expr_options.GetEvaluateExpressionOptions(target, m_varobj_options);

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;
+    LazyBool suppress_persistent_result;
   };
 
   CommandObjectExpression(CommandInterpreter &interpreter);
 
   ~CommandObjectExpression() override;
 
   Options *GetOptions() override;
 

lldb/source/Commands/CommandObjectExpression.cpp

case 'X': {
     break;
   }
 
   case '\x01': {
     bool success;
     bool persist_result =
         OptionArgParser::ToBoolean(option_arg, true, &success);
     if (success)
-      suppress_persistent_result = !persist_result;
+      suppress_persistent_result = !persist_result ? eLazyBoolYes : eLazyBoolNo;
     else
       error.SetErrorStringWithFormat(
           "could not convert \"%s\" to a boolean value.",
           option_arg.str().c_str());
     break;
   }
 
   default:
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;
+  suppress_persistent_result = eLazyBoolCalculate;
 }
 
 llvm::ArrayRef<OptionDefinition>
 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)
-    options.SetSuppressPersistentResult(true);
+  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);

lldb/test/API/commands/dwim-print/TestDWIMPrint.py

 
 class TestCase(TestBase):
     def _run_cmd(self, cmd: str) -> str:
         """Run the given lldb command and return its output."""
         result = lldb.SBCommandReturnObject()
         self.ci.HandleCommand(cmd, result)
         return result.GetOutput().rstrip()
 
-    VAR_IDENT_RAW = r"(?:\$\d+|\w+) = "
-    VAR_IDENT = re.compile(VAR_IDENT_RAW)
+    VAR_IDENT = re.compile(r"(?:\$\d+|\w+) = ")
 
-    def _mask_persistent_var(self, string: str) -> str:
+    def _strip_result_var(self, string: str) -> str:
         """
-        Replace persistent result variables (ex '$0', '$1', etc) with a regex
-        that matches any persistent result (r'\$\d+'). The returned string can
-        be matched against other `expression` results.
+        Strip (persistent) result variables (ex '$0 = ', or 'someVar = ', etc).
+
+        This allows for using the output of `expression`/`frame variable`, to
+        compare it to `dwim-print` output, which disables result variables.
         """
-        before, after = self.VAR_IDENT.split(string, maxsplit=1)
-        # Support either a frame variable (\w+) or a persistent result (\$\d+).
-        return re.escape(before) + self.VAR_IDENT_RAW + re.escape(after)
+        return self.VAR_IDENT.subn("", string, 1)[0]
 
     def _expect_cmd(
         self,
         dwim_cmd: str,
         actual_cmd: str,
     ) -> None:
         """Run dwim-print and verify the output against the expected command."""
         # Resolve the dwim-print command to either `expression` or `frame variable`.
         substitute_cmd = dwim_cmd.replace("dwim-print", actual_cmd, 1)
         interp = self.dbg.GetCommandInterpreter()
         result = lldb.SBCommandReturnObject()
         interp.ResolveCommand(substitute_cmd, result)
         self.assertTrue(result.Succeeded(), result.GetError())
 
         resolved_cmd = result.GetOutput()
         if actual_cmd == "frame variable":
             resolved_cmd = resolved_cmd.replace(" -- ", " ", 1)
 
-        expected_output = self._run_cmd(resolved_cmd)
+        resolved_cmd_output = self._run_cmd(resolved_cmd)
+        dwim_cmd_output = self._strip_result_var(resolved_cmd_output)
 
         # Verify dwim-print chose the expected command.
         self.runCmd("settings set dwim-print-verbosity full")
-        substrs = [f"note: ran `{resolved_cmd}`"]
-        patterns = []
-
-        if self.VAR_IDENT.search(expected_output):
-            patterns.append(self._mask_persistent_var(expected_output))
-        else:
-            substrs.append(expected_output)
 
-        self.expect(dwim_cmd, substrs=substrs, patterns=patterns)
+        self.expect(dwim_cmd, substrs=[
+            f"note: ran `{resolved_cmd}`",
+            dwim_cmd_output,
+        ])
 
     def test_variables(self):
         """Test dwim-print with variables."""
         self.build()
         lldbutil.run_to_name_breakpoint(self, "main")
         vars = ("argc", "argv")
         for var in vars:
             self._expect_cmd(f"dwim-print {var}", "frame variable")