[Lldb-commits] [PATCH] D95601: [lldb][AArch64] Add MTE memory tag reading to lldb-server

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added a comment.

This patch looks quite neat overall I have a few minor questions, comments and suggestions.

1. To get the review going consider further splitting up these patches into separate chunks containing, for example MemoryTagHandler API and related unit tests, GDB remote addons, ptrace work etc. Also add reference to GDB mailing list discussion where design of qMemTag packet has been discussed.

2. Soft suggestion to rename MemoryTagHandler, IMO it sounds more like a routine rather than a set of helpers used for manipulating memory tags. Also lldb/include/lldb/Target doesnt seem like a good place for helper functions as MemoryTagHandler is not used as Memory Tag container class. May be consider putting it under source/Plugins/Process/Utility

3. Changes to lldb/packages/Python/lldbsuite/test/decorators.py look reasonable and can be committed right away with a small caveat.

Some other minor comments inline as well.



================
Comment at: lldb/packages/Python/lldbsuite/test/decorators.py:825
 
+def skipUnlessAArch64MTELinuxToolchain(func):
+    """Decorate the item to skip test unless MTE is supported by the test compiler/toolchain."""
----------------
Consider using Compiler instead of Toolchain to keep terminology consistent with rest of the code in this file.


================
Comment at: lldb/packages/Python/lldbsuite/test/decorators.py:858
+
+
 def _get_bool_config(key, fail_value = True):
----------------
Omit one space.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1518
+  Status error = NativeProcessLinux::PtraceWrapper(
+      details->ptrace_read_req, GetID(),
+      reinterpret_cast<void *>(range.GetRangeBase()),
----------------
Do you think future architectures will have any different ptrace_read_req/ptrace_write_req than PTRACE_PEEKMTETAGS/PTRACE_POKEMTETAGS.

If not then there is is no advantage of putting them inside MemoryTaggingDetails


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h:60
 
+  struct MemoryTaggingDetails {
+    std::unique_ptr<MemoryTagHandler> handler;
----------------
Add comment about MemoryTaggingDetails


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h:66
+  virtual llvm::Expected<MemoryTaggingDetails>
+  GetMemoryTaggingDetails(int32_t type) {
+    return llvm::createStringError(
----------------
Can type be negative. Does gdb remote protocol specifies type as int32? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95601