[Bug 49018][lldb] Fix incorrect help text for 'memory write' command

[RamNalamothu]

Summary

Certain commands like 'memory write', 'register read' etc all use
the OptionGroupFormat options but the help usage text for those
options is not customized to those commands.

One such example is:

  (lldb) help memory read
           -s <byte-size> ( --size <byte-size> )
               The size in bytes to use when displaying with the selected format.
  (lldb) help memory write
	   -s <byte-size> ( --size <byte-size> )
               The size in bytes to use when displaying with the selected format.

This patch allows such commands to overwrite the help text for the options
in the OptionGroupFormat group as needed and fixes help text of memory write.

llvm.org/pr49018.

[DavidSpickett]

Thanks for looking at this bug! I admit I don't know the option parsing very well which is why I shied away from fixing it myself.

Some good finds elsewhere too, but better as their own changes I think. (feel free to add me on review there too)

lldb/include/lldb/Interpreter/OptionGroupFormat.h

37

Would be good to add a comment here, something like:

// The default help text is written for "memory read", this can be used to override that

84

If the message is a const char* do we need to store this in a std::string? I'm, perhaps naively, assuming that you'll only do this override once. Or that if you did it again in a subclassed command, it would replace the whole text.

lldb/source/Commands/CommandObjectMemory.cpp

1177

Can you do this like this? Saves writing push_back a few times.

return { std::make_tuple<...>, ... };

Or you could construct this directly in the constructor parameters below. I'm not sure you need a new function for it.
(depends what clang-format makes of it I guess, if it's unreadable then sure have a utility function)

1251

Which part of this change does this contribute to?

1295

Great job spotting this, but I'd rather it was in its own change with its own test.

lldb/source/Interpreter/OptionGroupFormat.cpp

73

Not sure if clang-format has done this, or just your preferred style. Nothing against either but it makes it difficult to tell if anything has changed in this particular part of the diff.

In general we want clang-formatted code but if it makes the diff tricky to read it's best done in a follow up change that just does formatting.

80

As I said above, if this is about commands being able to *replace* the help text I don't think append is needed here.

Unless std::string is helping you check whether it's been set or not, which I think you could do with nullptr just as well but please correct me if I'm missing some context.

lldb/test/API/commands/help/TestHelp.py

246

If these memory write <options> lines are checking for the other issue you found, I'd put those in their own test.

[DavidSpickett]

Not sure if you're a regular contributor and already know this but in case not, I tend to use https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting for doing clang-format stuff. It'll format only the lines you've changed.

[RamNalamothu]

In D114448#3150900, @DavidSpickett wrote:

Not sure if you're a regular contributor and already know this but in case not, I tend to use https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting for doing clang-format stuff. It'll format only the lines you've changed.

Thanks for sharing that. I normally use git-clang-format (i.e. clang-format extension for git) which does the same job.

lldb/source/Interpreter/OptionGroupFormat.cpp

73

The clang-format did it. Will mark this with clang-format off/on.

80

I kind of took the std::string approach from OptionGroupPythonClassWithDict but as you mentioned it is not needed and will fix that.

[RamNalamothu]

Address review comments.

[RamNalamothu]

Remove the unnecessary if {} gaurding around range for loop.

[DavidSpickett]

Thanks for splitting the patches.

lldb/source/Interpreter/OptionGroupFormat.cpp

23

You could use a std::copy for this and not need to spell out each index.

73

My bad, what I meant was that it's fine to format it in general but that in this case it should be avoided to make the diff more readable.

But now I see that this is essentially a new function so formatting it all is fine. I now see the change you're making, the body of the array/function isn't actually changing. You can remove the /* clang-format...*/ and just let it do its thing.

That said, what is the reason that you couldn't re-use the option table?

I have two thoughts about the function:

1. Do we need to remake the array each time, vs the constexpr array from before.
2. We're returning an ArrayRef to a return value, which probably works in some situations (if you immediately do a copy using the ref, then discard it) but I think it's a lifetime issue overall.

I'm assuming that ArrayRef is basically std::array& in a general sense, and if you returned a std::array& to something on the stack, that's a lifetime issue for sure. I looked at some other option groups and they all follow the pattern of returning an array ref to a const array defined outside the function.

[RamNalamothu]

lldb/source/Interpreter/OptionGroupFormat.cpp

73

Ah, I could re-use the option table. Just an oversite from my end. Thank you for spotting that.

[RamNalamothu]

Re-use the existing constexpr option table.

[DavidSpickett]

LGTM

lldb/include/lldb/Interpreter/OptionGroupFormat.h

 
 #include "lldb/Interpreter/OptionValueFormat.h"
 #include "lldb/Interpreter/OptionValueSInt64.h"
 #include "lldb/Interpreter/OptionValueUInt64.h"
 #include "lldb/Interpreter/Options.h"
 
 namespace lldb_private {
 
+typedef std::vector<std::tuple<lldb::CommandArgumentType, const char *>>
+    OptionGroupFormatUsageTextVector;
+
 // OptionGroupFormat
 
 class OptionGroupFormat : public OptionGroup {
 public:
   static const uint32_t OPTION_GROUP_FORMAT = LLDB_OPT_SET_1;
   static const uint32_t OPTION_GROUP_GDB_FMT = LLDB_OPT_SET_2;
   static const uint32_t OPTION_GROUP_SIZE = LLDB_OPT_SET_3;
   static const uint32_t OPTION_GROUP_COUNT = LLDB_OPT_SET_4;
 
   OptionGroupFormat(
       lldb::Format default_format,
       uint64_t default_byte_size =
           UINT64_MAX, // Pass UINT64_MAX to disable the "--size" option
       uint64_t default_count =
-          UINT64_MAX); // Pass UINT64_MAX to disable the "--count" option
+          UINT64_MAX, // Pass UINT64_MAX to disable the "--count" option
+      OptionGroupFormatUsageTextVector usage_text_vector = {}
+      // Use to override default option usage text with the command specific one
+  );
 
   ~OptionGroupFormat() override = default;
 
   llvm::ArrayRef<OptionDefinition> GetDefinitions() override;
 
   Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_value,
                         ExecutionContext *execution_context) override;
 
bool ParserGDBFormatLetter(ExecutionContext *execution_context,
                              uint32_t &byte_size);
 
   OptionValueFormat m_format;
   OptionValueUInt64 m_byte_size;
   OptionValueUInt64 m_count;
   char m_prev_gdb_format;
   char m_prev_gdb_size;
   bool m_has_gdb_format;
+  OptionDefinition m_option_definitions[4];
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_INTERPRETER_OPTIONGROUPFORMAT_H

lldb/source/Commands/CommandObjectMemory.cpp

 class CommandObjectMemoryWrite : public CommandObjectParsed {
 public:
   class OptionGroupWriteMemory : public OptionGroup {
   public:
     OptionGroupWriteMemory() : OptionGroup() {}
 
     ~OptionGroupWriteMemory() override = default;
 
     llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
       return llvm::makeArrayRef(g_memory_write_options);
     }
 
     Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_value,
                           ExecutionContext *execution_context) override {
       Status error;
       const int short_option = g_memory_write_options[option_idx].short_option;
 
public:
     off_t m_infile_offset;
   };
 
   CommandObjectMemoryWrite(CommandInterpreter &interpreter)
       : CommandObjectParsed(
             interpreter, "memory write",
             "Write to the memory of the current target process.", nullptr,
             eCommandRequiresProcess | eCommandProcessMustBeLaunched),
-        m_option_group(), m_format_options(eFormatBytes, 1, UINT64_MAX),
+        m_option_group(),
+        m_format_options(
+            eFormatBytes, 1, UINT64_MAX,
+            {std::make_tuple(
+                 eArgTypeFormat,
+                 "The format to use for each of the value to be written."),
+             std::make_tuple(
+                 eArgTypeByteSize,
+                 "The size in bytes to write from input file or each value.")}),
         m_memory_options() {
     CommandArgumentEntry arg1;
     CommandArgumentEntry arg2;
     CommandArgumentData addr_arg;
     CommandArgumentData value_arg;
 
     // Define the first (and only) variant of this arg.
     addr_arg.arg_type = eArgTypeAddress;
     addr_arg.arg_repetition = eArgRepeatPlain;
 
     // There is only one variant this argument could be; put it into the
     // argument entry.
     arg1.push_back(addr_arg);
 
     // Define the first (and only) variant of this arg.
     value_arg.arg_type = eArgTypeValue;
     value_arg.arg_repetition = eArgRepeatPlus;
     value_arg.arg_opt_set_association = LLDB_OPT_SET_1;
 
     // There is only one variant this argument could be; put it into the
     // argument entry.
     arg2.push_back(value_arg);
 
     // Push the data for the first argument into the m_arguments vector.
     m_arguments.push_back(arg1);
     m_arguments.push_back(arg2);
if (m_memory_options.m_infile) {
             m_cmd_name.c_str());
         return false;
       }
       if (argc > 1) {
         result.AppendErrorWithFormat(
             "%s takes only a destination address when writing file contents.\n",
             m_cmd_name.c_str());
         return false;
       }
     } else if (argc < 2) {
       result.AppendErrorWithFormat(
           "%s takes a destination address and at least one value.\n",
           m_cmd_name.c_str());
       return false;
     }
 
     StreamString buffer(

lldb/source/Interpreter/OptionGroupFormat.cpp

 #include "lldb/Host/OptionParser.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Target/ExecutionContext.h"
 #include "lldb/Target/Target.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
-OptionGroupFormat::OptionGroupFormat(lldb::Format default_format,
-                                     uint64_t default_byte_size,
-                                     uint64_t default_count)
-    : m_format(default_format, default_format),
-      m_byte_size(default_byte_size, default_byte_size),
-      m_count(default_count, default_count), m_prev_gdb_format('x'),
-      m_prev_gdb_size('w') {}
-
-static constexpr OptionDefinition g_option_table[] = {
+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."},
     {LLDB_OPT_SET_2, false, "gdb-format", 'G', OptionParser::eRequiredArgument,
      nullptr, {}, 0, eArgTypeGDBFormat,
      "Specify a format using a GDB format specifier string."},
     {LLDB_OPT_SET_3, false, "size", 's', OptionParser::eRequiredArgument,
      nullptr, {}, 0, eArgTypeByteSize,
      "The size in bytes to use when displaying with the selected format."},
     {LLDB_OPT_SET_4, false, "count", 'c', OptionParser::eRequiredArgument,
      nullptr, {}, 0, eArgTypeCount,
      "The number of total items to display."},
 };
 
+OptionGroupFormat::OptionGroupFormat(
+    lldb::Format default_format, uint64_t default_byte_size,
+    uint64_t default_count, OptionGroupFormatUsageTextVector usage_text_vector)
+    : m_format(default_format, default_format),
+      m_byte_size(default_byte_size, default_byte_size),
+      m_count(default_count, default_count), m_prev_gdb_format('x'),
+      m_prev_gdb_size('w') {
+  // Copy the default option definitions.
+  std::copy(std::begin(g_default_option_definitions),
+            std::end(g_default_option_definitions),
+            std::begin(m_option_definitions));
+
+  for (auto usage_text_tuple : usage_text_vector) {
+    switch (std::get<0>(usage_text_tuple)) {
+    case eArgTypeFormat:
+      m_option_definitions[0].usage_text = std::get<1>(usage_text_tuple);
+      break;
+    case eArgTypeByteSize:
+      m_option_definitions[2].usage_text = std::get<1>(usage_text_tuple);
+      break;
+    default:
+      llvm_unreachable("Unimplemented option");
+    }
+  }
+}
+
 llvm::ArrayRef<OptionDefinition> OptionGroupFormat::GetDefinitions() {
-  auto result = llvm::makeArrayRef(g_option_table);
+  auto result = llvm::makeArrayRef(m_option_definitions);
   if (m_byte_size.GetDefaultValue() < UINT64_MAX) {
     if (m_count.GetDefaultValue() < UINT64_MAX)
       return result;
     else
       return result.take_front(3);
   }
   return result.take_front(2);
 }
 
 Status OptionGroupFormat::SetOptionValue(uint32_t option_idx,
                                          llvm::StringRef option_arg,
                                          ExecutionContext *execution_context) {
   Status error;
-  const int short_option = g_option_table[option_idx].short_option;
+  const int short_option = m_option_definitions[option_idx].short_option;
 
   switch (short_option) {
   case 'f':
     error = m_format.SetValueFromString(option_arg);
     break;
 
   case 'c':
     if (m_count.GetDefaultValue() == 0) {
       error.SetErrorString("--count option is disabled");
     } else {
       error = m_count.SetValueFromString(option_arg);
       if (m_count.GetCurrentValue() == 0)
         error.SetErrorStringWithFormat("invalid --count option value '%s'",

lldb/test/API/commands/help/TestHelp.py

class HelpCommandTestCase(TestBase):
     def test_help_format_output(self):
         """Test that help output reaches TerminalWidth."""
         self.runCmd(
             'settings set term-width 108')
         self.expect(
             "help format",
             matching=True,
             substrs=['<format> -- One of the format names'])
+
+    @no_debug_info_test
+    def test_help_option_group_format_options_usage(self):
+        """Test that help on commands that use OptionGroupFormat options provide relevant help specific to that command."""
+        self.expect(
+            "help memory read",
+            matching=True,
+            substrs=[
+                "-f <format> ( --format <format> )", "Specify a format to be used for display.",
+                "-s <byte-size> ( --size <byte-size> )", "The size in bytes to use when displaying with the selected format."])
+
+        self.expect(
+            "help memory write",
+            matching=True,
+            substrs=[
+                "-f <format> ( --format <format> )", "The format to use for each of the value to be written.",
+                "-s <byte-size> ( --size <byte-size> )", "The size in bytes to write from input file or each value."])
+