[lldb] Remove non address bits from memory read arguments

[DavidSpickett]

Summary

Addresses on AArch64 can have top byte tags, memory tags and pointer
authentication signatures in the upper bits.

While testing memory tagging I found that memory read couldn't
read a range if the two addresses had different tags. The same
could apply to signed pointers given the right circumstance.

(lldb) memory read mte_buf_alt_tag mte_buf+16
error: end address (0x900fffff7ff8010) must be greater than the start
address (0xa00fffff7ff8000).

Or it would try to read a lot more memory than expected.

(lldb) memory read mte_buf mte_buf_alt_tag+16
error: Normally, 'memory read' will not read over 1024 bytes of data.
error: Please use --force to override this restriction just once.
error: or set target.max-memory-read-size if you will often need a
larger limit.

Fix this by removing non address bits before we calculate the read
range. A test is added for AArch64 Linux that confirms this by using
the top byte ignore feature.

This means that if you do read with a tagged pointer the output
does not include those tags. This is potentially confusing but I think
overall it's better that we don't pretend that we're reading memory
from a range that the process is unable to map.

(lldb) p ptr1
(char *) $4 = 0x3400fffffffff140 "\x80\xf1\xff\xff\xff\xff"
(lldb) p ptr2
(char *) $5 = 0x5600fffffffff140 "\x80\xf1\xff\xff\xff\xff"
(lldb) memory read ptr1 ptr2+16
0xfffffffff140: 80 f1 ff ff ff ff 00 00 38 70 bc f7 ff ff 00 00 ........8p......

[DavidSpickett]

The side effect here is that you do "memory read <tagged ptr>" and you see untagged addresses for the lines. It's not really that confusing but maybe something we should make a general decision about.

Same thing applies to the previous "memory region" patch.

(lldb) p ptr1
(char *) $1 = 0x344dfffffffffcb0 ""
(lldb) memory read ptr1
0xfffffffffcb0: f0 05 40 00 00 00 00 00 00 00 00 00 00 00 00 00  ..@.............
0xfffffffffcc0: e0 fc ff ff ff ff 00 00 54 f0 e7 f7 ff ff 00 00  ........T.......
(lldb) memory region ptr1
[0x0000fffffffdf000-0x0001000000000000) rw- [stack]

Personally it doesn't bother me, but then again I know what all the upper bits are doing anyway.

[omjavaid]

This doesnt build ... you need to include #include "lldb/Target/ABI.h" in lldb/source/Commands/CommandObjectMemory.cpp

lldb/test/API/linux/aarch64/tagged_memory_read/TestAArch64LinuxTaggedMemoryRead.py

23

May be consider renaming this test case.

24

This condition will always be true because we havent yet connected to remote platform.

[pcc]

In D103626#2796554, @DavidSpickett wrote:

The side effect here is that you do "memory read <tagged ptr>" and you see untagged addresses for the lines. It's not really that confusing but maybe something we should make a general decision about.

Same thing applies to the previous "memory region" patch.

(lldb) p ptr1
(char *) $1 = 0x344dfffffffffcb0 ""
(lldb) memory read ptr1
0xfffffffffcb0: f0 05 40 00 00 00 00 00 00 00 00 00 00 00 00 00  ..@.............
0xfffffffffcc0: e0 fc ff ff ff ff 00 00 54 f0 e7 f7 ff ff 00 00  ........T.......
(lldb) memory region ptr1
[0x0000fffffffdf000-0x0001000000000000) rw- [stack]

Personally it doesn't bother me, but then again I know what all the upper bits are doing anyway.

In Android tombstones we display the memory tag inline with the address, e.g.

memory near x8 ([anon:scudo:primary]):
    00000079fd11a1d0 0000000000000000 0000000000000000  ................
    00000079fd11a1e0 84e4000000020402 000037ecb699a1b3  .............7..
    0f000079fd11a1f0 0000000000000000 0000000000000000  ................
    0f000079fd11a200 0000000000000000 0000000000000000  ................
    00000079fd11a210 0000000000000000 0000000000000000  ................
    00000079fd11a220 0000000000000000 0000000000000000  ................
    00000079fd11a230 0000000000000000 0000000000000000  ................
    00000079fd11a240 0000000000000000 0000000000000000  ................
    00000079fd11a250 0000000000000000 0000000000000000  ................
    00000079fd11a260 0000000000000000 0000000000000000  ................
    00000079fd11a270 0000000000000000 0000000000000000  ................
    00000079fd11a280 0000000000000000 0000000000000000  ................
    00000079fd11a290 0000000000000000 0000000000000000  ................
    00000079fd11a2a0 0000000000000000 0000000000000000  ................
    00000079fd11a2b0 0000000000000000 0000000000000000  ................
    00000079fd11a2c0 0000000000000000 0000000000000000  ................

Maybe this is something worth considering for LLDB memory dumps?

[DavidSpickett]

Maybe this is something worth considering for LLDB memory dumps?

I'm working on that at the moment, it's on my github branch. This is what it looks like with the right options: (others look weird at the moment due to ordering issues)

(lldb) memory read mte_buf mte_buf+32 -f "x" -l 1 -s 16
0xfffff7ff6000: 0x00000000000000000000000000000000 (tag: 0x0)
0xfffff7ff6010: 0x00000000000000000000000000000000 (tag: 0x1)

The format is mostly a guess on my part, given that I'm mostly testing the debugger itself not MTE enabled software.

[DavidSpickett]

• Add missing include
• Rename test

[DavidSpickett]

lldb/test/API/linux/aarch64/tagged_memory_read/TestAArch64LinuxTaggedMemoryRead.py

24

Runs fine for me using dotest:

$ ./bin/lldb-dotest --platform-name remote-linux <...> -p TestAArch64LinuxTaggedMemoryRead.py --arch aarch64

And this is how it's used elsewhere. Perhaps you're running the tests in a different way?

[omjavaid]

lldb/test/API/linux/aarch64/tagged_memory_read/TestAArch64LinuxTaggedMemoryRead.py

24

This will be fixed by D105060

[DavidSpickett]

Rebase onto main.

[DavidSpickett]

Rebase

[DavidSpickett]

Just to update where I am with this, I think I've got a good argument to not show the non-address bits in the output.

That being that the memory itself that we read doesn't include these bits, so showing it is just going to be more confusing that the initial confusion of the address changing. In addition for things like memory tagging once you move beyond the granule your original pointer points to, the actual tag stored in hardware could be different. So we're misleading in that way.

The same applies to signed pointers where incrementing the signing bits isn't a thing in any case, so the signature for foo and foo+4 is probably different anyway.

I think that makes sense but I don't want to just go ahead until I get other's opinions.

[DavidSpickett]

Sent https://lists.llvm.org/pipermail/lldb-dev/2021-December/017163.html to get some more input.

[DavidSpickett]

Rebase onto main

[DavidSpickett]

Since we're so close to the end of the year I'm going to give this more time and ping early next year in case of any more feedback. Though I'm feeling confident in the direction so far.

While working on this I realised that the API calls should also remove the non-address bits. We will still need to have this change, so that checking for inverted ranges still works. Changes to the underlying MemoryRead and API will be a separate review.

[DavidSpickett]

Add release note for this change.

Closer to release I'll see if it makes more sense to make a list of commands
that have had the same changes, but this stops me forgetting entirely.

[danielkiss]

In D103626#2824083, @DavidSpickett wrote:

Maybe this is something worth considering for LLDB memory dumps?

I'm working on that at the moment, it's on my github branch. This is what it looks like with the right options: (others look weird at the moment due to ordering issues)

(lldb) memory read mte_buf mte_buf+32 -f "x" -l 1 -s 16
0xfffff7ff6000: 0x00000000000000000000000000000000 (tag: 0x0)
0xfffff7ff6010: 0x00000000000000000000000000000000 (tag: 0x1)

The format is mostly a guess on my part, given that I'm mostly testing the debugger itself not MTE enabled software.

That looks really good, could help a lot to understand tagging related issues. Looking forward for that patch :)

This change LGTM.

[DavidSpickett]

That looks really good, could help a lot to understand tagging related issues. Looking forward for that patch :)

https://reviews.llvm.org/D107140 is the end of the series for that if you want to see it. The test cases show various corner cases too.

[DavidSpickett]

Rebase, update some comment wording.

[DavidSpickett]

Since there was no great pushback on this I've landed it. With the final justification being that the non-address bits are properties of the pointer not of the memory pointed to.

I will do a follow up where I apply the same logic to the ReadMemory calls via the API.

lldb/source/Commands/CommandObjectMemory.cpp

 #include "lldb/Interpreter/OptionGroupFormat.h"
 #include "lldb/Interpreter/OptionGroupOutputFile.h"
 #include "lldb/Interpreter/OptionGroupValueObjectDisplay.h"
 #include "lldb/Interpreter/OptionValueLanguage.h"
 #include "lldb/Interpreter/OptionValueString.h"
 #include "lldb/Interpreter/Options.h"
 #include "lldb/Symbol/SymbolFile.h"
 #include "lldb/Symbol/TypeList.h"
+#include "lldb/Target/ABI.h"
 #include "lldb/Target/Language.h"
 #include "lldb/Target/MemoryHistory.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/StackFrame.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Utility/Args.h"
if (argc > 0)
                                         LLDB_INVALID_ADDRESS, &error);
 
     if (addr == LLDB_INVALID_ADDRESS) {
       result.AppendError("invalid start address expression.");
       result.AppendError(error.AsCString());
       return false;
     }
 
+    ABISP abi = m_exe_ctx.GetProcessPtr()->GetABI();
+    if (abi)
+      addr = abi->FixDataAddress(addr);
+
     if (argc == 2) {
       lldb::addr_t end_addr = OptionArgParser::ToAddress(
           &m_exe_ctx, command[1].ref(), LLDB_INVALID_ADDRESS, nullptr);
+      if (end_addr != LLDB_INVALID_ADDRESS && abi)
+        end_addr = abi->FixDataAddress(end_addr);
+
       if (end_addr == LLDB_INVALID_ADDRESS) {
         result.AppendError("invalid end address expression.");
         result.AppendError(error.AsCString());
         return false;
       } else if (end_addr <= addr) {
         result.AppendErrorWithFormat(
             "end address (0x%" PRIx64
             ") must be greater than the start address (0x%" PRIx64 ").\n",

lldb/test/API/linux/aarch64/tagged_memory_read/Makefile

+C_SOURCES := main.c
+CFLAGS_EXTRAS := -march=armv8.3-a
+
+include Makefile.rules

lldb/test/API/linux/aarch64/tagged_memory_read/TestAArch64LinuxTaggedMemoryRead.py

+"""
+Test that "memory read" removes non address bits from
+memory read arguments.
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class AArch64LinuxTaggedMemoryReadTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    NO_DEBUG_INFO_TESTCASE = True
+
+    # AArch64 Linux always enables top byte ignore
+    @skipUnlessArch("aarch64")
+    @skipUnlessPlatform(["linux"])
+    def test_tagged_memory_read(self):
+        self.build()
+        self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
+
+        lldbutil.run_break_set_by_file_and_line(self, "main.c",
+            line_number('main.c', '// Set break point at this line.'),
+            num_expected_locations=1)
+
+        self.runCmd("run", RUN_SUCCEEDED)
+
+        if self.process().GetState() == lldb.eStateExited:
+            self.fail("Test program failed to run.")
+
+        self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+            substrs=['stopped',
+                     'stop reason = breakpoint'])
+
+        # If we do not remove non address bits, this can fail in two ways.
+        # 1. We attempt to read much more than 16 bytes, probably more than
+        #    the default 1024 byte read size. Which will error.
+        # 2. We error because end address is < start address since end's
+        #    tag is < start's tag.
+        #
+        # Each time we check that the printed line addresses do not include
+        # either of the tags we set. Those bits are a property of the
+        # pointer not of the memory it points to.
+        tagged_addr_pattern = "0x(34|46)[0-9A-Fa-f]{14}:.*"
+        self.expect("memory read ptr1 ptr2+16", patterns=[tagged_addr_pattern], matching=False)
+        # Check that the stored previous end address is stripped
+        self.expect("memory read", patterns=[tagged_addr_pattern], matching=False)
+        # Would fail if we don't remove non address bits because 0x56... > 0x34...
+        self.expect("memory read ptr2 ptr1+16", patterns=[tagged_addr_pattern], matching=False)
+        self.expect("memory read", patterns=[tagged_addr_pattern], matching=False)

lldb/test/API/linux/aarch64/tagged_memory_read/main.c

+#include <stddef.h>
+
+static char *set_non_address_bits(char *ptr, size_t tag) {
+  // Set top byte tag (AArch64 Linux always enables top byte ignore)
+  return (char *)((size_t)ptr | (tag << 56));
+}
+
+int main(int argc, char const *argv[]) {
+  char buf[32];
+
+  char *ptr1 = set_non_address_bits(buf, 0x34);
+  char *ptr2 = set_non_address_bits(buf, 0x56);
+
+  return 0; // Set break point at this line.
+}

llvm/docs/ReleaseNotes.rst

* llvm-cov: `-name-allowlist` is now accepted in addition to `-name-whitelist`.
   releases.
 
 Changes to LLDB
 ---------------------------------
 
 * A change in Clang's type printing has changed the way LLDB names array types
   (from ``int [N]`` to ``int[N]``) - LLDB pretty printer type name matching
   code may need to be updated to handle this.
+* The ``memory read`` command now ignores non-address bits in start and end
+  addresses. In addition, non-address bits will not be shown in the addresses
+  in the output.
 
 Changes to Sanitizers
 ---------------------
 
 External Open Source Projects Using LLVM 14
 ===========================================
 
 * A project...