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

[David Spickett via Phabricator via lldb-commits]

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

Cool, I wasn't sure how to split while keeping it readable but that sounds good.



================
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.
+  ///
----------------
omjavaid wrote:
> Can you explain this line a bit? What i understood we dont include start and end address in tag range. right?
Does the description of `end_addr` answer your question?

GetMemoryTagHandler(10, 21) would check a range from 10-20.


================
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(),
----------------
omjavaid wrote:
> 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.
GDB is using a signed int, though we don't have need for negative numbers at this time. I'll cite the GDB design for this.

We could say well, for the moment it might as well be unsigned but I don't want to introduce a situation where in future we mix client/servers and lldb falls over on a "-".


================
Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:1272
 
-    def isAArch64SVE(self):
+    def hasLinuxCPUInfoFeature(self, feature):
         triple = self.dbg.GetSelectedPlatform().GetTriple()
----------------
omjavaid wrote:
> omjavaid wrote:
> > This change look good and can be committed outside this patch.
> This change looks fine commit it separately.
Yeah your registers patch does the same thing so depending on timing I might end up using that.


================
Comment at: lldb/source/Target/Process.cpp:6031
+  const MemoryTagHandler *tag_handler =
+      arch ? arch->GetMemoryTagHandler() : nullptr;
+  if (!arch || !tag_handler) {
----------------
omjavaid wrote:
> 'arch' may be nullptr so call to GetMemoryTagHandler is not safe.
Sure, that's why I check it first with the `arch ?`. Happy to refactor if it could be clearer.

I should say that some of this boilerplate looking for the tag handler is subject to change once I've written more commands. It's repetitive now but later I should be able to refactor with the context of how all the commands use it.


================
Comment at: lldb/source/Target/Process.cpp:6058
+  MemoryRegionInfo::RangeType tag_range(untagged_addr, len);
+  tag_range = tag_handler->AlignToGranules(tag_range);
+
----------------
omjavaid wrote:
> Can there be multiple 'granules' defined for an implementation ? if not this func may be renamed (AlignToGranule) to reflect that 
MTE only has one granule size, Sparc ADI uses cache lines so I assume theirs is just the cache line size.

If I read "align to granule" singular I think it's align to a single granule, probably up. Since this aligns up and down granules plural made more sense to me.

That said, it's never going to shrink the range, so ExpandToGranule would be more descriptive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95602