[lldb] Call FixUpPointer in WritePointerToMemory (try 2)

[felipepiovezan]

In architectures where pointers may contain metadata, such as arm64e, the metadata may need to be cleaned prior to sending this pointer to be used in expression evaluation generated code.

This patch is a step towards allowing consumers of pointers to decide whether they want to keep or remove metadata, as opposed to discarding metadata at the moment pointers are created. See #150537.

This was tested running the LLDB test suite on arm64e.

(The first attempt at this patch caused a failure in TestScriptedProcessEmptyMemoryRegion.py. This test exercises a case where IRMemoryMap uses host memory in its allocations; pointers to such allocations should not be fixed, which is what the original patch failed to account for).

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Felipe de Azevedo Piovezan (felipepiovezan)

Changes

In architectures where pointers may contain metadata, such as arm64e, the metadata may need to be cleaned prior to sending this pointer to be used in expression evaluation generated code.

This patch is a step towards allowing consumers of pointers to decide whether they want to keep or remove metadata, as opposed to discarding metadata at the moment pointers are created. See #150537.

This was tested running the LLDB test suite on arm64e.

(The first attempt at this patch caused a failure in TestScriptedProcessEmptyMemoryRegion.py. This test exercises a case where IRMemoryMap uses host memory in its allocations; pointers to such allocations should not be fixed, which is what the original patch failed to account for).

---

Full diff: https://github.com/llvm/llvm-project/pull/153585.diff

1 Files Affected:

• (modified) lldb/source/Expression/IRMemoryMap.cpp (+7)

diff --git a/lldb/source/Expression/IRMemoryMap.cpp b/lldb/source/Expression/IRMemoryMap.cpp
index 150699352a2e3..3ac42649d0834 100644
--- a/lldb/source/Expression/IRMemoryMap.cpp
+++ b/lldb/source/Expression/IRMemoryMap.cpp
@@ -640,6 +640,13 @@ void IRMemoryMap::WritePointerToMemory(lldb::addr_t process_address,
                                        lldb::addr_t address, Status &error) {
   error.Clear();
 
+  /// Only ask the Process to fix the address if this address belongs to the
+  /// process (host allocations are stored in m_data).
+  if (auto it = FindAllocation(process_address, 1);
+      it != m_allocations.end() && it->second.m_data.GetByteSize() == 0)
+    if (auto process_sp = GetProcessWP().lock())
+      address = process_sp->FixAnyAddress(address);
+
   Scalar scalar(address);
 
   WriteScalarToMemory(process_address, scalar, GetAddressByteSize(), error);

[felipepiovezan]

Thanks @jasonmolenda for suggesting a solution to the test issues

[DavidSpickett]

This is very "modern C++" but very much confusing to read. I think splitting the first if condition up is better. Or add braces to the first if, whichever you prefer.

[DavidSpickett]

Also could we have an overlap between host and debugee memory addresses?

I suspect the answer is "yes but not in the scenario I'm fixing". As if we were going to write anything to the debugee, there would be no host allocatons in m_data.

Add a comment to state that if so.

[felipepiovezan]

Also could we have an overlap between host and debugee memory addresses?

Probably, in the sense that these are all just numbers. But I'm not sure I follow the concern here: IRMemoryMap is supposed to track which address comes from where, so it's not clear why overlapping is a concern? It is the job of IRMemoryMap to think about overlaps when it is creating Allocations, not during WritePointerToMemory.

[felipepiovezan]

Address review comments

[DavidSpickett]

Does this change anything with regard to #134247 ?

I'm not sure. I don't think so because in that case the function pointer is probably just read from memory instead of being written to memory again.

Which reminds me, this should have a testcase, even if it's very artificial. It'll help explain how to hit this code and remind us later if we change how this is handled.

int check_non_address(void** ptr) {
 if (*ptr & ~top_byte_mask)
  return 1;

 return 0;
}

Along those lines. If you use the top byte you don't have to care about virtual address size or amount of pointer authentication bits used.

[felipepiovezan]

Does this change anything with regard to #134247 ?

I'm not sure. I don't think so because in that case the function pointer is probably just read from memory instead of being written to memory again.

Which reminds me, this should have a testcase, even if it's very artificial. It'll help explain how to hit this code and remind us later if we change how this is handled.

int check_non_address(void** ptr) {
 if (*ptr & ~top_byte_mask)
  return 1;

 return 0;
}

Along those lines. If you use the top byte you don't have to care about virtual address size or amount of pointer authentication bits used.

Can you elaborate on what that program is showing and how this PR would affect that?

Which reminds me, this should have a testcase, even if it's very artificial. It'll help explain how to hit this code and remind us later if we change how this is handled.

The test TestScriptedProcessEmptyMemoryRegion.py is exercising this patch, so much so that it started failing on the first iteration of this PR (the first commit has a note about this)

[jasonmolenda]

Looking at IRMemoryMap::WriteMemory() (I haven't read these methods in years, needed to refresh my memory), it has different behavior based on the Allocation m_policy which can be one of eAllocationPolicyHostOnly, eAllocationPolicyMirror, or eAllocationPolicyProcessOnly. The goal is to run any pointer that is written into actual process memory through Fix, because it will be used by a jitted expression running in native code, and cannot refer to the host-side-only fake addresses that IRMemoryMap may hand out. Maybe the m_data.GetByteSize() test is sufficient, but I think it's maybe clearer to test for m_policy != eAllocationPolicyHostOnly? I'm not sure what Mirror is used for, possibly expression results where we might want to take the address of them in inferior-memory, but for efficiency of display/use in lldb, also stored in lldb.

[DavidSpickett]

Can you elaborate on what that program is showing and how this PR would affect that?

That function, when called with a pointer to a pointer written to memory with WritePointerToMemory, would check whether that pointer had retained it's non-address bits. Without this change, it would see the non-address bits. With this change it would not.

(which is unrelated to the host/debugee problem the reland is also fixing)

I'm not sure how to craft an expression that will hit WritePointerToMemory though. Maybe you could use API calls to trigger WritePointerToMemory, then run an expression using the address it was written to.

In other words:

• Write a tagged pointer to memory via. WritePointerToMemory.
• Run an expression that calls the check function, passing the address of where that pointer is stored.
• If the check function sees non-address bits set, this PR is not working.
• If it does not, this PR is working as expected.

By doing the check using code in the debugee, you make sure that no other fix address call can get in the way. And if you change the strategy later, the test will break and remind us to change this particular call to fix address.

[felipepiovezan]

I'm out next week, but I'll have a look at that idea when I'm back!

[felipepiovezan]

@DavidSpickett in a different comment, you mentioned that

the AArch64 Linux bot has top byte ignore and pointer authentication available.

Do you know how to write a test targeting that architecture specifically? One of the main challenges I have right now is that this seems almost impossible to test. I can't write an expression whose variables will have metadata in their pointers.

[DavidSpickett]

You can manually set the top byte to some value, then in the expression call a function that checks for those bits. This does not require special code just a bunch of uintptr_t casts. You can do this on any AArch64 Linux, Top Byte Ignore has been enabled in userspace since the beginning.

lldb/test/API/linux/aarch64/non_address_bit_code_break/main.c is an example of adding the top byte. You could add a PAuth signature too, but you don't have to to test this. Top byte and PAC code are both considered "non-address bits" by lldb and if you just use TBI, it'll run in more places.

(including MacOS I think)

The tricky bit will be getting the expression to trigger writing the pointer to memory. I'd start with something like:

void* ptr = <the pointer you tagged in the application>; check_top_byte(ptr);

Where check_top_byte is a function in the application that does what it sounds like.

We're hoping that the ptr = forces a write to memory. It might not.

In which case, you could look for a way to trigger the write via. the API and then read it back? In fact if you can do that, it's easier overall. Have the application tag the pointer, read it into lldb, write it back to some new location and read that back. If it comes back untagged, this patch didn't work.

Whether you can hit this from the API, idk. I'll have a quick look now.

[felipepiovezan]

So that's the challenge, what the function itself does is irrelevant. In this PR, the code we're writing only affects this:

expr foo(some_var, do_soemthing(some_other_var))

In this case, WritePointerToMemory will be called on the address of some_var and some_other_var.
To reiterate the subtlety: on the address of those variables. So we can't artificially manipulate the pointers that reach WritePointerToMemory

[DavidSpickett]

Ok nothing in SBAPI directly. Only IRMemoryMap calls this.

If it only calls it when writing the address of a symbol, problem is that won't be tagged unless the ABI decided it was, certainly the debug info won't include tag bits.

How did you hit this problem in the first place? Some failure on arm64e I presume?

[DavidSpickett]

To reiterate the subtlety: on the address of those variables. So we can't artificially manipulate the pointers that reach WritePointerToMemory

Ok so this is true then:

If it only calls it when writing the address of a symbol, problem is that won't be tagged unless the ABI decided it was, certainly the debug info won't include tag bits.

So yeah, how did you hit this problem in the first place? Test failure on arm64e? I'd be ok with a test that only runs there if it is the only way to trigger this.

Could we do something like loading new symbol information using https://lldb.llvm.org/use/symbolfilejson.html that includes a tagged address? Idk if we can overwrite information that way though. We could somehow read back the address we want, load the binary without debug info then make a json file with just that address, tagged 🤣

...which is a bit much.

[felipepiovezan]

How did you hit this problem in the first place? Some failure on arm64e I presume?

Yeah, once I remove the calls in #150537, some pointers from expression evaluation were no longer being stripped of their arm64e signatures. Let me re-run the test suite without this patch on an arm64e target and see if I can draw inspiration from those failing tests. Will report back in a bit.

I don't think there are any upstream targets where this test would be able to run, though I don't mind merging a test guarded by the mac&arm64e triples.

[DavidSpickett]

....did not mean to click the update button, sorry!

I will try the JSON symbol file route. I think we can make a new tagged address symbol overlapping an existing one.

[jasonmolenda]

....did not mean to click the update button, sorry!

I will try the JSON symbol file route. I think we can make a new tagged address symbol overlapping an existing one.

I'm sure you remember seeing it, but I created one of these for a test case in #153922 lldb/test/API/functionalities/unwind/cortex-m-exception/binary.json

[DavidSpickett]

There is also lldb/test/API/functionalities/json/symbol-file/TestSymbolFileJSON.py which is almost what we want.

I got this to work:

{
    "triple": "aarch64--linux",
    "uuid": "A2180C69-2DC0-DE61-3689-5CDC84D0AB63-A898EE40",
    "sections": [
        {   
            "name": "globals",
            "type": "data",
            "address": 1297224342667202580,
            "size": 16
        }   
    ],  
    "symbols": [
        {
            "name": "a_global_tagged",
            "size": 8,
            "type": "data",
            "address": 1297224342667202580
        }   
    ]   
}

(note that the section address must also be tagged otherwise it won't match the symbol to it)

We can get all this info from the running program to build the file during the test.

(lldb) target symbol add ~/symbol.json
symbol file '/home/david.spickett/symbol.json' has been added to '/tmp/test.o'
(lldb) p &a_global
(void **) 0x0000aaaaaaab1014
(lldb) p &a_global_tagged
(void **) 0x1200aaaaaaab1014
(lldb) p (void*)return_ptr(&a_global)
(void *) 0x0000aaaaaaab1014
(lldb) p (void*)return_ptr(&a_global_tagged)
(void *) 0x1200aaaaaaab1014

I'm passing the address here otherwise we have no way to know the bits were or were not stripped.

I think that's not enough though right? We need some function(variable_name) where loading variable_name via an untagged pointer fails. Or via a tagged pointer?

Anyway you see the idea here. Tell me if that could be made to work.

[felipepiovezan]

This seems promising!

(lldb) target symbol add ~/symbol.json

How did you get this to work? As soon as I do this, I lose all the other symbols in my original program, including the function I want to call (return_ptr in your example).

[DavidSpickett]

Could it be because I run to main and then load the other symbols? Also I forgot to mention why I added the cast:

(lldb) run
Process 441637 launched: '/tmp/test.o' (aarch64)
Process 441637 stopped
* thread #1, name = 'test.o', stop reason = breakpoint 1.1
    frame #0: 0x0000aaaaaaaa0728 test.o`main at test.c:5:21
   2   	
   3   	void* return_ptr(void* ptr) { return ptr; }
   4   	
-> 5   	int main() { return 0; }
(lldb) p return_ptr
(void *(*)(void *)) 0x0000aaaaaaaa0714 (test.o`return_ptr at test.c:3:29)
(lldb) target symbol add ~/symbol.json
symbol file '/home/david.spickett/symbol.json' has been added to '/tmp/test.o'
(lldb) p return_ptr
         ˄
         ╰─ error: 'return_ptr' has unknown type; cast it to its declared type to use it

Something clearly happens to the previous symbols. Maybe they are removed and we know enough to say that return_ptr is a function but no more than that and the expression evaluator does its best to make it work.

[DavidSpickett]

It still knows where the global variable is:

(lldb) p a_global
(void *) 0x0000000000000000

But again this could be because that's known from something other than the DWARF.

[felipepiovezan]

Sorry for the delay, long weekend here in the US.

Ok, I think I finally got it working, mostly? I was facing two issues: 1) v gets confused by what we were doing, but expr doesn't; 2) Older versions of LLDB would fail to find the symbol -- even with expr -- after target symbol add...

So I have this:

{
    "triple": "arm64-apple-macosx13.0.0",
    "uuid": "C8576DD6-9D22-48ED-9890-FB216DEE329F",
    "type": "executable",
    "sections": [
        {   
            "name": "__DATA",
            "type": "data",
            "address": 1297224342667202580,
            "size": 16
        }   
    ],  
    "symbols": [
        {
            "name": "myglobal_json",
            "size": 8,
            "type": "data",
            "address": 1297224342667202580
        }   
    ]   
}

And this main.c:

#include <assert.h>
#include <stdint.h>
#include <stdio.h>


int myglobal = 0;

uint64_t get_high_bits(void *ptr) {
  uint64_t mask = ~((1ULL << 48) - 1);
  uint64_t ptrtoint = (uint64_t)ptr;
  uint64_t high_bits = ptrtoint & mask;
  printf("Higher bits are = %llx\n", high_bits);
  return high_bits;
}

int main (){
  int x = 42;
  assert(0 == get_high_bits(&x));
  assert(0 == get_high_bits(&myglobal));
  return 0; //breakhere
}

The contents of main are irrelevant here, more of a sanity check (the second assert should fail on an arm64e target).

So we load the json file, add the new symbol (I opted for a new symbol instead of overwriting the existing one), and run it through the expression evaluator.
WITHOUT this patch, this is what we get:

(lldb) target module add test.json
(lldb) expr &myglobal_json
(void **) $2 = 0x1200aaaaaaab1014
(lldb) expr get_high_bits(&myglobal_json)
Higher bits are = 1200000000000000
(uint64_t) $3 = 1297036692682702848

WITH this patch:

(lldb) target module add test.json
(lldb) expr &myglobal_json
(void **) $2 = 0x00002aaaaaab1014
(lldb) expr get_high_bits(&myglobal_json)
Higher bits are = 0
(uint64_t) $3 = 0

This looks like what we expect, right? If so, I will go ahead and try to make this into an API test!

[DavidSpickett]

Yes that's right. I'm still a bit unclear how it makes its way to WritePointerToMemory, but as long as it'll break if we change things, that's the point.

I think it is that:

• Expression evaluation needs to pass void* ptr to the function.
• To do that we allocate a stack slot in the expression wrapper, and write it there - using WritePointerToMemory.
• The wrapper loads it back from memory to call the function.

Yes, because the wrapper itself has no arguments. So this isn't like a normal compilation where you put the pointer in a register, everything has to be on the stack first.

Also stylistic point, use uintptr_t for the pointer manipulation. No practical difference in this case, but a bit more self-describing.

A new variable name is a good idea too, if you check both in the test, if it does fail, the difference between the 2 will be a big clue to figuring it out.

[DavidSpickett]

Also is there a store reference to memory and if so does it do its own thing or call store pointer?

[felipepiovezan]

Your guess is right. Enabling log enable lldb expr, we can see the IR generated for expr get_high_bits(&myglobal_json)

@"_ZGVZ12$__lldb_exprPvE19$__lldb_expr_result" = internal global i8 0, align 1

; Function Attrs: convergent noinline nounwind optnone
define void @"_Z12$__lldb_exprPv"(ptr %"$__lldb_arg") #0 {
entry:
  %0 = getelementptr i8, ptr %"$__lldb_arg", i32 8
  %1 = getelementptr i8, ptr %"$__lldb_arg", i32 0
  %2 = load ptr, ptr %1, align 8
  %"$__lldb_arg.addr" = alloca ptr, align 8, !clang.decl.ptr !8
  store ptr %"$__lldb_arg", ptr %"$__lldb_arg.addr", align 8
  %guard.uninitialized = icmp eq i8 0, 0
  br i1 %guard.uninitialized, label %init.check, label %init.end, !prof !9

init.check:                                       ; preds = %entry
  %3 = load ptr, ptr %0, align 8, !nonnull !10, !align !11
  %call = call i64 @get_high_bits(ptr %3) #2
  store i64 %call, ptr %2, align 8
  br label %init.end

init.end:                                         ; preds = %init.check, %entry
  ret void
}

%"$__lldb_arg is the memory region allocated by IRMemoryMap.
%0 is the offset where the address of myglobal_json's address was stored by WritePointerToMemory (after going through FixAddress). We load %0 into %3 and pass the result directly to get_high_bits

[felipepiovezan]

Also is there a store reference to memory and if so does it do its own thing or call store pointer?

could you elaborate? Not sure what you meant by "store reference to memory". Maybe the question is answered by the IR above?

[felipepiovezan]

Added a test!

[felipepiovezan]

I think I need to make this test run only on apple platforms?

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.