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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
