[Lldb-commits] [PATCH] D97281: [lldb][AArch64] Add class for managing memory tags

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added a comment.

This looks ok to me apart from some cosmetic nits. Also you may figure out a way to clean up the whole address including pauth now after D99944 <https://reviews.llvm.org/D99944> is merged.



================
Comment at: lldb/include/lldb/Target/MemoryTagManager.h:72
+  // transport.
+  virtual size_t GetBytesPerTag() const = 0;
+
----------------
Bytes per tag conveys a different message like "No of bytes of memory this tag corresponds to" 
It could GetTagByteSize or something.


================
Comment at: lldb/include/lldb/Target/MemoryTagManager.h:79
+  virtual llvm::Expected<std::vector<lldb::addr_t>>
+  UnpackTags(const std::vector<uint8_t> &tags, size_t granules) const = 0;
+
----------------
This might also be renamed to UnpackTagsData because input is not guaranteed to be tags unless their byte size is 1. 


================
Comment at: lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp:28
+  // user metadata.
+  return addr & ~((lldb::addr_t)0xFF << MTE_START_BIT);
+}
----------------
This function need to take care of Pointer Auth mask which could be living in bits 48:54 


================
Comment at: lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp:61
+  size_t new_len = range.GetByteSize() + align_down_amount;
+  // Then align up to the end of the granule
+  if (new_len % granule)
----------------
May be consider doing something like below to avoid calculating mod twice.
  lldb::addr_t align_up_amount = new_len % granule;
  if (align_up_amount)
  ....



================
Comment at: lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.h:24
+  };
+
+  lldb::addr_t GetLogicalTag(lldb::addr_t addr) const override;
----------------
All functions below have no spacing between them may be consider adding a few spaces while combining declaration of setter/getter or other similar function in consecutive lines.


================
Comment at: lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp:73
+
+  // Reading less than 1 granule, rounds up to 1 granule
+  ASSERT_EQ(
----------------
This is confusing   // Reading less than 1 granule, rounds up to 1 granule
May be reword a little like "No of bytes being read are less than bytes per granule"



================
Comment at: lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp:100
+  ASSERT_EQ(0, 0);
+  ASSERT_EQ((lldb::addr_t)0x00ffeedd11223344,
+            manager.RemoveNonAddressBits(0x00ffeedd11223344));
----------------
This should be able to remove pointer auth mask so fix this test accordingly.


================
Comment at: lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp:116
+  // Anything in the top byte is ignored
+  ASSERT_EQ(0, manager.AddressDiff(0x2211222233334444, 0x3311222233334444));
+  ASSERT_EQ(-32, manager.AddressDiff(0x5511222233334400, 0x4411222233334420));
----------------
likewise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97281