[lldb] Add --gdb-format flag to dwim-print

[kastiglione]

Summary

Add support for the --gdb-format/-G flag to dwim-print.

The gdb-format flag enables better compatibility with p and v, because it
allows /x and the other gdb-format specifiers.

[jingham]

I don't think this is quite right.

First off, it's a little weird that your dwim-print command only supports the gdb-format option and not the format option? In the long run, I think you need to support both. Being able to say v -fA variable but not dwim-print -fA would be wrong. But given this is early days, that can be a separate patch.

More importantly, your translation is lossy. The gdb format option supports count & size as well as format: e.g. -G 32xb. But the current code will turn that into -G x for the underlying command. So you either need to reconstitute the gdb-format string from the m_count and m_byte_size as well as m_format, or get the OptionGroupFormat to store the unparsed gdb format string and just hand that back to you.

[jingham]

lldb/source/Commands/CommandObjectDWIMPrint.cpp

63

Is this your long term plan? Even in a dwim print command, I will want to control the depth of structure/pointer traversal, and whether I see the raw or synthetic child version of the result, etc.

[kastiglione]

@jingham The brief answer is that decisions have been based on compatibility with the behavior of p.

1. expression (also p) and frame variable, only support the the format part of gdb options. Neither support --count or --size and so don't support the equivalent gdb options.
2. p takes no flags (other than the gdb-format), and dwim-print matches that behavior.

Taking the example of dwim-print -fA variable, if the user were to do the same but with an expression, they'd have to write dwim-print -fA -- expression. Will there be users who know and want the printing options you mentioned (synthetic/raw, depth, etc) but who try to use those with dwim-print instead of directly using v or e?

My expectation has been that dwim-print would (1) generally not be used directly, but be via an alias (either p or another choice) and that (2) for compatibility, the alias would be dwim-print --.

Thoughts?

[jingham]

In D141425#4044280, @kastiglione wrote:

@jingham The brief answer is that decisions have been based on compatibility with the behavior of p.

1. expression (also p) and frame variable, only support the the format part of gdb options. Neither support --count or --size and so don't support the equivalent gdb options.
2. p takes no flags (other than the gdb-format), and dwim-print matches that behavior.

Taking the example of dwim-print -fA variable, if the user were to do the same but with an expression, they'd have to write dwim-print -fA -- expression. Will there be users who know and want the printing options you mentioned (synthetic/raw, depth, etc) but who try to use those with dwim-print instead of directly using v or e?

My expectation has been that dwim-print would (1) generally not be used directly, but be via an alias (either p or another choice) and that (2) for compatibility, the alias would be dwim-print --.

Thoughts?

1. I guess that sort of makes sense - since the results of expr & frame var are always typed, specifying sizes and counts doesn't make sense. Those fields only make sense for memory read. It makes the rationale for using gdb-format in the / accelerator a little less well motivated, but that's not on you...

2. To me the analogy between p and dwim-print is not the right one. dwim-print is parallel with expr: both are full featured commands from the standpoint of configuring the output printing. p is the alias that we offer for user-convenience - it picks the defaults for the presentation. This separation is nice because then if a user has a different set of default printing expectations (they always want to print pointers one level deep or something) they can re-alias p to fit their needs, or even make two variants that serve different purposes.

So I was expecting at first we'd add a dp or something alias, that was dwim-print -- and in the fullness of time we would switch to it as the "first choice expression printer" by replacing the p -> expr -- alias with p->dwim-print --, thus getting the same convenience AND configurability we got with expr...

[kastiglione]

Add display options to dwim-print command

[kastiglione]

Add test with standard lldb flags

[kastiglione]

remove unnecessary header addition

[kastiglione]

Add a test for display options too (-T)

[kastiglione]

replace raise with assertTrue

[jingham]

LGTM

lldb/include/lldb/Interpreter/OptionGroupFormat.h

public:
   const OptionValueUInt64 &GetByteSizeValue() const { return m_byte_size; }
 
   OptionValueUInt64 &GetCountValue() { return m_count; }
 
   const OptionValueUInt64 &GetCountValue() const { return m_count; }
 
   bool HasGDBFormat() const { return m_has_gdb_format; }
 
+  /// Return the GDB format letter that identifies this format.
+  /// Valid only when `HasGDBFormat` is true.
+  char GetGDBFormat() const;
+
   bool AnyOptionWasSet() const {
     return m_format.OptionWasSet() || m_byte_size.OptionWasSet() ||
            m_count.OptionWasSet();
   }
 
 protected:
   bool ParserGDBFormatLetter(ExecutionContext *execution_context,
                              char format_letter, lldb::Format &format,

lldb/source/Commands/CommandObjectDWIMPrint.h

 //===-- CommandObjectDWIMPrint.h --------------------------------*- 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_SOURCE_COMMANDS_COMMANDOBJECTDWIMPRINT_H
 #define LLDB_SOURCE_COMMANDS_COMMANDOBJECTDWIMPRINT_H
 
 #include "lldb/Interpreter/CommandObject.h"
+#include "lldb/Interpreter/OptionGroupFormat.h"
+#include "lldb/Interpreter/OptionGroupValueObjectDisplay.h"
+#include "lldb/Interpreter/OptionValueFormat.h"
 
 namespace lldb_private {
 
 /// Implements `dwim-print`, a printing command that chooses the most direct,
 /// efficient, and resilient means of printing a given expression.
 ///
 /// DWIM is an acronym for Do What I Mean. From Wikipedia, DWIM is described as:
 ///
 ///   > attempt to anticipate what users intend to do, correcting trivial errors
 ///   > automatically rather than blindly executing users' explicit but
 ///   > potentially incorrect input
 ///
 /// The `dwim-print` command serves as a single print command for users who
 /// don't yet know, or perfer not to know, the various lldb commands that can be
 /// used to print, and when to use them.
 class CommandObjectDWIMPrint : public CommandObjectRaw {
 public:
   CommandObjectDWIMPrint(CommandInterpreter &interpreter);
 
   ~CommandObjectDWIMPrint() override = default;
 
+  Options *GetOptions() override;
+
 private:
   bool DoExecute(llvm::StringRef command, CommandReturnObject &result) override;
+
+  OptionGroupOptions m_option_group;
+  OptionGroupFormat m_format_options = lldb::eFormatDefault;
+  OptionGroupValueObjectDisplay m_varobj_options;
 };
 
 } // namespace lldb_private
 
 #endif

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/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"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-forward.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FormatVariadic.h"
 
 using namespace llvm;
 using namespace lldb;
 using namespace lldb_private;
 
 CommandObjectDWIMPrint::CommandObjectDWIMPrint(CommandInterpreter &interpreter)
     : CommandObjectRaw(interpreter, "dwim-print",
                        "Print a variable or expression.",
                        "dwim-print [<variable-name> | <expression>]",
                        eCommandProcessMustBePaused | eCommandTryTargetAPILock) {
+  m_option_group.Append(&m_format_options,
+                        OptionGroupFormat::OPTION_GROUP_FORMAT |
+                            OptionGroupFormat::OPTION_GROUP_GDB_FMT,
+                        LLDB_OPT_SET_1);
+  m_option_group.Append(&m_varobj_options, LLDB_OPT_SET_ALL, LLDB_OPT_SET_1);
+  m_option_group.Finalize();
 }
 
-bool CommandObjectDWIMPrint::DoExecute(StringRef expr,
-                                       CommandReturnObject &result) {
-  // Ignore leading and trailing whitespace.
-  expr = expr.trim();
+Options *CommandObjectDWIMPrint::GetOptions() { return &m_option_group; }
 
-  if (expr.empty()) {
+bool CommandObjectDWIMPrint::DoExecute(StringRef command,
+                                       CommandReturnObject &result) {
+  m_option_group.NotifyOptionParsingStarting(&m_exe_ctx);
+  OptionsWithRaw args{command};
+  StringRef expr = args.GetRawPart();
+
+  if (args.HasArgs()) {
+    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;
   }
 
   auto verbosity = GetDebugger().GetDWIMPrintVerbosity();
 
+  DumpValueObjectOptions dump_options = m_varobj_options.GetAsDumpOptions(
+      eLanguageRuntimeDescriptionDisplayVerbosityFull,
+      m_format_options.GetFormat());
+
+  std::string format_flag;
+  if (m_format_options.HasGDBFormat())
+    format_flag = llvm::formatv(" -G {0}", m_format_options.GetGDBFormat());
+
   // 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 (verbosity == eDWIMPrintVerbosityFull)
-        result.AppendMessageWithFormatv("note: ran `frame variable {0}`", expr);
-      valobj_sp->Dump(result.GetOutputStream());
+        result.AppendMessageWithFormatv("note: ran `frame variable{0} {1}`",
+                                        format_flag, expr);
+      valobj_sp->Dump(result.GetOutputStream(), dump_options);
       result.SetStatus(eReturnStatusSuccessFinishResult);
       return true;
     }
   }
 
   // Second, also lastly, try `expr` as a source expression to evaluate.
   {
     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();
 
     auto *exe_scope = m_exe_ctx.GetBestExecutionContextScope();
     ValueObjectSP valobj_sp;
     if (target.EvaluateExpression(expr, exe_scope, valobj_sp) ==
         eExpressionCompleted) {
       if (verbosity != eDWIMPrintVerbosityNone)
-        result.AppendMessageWithFormatv("note: ran `expression -- {0}`", expr);
-      valobj_sp->Dump(result.GetOutputStream());
+        result.AppendMessageWithFormatv("note: ran `expression{0} -- {1}`",
+                                        format_flag, 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(
             "unknown error evaluating expression `{0}`", expr);
       return false;
     }
   }
 }

lldb/source/Interpreter/OptionGroupFormat.cpp

 //===-- OptionGroupFormat.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 "lldb/Interpreter/OptionGroupFormat.h"
 
 #include "lldb/Host/OptionParser.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Target/ExecutionContext.h"
 #include "lldb/Target/Target.h"
+#include "lldb/lldb-enumerations.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 static constexpr OptionDefinition g_default_option_definitions[] = {
     {LLDB_OPT_SET_1, false, "format", 'f', OptionParser::eRequiredArgument,
      nullptr, {}, 0, eArgTypeFormat,
      "Specify a format to be used for display."},
bool OptionGroupFormat::ParserGDBFormatLetter(
   default:
     break;
   }
 
 
   return false;
 }
 
+char OptionGroupFormat::GetGDBFormat() const {
+  switch (GetFormat()) {
+  case lldb::eFormatOctal:
+    return 'o';
+  case lldb::eFormatHex:
+    return 'x';
+  case lldb::eFormatDecimal:
+    return 'd';
+  case lldb::eFormatUnsigned:
+    return 'u';
+  case lldb::eFormatBinary:
+    return 't';
+  case lldb::eFormatFloat:
+    return 'f';
+  case lldb::eFormatAddressInfo:
+    return 'a';
+  case lldb::eFormatInstruction:
+    return 'i';
+  case lldb::eFormatChar:
+    return 'c';
+  case lldb::eFormatCString:
+    return 's';
+  case lldb::eFormatOSType:
+    return 'T';
+  case lldb::eFormatHexFloat:
+    return 'A';
+  default:
+    llvm_unreachable("format is not a valid gdb-format");
+  }
+}
+
 void OptionGroupFormat::OptionParsingStarting(
     ExecutionContext *execution_context) {
   m_format.Clear();
   m_byte_size.Clear();
   m_count.Clear();
   m_has_gdb_format = false;
 }

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

def _mask_persistent_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.
         """
         before, after = self.PERSISTENT_VAR.split(string, maxsplit=1)
         return re.escape(before) + r"\$\d+" + re.escape(after)
 
-    def _expect_cmd(self, expr: str, base_cmd: str) -> None:
+    def _expect_cmd(
+        self,
+        dwim_cmd: str,
+        base_cmd: str,
+    ) -> None:
         """Run dwim-print and verify the output against the expected command."""
-        cmd = f"{base_cmd} {expr}"
+        match = re.match("dwim-print(?:/(.))? (.+)", dwim_cmd)
+        format = match.group(1)
+        expr = match.group(2)
+
+        cmd_args = []
+        if format:
+            cmd_args.append(f"-G {format}")
+        if base_cmd == "expression":
+            cmd_args.append("--")
+        cmd_args.append(expr)
+
+        cmd_argstr = " ".join(cmd_args)
+        cmd = f"{base_cmd} {cmd_argstr}"
         cmd_output = self._run_cmd(cmd)
 
         # Verify dwim-print chose the expected command.
         self.runCmd("settings set dwim-print-verbosity full")
         substrs = [f"note: ran `{cmd}`"]
         patterns = []
 
-        if base_cmd == "expression --" and self.PERSISTENT_VAR.search(cmd_output):
+        if base_cmd == "expression" and self.PERSISTENT_VAR.search(cmd_output):
             patterns.append(self._mask_persistent_var(cmd_output))
         else:
             substrs.append(cmd_output)
 
-        self.expect(f"dwim-print {expr}", substrs=substrs, patterns=patterns)
+        self.expect(dwim_cmd, substrs=substrs, patterns=patterns)
 
     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(var, "frame variable")
+            self._expect_cmd(f"dwim-print {var}", "frame variable")
 
     def test_variable_paths(self):
         """Test dwim-print with variable path expressions."""
         self.build()
         lldbutil.run_to_name_breakpoint(self, "main")
         exprs = ("&argc", "*argv", "argv[0]")
         for expr in exprs:
-            self._expect_cmd(expr, "expression --")
+            self._expect_cmd(f"dwim-print {expr}", "expression")
 
     def test_expressions(self):
         """Test dwim-print with expressions."""
         self.build()
         lldbutil.run_to_name_breakpoint(self, "main")
         exprs = ("argc + 1", "(void)argc", "(int)abs(argc)")
         for expr in exprs:
-            self._expect_cmd(expr, "expression --")
+            self._expect_cmd(f"dwim-print {expr}", "expression")
 
     def test_dummy_target_expressions(self):
         """Test dwim-print's ability to evaluate expressions without a target."""
-        self._expect_cmd("1 + 2", "expression --")
+        self._expect_cmd("dwim-print 1 + 2", "expression")
+
+    def test_gdb_format(self):
+        self.build()
+        lldbutil.run_to_name_breakpoint(self, "main")
+        self._expect_cmd(f"dwim-print/x argc", "frame variable")
+        self._expect_cmd(f"dwim-print/x argc + 1", "expression")