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

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. Reviewed By: omjavaid Differential Revision: https://reviews.llvm.org/D125089
llvm · May 18, 2022 · 29e556f · 29e556f
1 parent ca302f0
commit 29e556f
Show file tree

Hide file tree

Showing 7 changed files with 143 additions and 13 deletions.
diff --git a/lldb/include/lldb/Interpreter/OptionGroupMemoryTag.h b/lldb/include/lldb/Interpreter/OptionGroupMemoryTag.h
@@ -0,0 +1,40 @@
+//===-- 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
diff --git a/lldb/source/Commands/CommandObjectMemory.cpp b/lldb/source/Commands/CommandObjectMemory.cpp
@@ -16,6 +16,7 @@
 #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"
@@ -90,10 +91,6 @@ class OptionGroupReadMemory : public OptionGroup {
       error = m_offset.SetValueFromString(option_value);
       break;
 
-    case '\x01':
-      m_show_tags = true;
-      break;
-
     default:
       llvm_unreachable("Unimplemented option");
     }
@@ -107,7 +104,6 @@ class OptionGroupReadMemory : public OptionGroup {
     m_force = false;
     m_offset.Clear();
     m_language_for_type.Clear();
-    m_show_tags = false;
   }
 
   Status FinalizeSettings(Target *target, OptionGroupFormat &format_options) {
@@ -281,7 +277,6 @@ class OptionGroupReadMemory : public OptionGroup {
   bool m_force;
   OptionValueUInt64 m_offset;
   OptionValueLanguage m_language_for_type;
-  bool m_show_tags = false;
 };
 
 // Read memory from the inferior process
@@ -336,6 +331,8 @@ class CommandObjectMemoryRead : public CommandObjectParsed {
     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();
   }
 
@@ -555,11 +552,13 @@ class CommandObjectMemoryRead : public CommandObjectParsed {
       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;
       }
     }
 
@@ -753,6 +752,7 @@ class CommandObjectMemoryRead : public CommandObjectParsed {
     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;
@@ -864,7 +864,7 @@ class CommandObjectMemoryRead : public CommandObjectParsed {
     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;
@@ -875,12 +875,14 @@ class CommandObjectMemoryRead : public CommandObjectParsed {
   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;
 };
 

diff --git a/lldb/source/Commands/Options.td b/lldb/source/Commands/Options.td
@@ -491,8 +491,6 @@ let Command = "memory read" in {
     "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 {

diff --git a/lldb/source/Interpreter/CMakeLists.txt b/lldb/source/Interpreter/CMakeLists.txt
@@ -18,6 +18,7 @@ add_lldb_library(lldbInterpreter
   OptionGroupBoolean.cpp
   OptionGroupFile.cpp
   OptionGroupFormat.cpp
+  OptionGroupMemoryTag.cpp
   OptionGroupPythonClassWithDict.cpp
   OptionGroupOutputFile.cpp
   OptionGroupPlatform.cpp

diff --git a/lldb/source/Interpreter/OptionGroupMemoryTag.cpp b/lldb/source/Interpreter/OptionGroupMemoryTag.cpp
@@ -0,0 +1,59 @@
+//===-- 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();
+}
diff --git a/lldb/test/API/commands/help/TestHelp.py b/lldb/test/API/commands/help/TestHelp.py
@@ -262,9 +262,9 @@ 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):

diff --git a/lldb/test/API/linux/aarch64/mte_tag_access/TestAArch64LinuxMTEMemoryTagAccess.py b/lldb/test/API/linux/aarch64/mte_tag_access/TestAArch64LinuxMTEMemoryTagAccess.py
@@ -411,3 +411,33 @@ def test_mte_memory_read_tag_display(self):
         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+"])