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

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added a comment.

This looks good apart from minor nits inline



================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1357
+
+  if (error.Fail())
+    return error;
----------------
ptrace request is a success if number of tags requested is not equal to no of tags read? If not then this and following condition may be redundant.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:1157
+llvm::Expected<NativeRegisterContextLinux::MemoryTaggingDetails>
+NativeRegisterContextLinux_arm64::GetMemoryTaggingDetails(int32_t type) {
+  if (type == MemoryTagManagerAArch64MTE::eMTE_allocation) {
----------------
this piece needs to run clang-format


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:3578
+
+  response.PutChar('m');
+  response.PutBytesAsRawHex8(tags.data(), tags.size());
----------------
Just curious response starts with 'm'. Whats the design need for using m in qMemTags response?


================
Comment at: lldb/test/API/tools/lldb-server/memory-tagging/TestGdbRemoteMemoryTagging.py:19
+    @skipUnlessPlatform(["linux"])
+    @skipUnlessAArch64MTELinuxCompiler
+    def test_qmemtags_packets(self):
----------------
If skipUnlessAArch64MTELinuxCompiler can check for AArch64 and Linux then we wont need above two decorators.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95601