[lldb] Change implementation of memory read --show-tags option

[DavidSpickett]

Summary

This does 2 things:

• Moves it after the short options. Which makes sense given it's a niche, default off option. (if 2 files for one option seems a bit much, I am going to reuse them for "memory find" later)
• Fixes the use of repeated commands. For example: memory read buf --show-tags <shows tags> memory read <shows tags>

Added tests for the repetition and updated existing help tests.

[DavidSpickett]

I did try to add the option via Options.td but adding a multi character short option there doesn't seem to work. I could enable that but it seems like just as much work to make an option group for it.

lldb/include/lldb/Interpreter/OptionGroupMemoryTag.h

+//===-- OptionGroupMemoryTag.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_INTERPRETER_OPTIONGROUPMEMORYTAG_H
+#define LLDB_INTERPRETER_OPTIONGROUPMEMORYTAG_H
+
+#include "lldb/Interpreter/OptionValueBoolean.h"
+#include "lldb/Interpreter/Options.h"
+
+namespace lldb_private {
+
+class OptionGroupMemoryTag : public OptionGroup {
+public:
+  OptionGroupMemoryTag();
+
+  ~OptionGroupMemoryTag() override = default;
+
+  llvm::ArrayRef<OptionDefinition> GetDefinitions() override;
+
+  Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_value,
+                        ExecutionContext *execution_context) override;
+
+  void OptionParsingStarting(ExecutionContext *execution_context) override;
+
+  bool AnyOptionWasSet() const { return m_show_tags.OptionWasSet(); }
+
+  OptionValueBoolean GetShowTags() { return m_show_tags; };
+
+protected:
+  OptionValueBoolean m_show_tags;
+};
+
+} // namespace lldb_private
+
+#endif // LLDB_INTERPRETER_OPTIONGROUPMEMORYTAG_H

lldb/source/Commands/CommandObjectMemory.cpp

 #include "lldb/Core/DumpDataExtractor.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Core/ValueObjectMemory.h"
 #include "lldb/Expression/ExpressionVariable.h"
 #include "lldb/Host/OptionParser.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "lldb/Interpreter/OptionArgParser.h"
 #include "lldb/Interpreter/OptionGroupFormat.h"
+#include "lldb/Interpreter/OptionGroupMemoryTag.h"
 #include "lldb/Interpreter/OptionGroupOutputFile.h"
 #include "lldb/Interpreter/OptionGroupValueObjectDisplay.h"
 #include "lldb/Interpreter/OptionValueLanguage.h"
 #include "lldb/Interpreter/OptionValueString.h"
 #include "lldb/Interpreter/Options.h"
 #include "lldb/Symbol/SymbolFile.h"
 #include "lldb/Symbol/TypeList.h"
 #include "lldb/Target/ABI.h"
Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_value,
     case 'x':
       error = m_language_for_type.SetValueFromString(option_value);
       break;
 
     case 'E':
       error = m_offset.SetValueFromString(option_value);
       break;
 
-    case '\x01':
-      m_show_tags = true;
-      break;
-
     default:
       llvm_unreachable("Unimplemented option");
     }
     return error;
   }
 
   void OptionParsingStarting(ExecutionContext *execution_context) override {
     m_num_per_line.Clear();
     m_output_as_binary = false;
     m_view_as_type.Clear();
     m_force = false;
     m_offset.Clear();
     m_language_for_type.Clear();
-    m_show_tags = false;
   }
 
   Status FinalizeSettings(Target *target, OptionGroupFormat &format_options) {
     Status error;
     OptionValueUInt64 &byte_size_value = format_options.GetByteSizeValue();
     OptionValueUInt64 &count_value = format_options.GetCountValue();
     const bool byte_size_option_set = byte_size_value.OptionWasSet();
     const bool num_per_line_option_set = m_num_per_line.OptionWasSet();
public:
   }
 
   OptionValueUInt64 m_num_per_line;
   bool m_output_as_binary = false;
   OptionValueString m_view_as_type;
   bool m_force;
   OptionValueUInt64 m_offset;
   OptionValueLanguage m_language_for_type;
-  bool m_show_tags = false;
 };
 
 // Read memory from the inferior process
 class CommandObjectMemoryRead : public CommandObjectParsed {
 public:
   CommandObjectMemoryRead(CommandInterpreter &interpreter)
       : CommandObjectParsed(
             interpreter, "memory read",
CommandObjectMemoryRead(CommandInterpreter &interpreter)
     // Add the "--size" option to group 1 and 2
     m_option_group.Append(&m_format_options,
                           OptionGroupFormat::OPTION_GROUP_SIZE,
                           LLDB_OPT_SET_1 | LLDB_OPT_SET_2);
     m_option_group.Append(&m_memory_options);
     m_option_group.Append(&m_outfile_options, LLDB_OPT_SET_ALL,
                           LLDB_OPT_SET_1 | LLDB_OPT_SET_2 | LLDB_OPT_SET_3);
     m_option_group.Append(&m_varobj_options, LLDB_OPT_SET_ALL, LLDB_OPT_SET_3);
+    m_option_group.Append(&m_memory_tag_options, LLDB_OPT_SET_ALL,
+                          LLDB_OPT_SET_ALL);
     m_option_group.Finalize();
   }
 
   ~CommandObjectMemoryRead() override = default;
 
   Options *GetOptions() override { return &m_option_group; }
 
   llvm::Optional<std::string> GetRepeatCommand(Args &current_command_args,
if (argc == 0) {
       // Use the last address and byte size and all options as they were if no
       // options have been set
       addr = m_next_addr;
       total_byte_size = m_prev_byte_size;
       compiler_type = m_prev_compiler_type;
       if (!m_format_options.AnyOptionWasSet() &&
           !m_memory_options.AnyOptionWasSet() &&
           !m_outfile_options.AnyOptionWasSet() &&
-          !m_varobj_options.AnyOptionWasSet()) {
+          !m_varobj_options.AnyOptionWasSet() &&
+          !m_memory_tag_options.AnyOptionWasSet()) {
         m_format_options = m_prev_format_options;
         m_memory_options = m_prev_memory_options;
         m_outfile_options = m_prev_outfile_options;
         m_varobj_options = m_prev_varobj_options;
+        m_memory_tag_options = m_prev_memory_tag_options;
       }
     }
 
     size_t item_count = m_format_options.GetCountValue().GetCurrentValue();
 
     // TODO For non-8-bit byte addressable architectures this needs to be
     // revisited to fully support all lldb's range of formatting options.
     // Furthermore code memory reads (for those architectures) will not be
#undef ALL_KEYWORDS
     }
 
     m_next_addr = addr + bytes_read;
     m_prev_byte_size = bytes_read;
     m_prev_format_options = m_format_options;
     m_prev_memory_options = m_memory_options;
     m_prev_outfile_options = m_outfile_options;
     m_prev_varobj_options = m_varobj_options;
+    m_prev_memory_tag_options = m_memory_tag_options;
     m_prev_compiler_type = compiler_type;
 
     std::unique_ptr<Stream> output_stream_storage;
     Stream *output_stream_p = nullptr;
     const FileSpec &outfile_spec =
         m_outfile_options.GetFile().GetCurrentValue();
 
     std::string path = outfile_spec.GetPath();
if (((format == eFormatChar) || (format == eFormatCharPrintable)) &&
         return false;
       }
     }
 
     assert(output_stream_p);
     size_t bytes_dumped = DumpDataExtractor(
         data, output_stream_p, 0, format, item_byte_size, item_count,
         num_per_line / target->GetArchitecture().GetDataByteSize(), addr, 0, 0,
-        exe_scope, m_memory_options.m_show_tags);
+        exe_scope, m_memory_tag_options.GetShowTags().GetCurrentValue());
     m_next_addr = addr + bytes_dumped;
     output_stream_p->EOL();
     return true;
   }
 
   OptionGroupOptions m_option_group;
   OptionGroupFormat m_format_options;
   OptionGroupReadMemory m_memory_options;
   OptionGroupOutputFile m_outfile_options;
   OptionGroupValueObjectDisplay m_varobj_options;
+  OptionGroupMemoryTag m_memory_tag_options;
   lldb::addr_t m_next_addr = LLDB_INVALID_ADDRESS;
   lldb::addr_t m_prev_byte_size = 0;
   OptionGroupFormat m_prev_format_options;
   OptionGroupReadMemory m_prev_memory_options;
   OptionGroupOutputFile m_prev_outfile_options;
   OptionGroupValueObjectDisplay m_prev_varobj_options;
+  OptionGroupMemoryTag m_prev_memory_tag_options;
   CompilerType m_prev_compiler_type;
 };
 
 #define LLDB_OPTIONS_memory_find
 #include "CommandOptions.inc"
 
 // Find the specified data in memory
 class CommandObjectMemoryFind : public CommandObjectParsed {

lldb/source/Commands/Options.td

def memory_read_type : Option<"type", "t">, Groups<[3,4]>, Arg<"Name">,
     Required, Desc<"The name of a type to view memory as.">;
   def memory_read_language : Option<"language", "x">, Group<4>, Arg<"Language">,
     Desc<"The language of the type to view memory as.">;
   def memory_read_offset : Option<"offset", "E">, Group<3>, Arg<"Count">,
     Desc<"How many elements of the specified type to skip before starting to "
     "display data.">;
   def memory_read_force : Option<"force", "r">, Groups<[1,2,3]>,
     Desc<"Necessary if reading over target.max-memory-read-size bytes.">;
-  def memory_read_show_tags : Option<"show-tags", "\\x01">, Group<1>,
-    Desc<"Include memory tags in output (does not apply to binary output).">;
 }
 
 let Command = "memory find" in {
   def memory_find_expression : Option<"expression", "e">, Group<1>,
     Arg<"Expression">, Required,
     Desc<"Evaluate an expression to obtain a byte pattern.">;
   def memory_find_string : Option<"string", "s">, Group<2>, Arg<"Name">,
     Required, Desc<"Use text to find a byte pattern.">;

lldb/source/Interpreter/CMakeLists.txt

add_lldb_library(lldbInterpreter
   CommandObject.cpp
   CommandOptionValidators.cpp
   CommandReturnObject.cpp
   OptionArgParser.cpp
   OptionGroupArchitecture.cpp
   OptionGroupBoolean.cpp
   OptionGroupFile.cpp
   OptionGroupFormat.cpp
+  OptionGroupMemoryTag.cpp
   OptionGroupPythonClassWithDict.cpp
   OptionGroupOutputFile.cpp
   OptionGroupPlatform.cpp
   OptionGroupString.cpp
   OptionGroupUInt64.cpp
   OptionGroupUUID.cpp
   OptionGroupValueObjectDisplay.cpp
   OptionValue.cpp

lldb/source/Interpreter/OptionGroupMemoryTag.cpp

+//===-- OptionGroupMemoryTag.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/OptionGroupMemoryTag.h"
+
+#include "lldb/Host/OptionParser.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+OptionGroupMemoryTag::OptionGroupMemoryTag() : m_show_tags(false, false) {}
+
+static const uint32_t SHORT_OPTION_SHOW_TAGS = 0x54414753; // 'tags'
+
+static constexpr OptionDefinition g_option_table[] = {
+    {LLDB_OPT_SET_1,
+     false,
+     "show-tags",
+     SHORT_OPTION_SHOW_TAGS,
+     OptionParser::eNoArgument,
+     nullptr,
+     {},
+     0,
+     eArgTypeNone,
+     "Include memory tags in output (does not apply to binary output)."},
+};
+
+llvm::ArrayRef<OptionDefinition> OptionGroupMemoryTag::GetDefinitions() {
+  return llvm::makeArrayRef(g_option_table);
+}
+
+Status
+OptionGroupMemoryTag::SetOptionValue(uint32_t option_idx,
+                                     llvm::StringRef option_arg,
+                                     ExecutionContext *execution_context) {
+  assert(option_idx == 0 && "Only one option in memory tag group!");
+
+  switch (g_option_table[0].short_option) {
+  case SHORT_OPTION_SHOW_TAGS:
+    m_show_tags.SetCurrentValue(true);
+    m_show_tags.SetOptionWasSet();
+    break;
+
+  default:
+    llvm_unreachable("Unimplemented option");
+  }
+
+  return {};
+}
+
+void OptionGroupMemoryTag::OptionParsingStarting(
+    ExecutionContext *execution_context) {
+  m_show_tags.Clear();
+}

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

def test_help_shows_command_options_usage(self):
         """Test that we start the usage section with a specific line."""
         self.expect("help memory read", substrs=["Command Options Usage:\n  memory read"])
 
     @no_debug_info_test
     def test_help_detailed_information_spacing(self):
         """Test that we put a break between the usage and the options help lines,
            and between the options themselves."""
         self.expect("help memory read", substrs=[
-                    "[<address-expression>]\n\n       --show-tags",
-                    # Starts with the end of the show-tags line
-                    "output).\n\n       -A"])
+                    "[<address-expression>]\n\n       -A ( --show-all-children )",
+                    # Starts with the end of the show-all-children line
+                    "to show.\n\n       -D"])
 
     @no_debug_info_test
     def test_help_detailed_information_ordering(self):
         """Test that we order options alphabetically, upper case first."""
         # You could test this with a simple regex like:
         # <upper case>.*<later upper case>.*<lower case>.*<later lower case>
         # Except that that could pass sometimes even with shuffled output.
         # This makes sure that doesn't happen.

lldb/test/API/linux/aarch64/mte_tag_access/TestAArch64LinuxMTEMemoryTagAccess.py

def test_mte_memory_read_tag_display(self):
 
         self.expect("memory read mte_buf mte_buf+32 -f \"bytes with ASCII\" --show-tags",
                 patterns=["0x[0-9A-Fa-f]+00: (00 ){16} \.{16} \(tag: 0x0\)\n",
                           "0x[0-9A-Fa-f]+10: (00 ){16} \.{16} \(tag: 0x1\)"])
 
         self.expect("memory read mte_buf mte_buf+32 -f \"uint8_t[]\" -s 16 -l 1 --show-tags",
                 patterns=["0x[0-9A-Fa-f]+00: \{(0x00 ){15}0x00\} \(tag: 0x0\)\n"
                           "0x[0-9A-Fa-f]+10: \{(0x00 ){15}0x00\} \(tag: 0x1\)"])
+
+    @skipUnlessArch("aarch64")
+    @skipUnlessPlatform(["linux"])
+    @skipUnlessAArch64MTELinuxCompiler
+    def test_mte_memory_read_tag_display_repeated(self):
+        """Test that the --show-tags option is kept when repeating the memory read command."""
+        self.setup_mte_test()
+
+        self.expect("memory read mte_buf mte_buf+16 -f \"x\" --show-tags",
+                    patterns=["0x[0-9A-fa-f]+00: 0x0+ 0x0+ 0x0+ 0x0+ \(tag: 0x0\)"])
+        # Equivalent to just pressing enter on the command line.
+        self.expect("memory read",
+                    patterns=["0x[0-9A-fa-f]+10: 0x0+ 0x0+ 0x0+ 0x0+ \(tag: 0x1\)"])
+
+        # You can add the argument to an existing repetition without resetting
+        # the whole command. Though all other optional arguments will reset to
+        # their default values when you do this.
+        self.expect("memory read mte_buf mte_buf+16 -f \"x\"",
+                    patterns=["0x[0-9A-fa-f]+00: 0x0+ 0x0+ 0x0+ 0x0+"])
+        self.expect("memory read",
+                    patterns=["0x[0-9A-fa-f]+10: 0x0+ 0x0+ 0x0+ 0x0+"])
+        # Note that the formatting returns to default here.
+        self.expect("memory read --show-tags",
+                    patterns=["0x[0-9A-fa-f]+20: (00 )+ \.+ \(tag: 0x2\)"])
+        self.expect("memory read",
+                    patterns=["0x[0-9A-fa-f]+30: (00 )+ \.+ \(tag: 0x3\)"])
+
+        # A fresh command reverts to the default of tags being off.
+        self.expect("memory read mte_buf mte_buf+16 -f \"x\"",
+                    patterns=["0x[0-9A-fa-f]+00: 0x0+ 0x0+ 0x0+ 0x0+"])