omjavaid added a comment.
This patch looks quite neat overall I have a few minor questions, comments and
suggestions.
1. To get the review going consider further splitting up these patches into
separate chunks containing, for example MemoryTagHandler API and related unit
tests, GDB remote addons, ptrace work etc. Also add reference to GDB mailing
list discussion where design of qMemTag packet has been discussed.
2. Soft suggestion to rename MemoryTagHandler, IMO it sounds more like a
routine rather than a set of helpers used for manipulating memory tags. Also
lldb/include/lldb/Target doesnt seem like a good place for helper functions as
MemoryTagHandler is not used as Memory Tag container class. May be consider
putting it under source/Plugins/Process/Utility
3. Changes to lldb/packages/Python/lldbsuite/test/decorators.py look reasonable
and can be committed right away with a small caveat.
Some other minor comments inline as well.
================
Comment at: lldb/packages/Python/lldbsuite/test/decorators.py:825
+def skipUnlessAArch64MTELinuxToolchain(func):
+ """Decorate the item to skip test unless MTE is supported by the test
compiler/toolchain."""
----------------
Consider using Compiler instead of Toolchain to keep terminology consistent
with rest of the code in this file.
================
Comment at: lldb/packages/Python/lldbsuite/test/decorators.py:858
+
+
def _get_bool_config(key, fail_value = True):
----------------
Omit one space.
================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1518
+ Status error = NativeProcessLinux::PtraceWrapper(
+ details->ptrace_read_req, GetID(),
+ reinterpret_cast<void *>(range.GetRangeBase()),
----------------
Do you think future architectures will have any different
ptrace_read_req/ptrace_write_req than PTRACE_PEEKMTETAGS/PTRACE_POKEMTETAGS.
If not then there is is no advantage of putting them inside MemoryTaggingDetails
================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h:60
+ struct MemoryTaggingDetails {
+ std::unique_ptr<MemoryTagHandler> handler;
----------------
Add comment about MemoryTaggingDetails
================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h:66
+ virtual llvm::Expected<MemoryTaggingDetails>
+ GetMemoryTaggingDetails(int32_t type) {
+ return llvm::createStringError(
----------------
Can type be negative. Does gdb remote protocol specifies type as int32?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95601/new/
https://reviews.llvm.org/D95601
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
