omjavaid 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.
================
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.
+ ///
----------------
Can you explain this line a bit? What i understood we dont include start and
end address in tag range. right?
================
Comment at: lldb/include/lldb/Target/Process.h:1724
+ /// \param[in] addr
+ /// Start of memory range to tags for.
+ ///
----------------
I guess you meant to say read tags for?
================
Comment at: lldb/include/lldb/Target/Process.h:2749
+ /// Check whether the remote supports memory tagging.
+ ///
----------------
By remote you mean gdb-remote process? This probably will be generic routine
used by other platforms.
in context of lldb we dont have remote rather platforms where gdb-remote is a
type of platform.
================
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(),
----------------
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.
================
Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:1272
- def isAArch64SVE(self):
+ def hasLinuxCPUInfoFeature(self, feature):
triple = self.dbg.GetSelectedPlatform().GetTriple()
----------------
This change look good and can be committed outside this patch.
================
Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:1272
- def isAArch64SVE(self):
+ def hasLinuxCPUInfoFeature(self, feature):
triple = self.dbg.GetSelectedPlatform().GetTriple()
----------------
omjavaid wrote:
> This change look good and can be committed outside this patch.
This change looks fine commit it separately.
================
Comment at: lldb/source/Target/Process.cpp:6031
+ const MemoryTagHandler *tag_handler =
+ arch ? arch->GetMemoryTagHandler() : nullptr;
+ if (!arch || !tag_handler) {
----------------
'arch' may be nullptr so call to GetMemoryTagHandler is not safe.
================
Comment at: lldb/source/Target/Process.cpp:6058
+ MemoryRegionInfo::RangeType tag_range(untagged_addr, len);
+ tag_range = tag_handler->AlignToGranules(tag_range);
+
----------------
Can there be multiple 'granules' defined for an implementation ? if not this
func may be renamed (AlignToGranule) to reflect that
================
Comment at: lldb/test/API/linux/aarch64/mte_tag_read/main.c:44
+ // Tag the original pointer with 9
+ buf = __arm_mte_create_random_tag(buf, ~(1 << 9));
+ // A different tag so that buf_alt_tag > buf if you don't handle the tag
----------------
May be add a line or two of comment about mte intrinsics for any future readers
of these test code.
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
