[Lldb-commits] [PATCH] D95601: [lldb][AArch64] Add memory tag reading to lldb-server

[David Spickett via Phabricator via lldb-commits]

DavidSpickett marked an inline comment as done.
DavidSpickett added inline comments.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1357
+
+  if (error.Fail())
+    return error;
----------------
DavidSpickett wrote:
> omjavaid wrote:
> > ptrace request is a success if number of tags requested is not equal to no of tags read? If not then this and following condition may be redundant.
> Well ptracewrapper doesn't check the iovec, but I'll check the kernel source to see if it's actually possible for it to fail that way.
In `linux/arch/arm64/kernel/mte.c` `__access_remote_tags` there is a comment:
```
+/*
+ * Access MTE tags in another process' address space as given in mm. Update
+ * the number of tags copied. Return 0 if any tags copied, error otherwise.
+ * Inspired by __access_remote_vm().
+ */
```

*any tags* being the key words.

So the scenario is:
* ask to read from addr X in page 0, with length of pagesize+some so the range spills into page 1
* kernel can access page 0, reads tags until the end of the page
* tries to access page 1 to read the rest, fails, returns 0 (success) since *some* tags were read
* we see the ptrace call succeeded but with less tags than we expected

I don't see it's worth dealing with this corner case here since lldb will look before it leaps. It would have errored much earlier here because either page 1 isn't in the tracee's memory regions or it wasn't MTE enabled.




================
Comment at: lldb/test/API/tools/lldb-server/memory-tagging/TestGdbRemoteMemoryTagging.py:19
+    @skipUnlessPlatform(["linux"])
+    @skipUnlessAArch64MTELinuxCompiler
+    def test_qmemtags_packets(self):
----------------
DavidSpickett wrote:
> omjavaid wrote:
> > If skipUnlessAArch64MTELinuxCompiler can check for AArch64 and Linux then we wont need above two decorators.
> I'll merge them into one (at least one you use in tests, separate functions). Also I just realised this is not checking that the remote supports MTE, only worked because I've been using the one qemu instance.
On further consideration I don't think it's worth merging them. Sure we save 2 lines in each test but then anyone reading it is going to have to lookup what the combo does. I'd rather keep them listed like this for clarity (and later adding new platforms?).

Also I was wrong, the test does check for non MTE systems. If the tracee prints (nil) for the buffer, that means it's not an MTE system.
(we can't use the isAArch64MTE call in this type of test)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95601