[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.

it look good to me but I have some final minor nits inline.



================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1458
+
+  struct iovec tags_vec;
+  uint8_t *dest = &tags[0];
----------------
may be rename to tags_iovec? because reading code below i had the tags vector in mind and confused it for that vector which was passed as parameter to this function.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1459
+  struct iovec tags_vec;
+  uint8_t *dest = &tags[0];
+  lldb::addr_t read_addr = range.GetRangeBase();
----------------
may be use tags.data() ?


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1464
+  // get all tags back.
+  while (num_tags > 0) {
+    tags_vec.iov_base = dest;
----------------
this loop condition is is a little fishy.  num_tags is unsigned which means if by chance it doesnt end up going to zero we ll keep looping for ever.


================
Comment at: lldb/test/API/tools/lldb-server/memory-tagging/main.c:53
+  // smoke test in case something didn't account for them.
+  buf = (char *)((size_t)buf | ((size_t)0xAA << 56));
+  return print_result(buf);
----------------
Just a side question about TBI, for memroy reads/write or tags/query is it necessary to send non address bits to remote? (tags and pauth masks). Can we instead clear these bits before sending address over using the code/data masks we have calculated in our host process class.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95601