This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Remove non address bits when looking up memory regions
ClosedPublic

Authored by DavidSpickett on May 19 2021, 3:53 AM.

Details

Summary

On AArch64 we have various things using the non address bits
of pointers. This means when you lookup their containing region
you won't find it if you don't remove them.

This changes Process GetMemoryRegionInfo to a non virtual method
that uses the current ABI plugin to remove those bits. Then it
calls DoGetMemoryRegionInfo.

That function does the actual work and is virtual to be overriden
by Process implementations.

A test case is added that runs on AArch64 Linux using the top
byte ignore feature.

Diff Detail

Event Timeline

DavidSpickett created this revision.May 19 2021, 3:53 AM
DavidSpickett requested review of this revision.May 19 2021, 3:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2021, 3:53 AM
DavidSpickett added subscribers: vsk, jasonmolenda.

One of the main reasons the MTE commands need to handle addresses is to lookup memory regions, this generalises the handling of that. I'm not sure yet if the "GetStuff" in process and "DoGetStuff" in subclasses is viable for all the areas where non address bits are important. If there's a lot of instances maybe we'd go in the direction of handling "smart" address parameters that know what their address bits are but that's more involved.

I need to add a corefile test but I'm pretty sure the ABI plugin approach should work there. (Omair was able to use a core file in https://reviews.llvm.org/D99944)

Also there is the issue that in GetMemoryRegion we don't know if it's a code or data pointer which is relevant for PAC. (though you could apply both masks, assuming the VA size is the same for code and data, seems reasonable)

@jasonmolenda / @vsk You might have something downstream that tackles the same issue. I wonder if we're on the same track.

Now that we are stripping away top byte is there any information that may be useful for the remote side and we are removing that on the host side. I am thinking why we should strip top byte on host side rather than making it the responsibility of the remote end?

DavidSpickett added a comment.EditedMay 26 2021, 4:09 AM

Now that we are stripping away top byte is there any information that may be useful for the remote side and we are removing that on the host side. I am thinking why we should strip top byte on host side rather than making it the responsibility of the remote end?

I don't think there'd be anything in there the remote needs. Not for qMemoryRegionInfo.

lldb-server will need to remove memory tags somehow anyway (because the tag packet spec says you can send tagged addresses) so we could have it do all of it. My unknown there is "non remote" kinda targets like ELF cores or minidumps, does using the ABI for just those get for those targets get messy.

One thing that you might think of would be the top bits set for kernel alloations. However if we're using the masks and TBI correctly those will be left intact.

Now that we are stripping away top byte is there any information that may be useful for the remote side and we are removing that on the host side. I am thinking why we should strip top byte on host side rather than making it the responsibility of the remote end?

I don't think there'd be anything in there the remote needs. Not for qMemoryRegionInfo.

lldb-server will need to remove memory tags somehow anyway (because the tag packet spec says you can send tagged addresses) so we could have it do all of it. My unknown there is "non remote" kinda targets like ELF cores or minidumps, does using the ABI for just those get for those targets get messy.

This looks ok but keep in mind it has effect on ProcessWinodws and Minidump that we are not currently testing.

omjavaid accepted this revision.Jun 16 2021, 2:25 PM
omjavaid added inline comments.
lldb/source/Target/Process.cpp
5902

Yes this is ok for now as both Data and Code masks are same in case of Linux. If anything changes in future we ll take care of that at that time.

This revision is now accepted and ready to land.Jun 16 2021, 2:25 PM
DavidSpickett added inline comments.Jun 17 2021, 1:46 AM
lldb/source/Target/Process.cpp
5902

Cool. I was also thinking that if they were different you could just apply both masks. Since the virtual address size will be the same for either type of pointer.

I'll remove this comment.

  • Move DoGetMemoryRegionInfo into protected section, GetMemoryRegionInfo is the public part.
  • Fix a comment typo.
DavidSpickett edited the summary of this revision. (Show Details)Jun 18 2021, 3:20 AM

Do you think this needs a test with a core file as well?

Do you think this needs a test with a core file as well?

I guess it wont be needed. Included test is fine to check whether we remove non-address bits or not. Rest functionality does not change for elf-core.

  • Rebase onto main
  • Add DoGetMemoryRegionInfo for ScriptedProcess

I'm going to build this and the follow up patch on Windows
to get some more coverage.

Matt added a subscriber: Matt.Aug 10 2021, 1:24 PM

Rebase onto latest main.

Remove pointer auth from the test. This means we can run it on current buildbots
more easily. There are other tests that look at pauth specifically, here we just
care that the ABI plugin was queried at all.

DavidSpickett edited the summary of this revision. (Show Details)Nov 3 2021, 3:33 AM

Update comments.

Change ProcessDebugger to DoGetMemoryRegionInfo also, to reduce confusion.

thakis added a subscriber: thakis.Nov 3 2021, 5:33 AM

Looks like this breaks the build on Windows: http://45.33.8.238/win/48210/step_4.txt

Please take a look.

Looks like this breaks the build on Windows

Should be fixed now.