[Lldb-commits] [PATCH] D95602: [lldb][AArch64] Add MTE memory tag reading for lldb

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added a comment.

This patch also looks quite good. Some minor nits inline and also move gdb* changes into a single patch with both client and server side code.



================
Comment at: lldb/include/lldb/Target/Process.h:1701
+  /// handler that can be used to maniupulate those memory tags.
+  /// Tags present in the addresses given are ignored.
+  ///
----------------
Can you explain this line a bit? What i understood we dont include start and end address in tag range. right?


================
Comment at: lldb/include/lldb/Target/Process.h:1724
+  /// \param[in] addr
+  ///     Start of memory range to tags for.
+  ///
----------------
I guess you meant to say read tags for?


================
Comment at: lldb/include/lldb/Target/Process.h:2749
 
+  /// Check whether the remote supports memory tagging.
+  ///
----------------
By remote you mean gdb-remote process? This probably will be generic routine used by other platforms.
in context of lldb we dont have remote rather platforms where gdb-remote is a type of platform.


================
Comment at: lldb/include/lldb/Target/Process.h:2773
+  virtual llvm::Expected<std::vector<uint8_t>>
+  DoReadMemoryTags(lldb::addr_t addr, size_t len, int32_t type) {
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
----------------
I guess if there are no restrictions by specs then we should rethink use of int32_t for type may be uint32_t if possible.


================
Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:1272
 
-    def isAArch64SVE(self):
+    def hasLinuxCPUInfoFeature(self, feature):
         triple = self.dbg.GetSelectedPlatform().GetTriple()
----------------
This change look good and can be committed outside this patch.


================
Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:1272
 
-    def isAArch64SVE(self):
+    def hasLinuxCPUInfoFeature(self, feature):
         triple = self.dbg.GetSelectedPlatform().GetTriple()
----------------
omjavaid wrote:
> This change look good and can be committed outside this patch.
This change looks fine commit it separately.


================
Comment at: lldb/source/Target/Process.cpp:6031
+  const MemoryTagHandler *tag_handler =
+      arch ? arch->GetMemoryTagHandler() : nullptr;
+  if (!arch || !tag_handler) {
----------------
'arch' may be nullptr so call to GetMemoryTagHandler is not safe.


================
Comment at: lldb/source/Target/Process.cpp:6058
+  MemoryRegionInfo::RangeType tag_range(untagged_addr, len);
+  tag_range = tag_handler->AlignToGranules(tag_range);
+
----------------
Can there be multiple 'granules' defined for an implementation ? if not this func may be renamed (AlignToGranule) to reflect that 


================
Comment at: lldb/test/API/linux/aarch64/mte_tag_read/main.c:44
+  // Tag the original pointer with 9
+  buf = __arm_mte_create_random_tag(buf, ~(1 << 9));
+  // A different tag so that buf_alt_tag > buf if you don't handle the tag
----------------
May be add a line or two of comment about mte intrinsics for any future readers of these test code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95602/new/

https://reviews.llvm.org/D95602