[Lldb-commits] [PATCH] D112824: [lldb][AArch64] Add MakeTaggedRanges to MemoryTagManager

[David Spickett via Phabricator via lldb-commits]

DavidSpickett marked 2 inline comments as done.
DavidSpickett added inline comments.


================
Comment at: lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp:148
+  // For the logic to work regions must be in ascending order.
+  // We're going to assume that there are no overlapping regions
+  // and that each region is granule aligned already.
----------------
omjavaid wrote:
> so what would be the behavior if regions do overlap. Should we return an error rather than keep going?
I've addressed this by checking up front for overlaps in the list. We might see this if:
* there is invalid data from the remote
* the remote is using a memory protection unit that allows overlaps (Arm M class allows this)

Afaik no hosted OS would let you have two mappings that use the same range of virtual memory.

There's no good way to reconcile the overlap so an error is what I've gone with. I'll handle this error in the later patch to DumpDataExtractor.

It is a shame to walk the regions up front but as mentioned in the comment I found some corner cases that lead to some ugly code that would have its own drawbacks.

Given that the eventual goal here is "memory read <...> --show-tags" which is opt in, and we expect a fairly small amount of regions, I think the compromise is worth it.

(if M class ever got memory tagging we can make this smarter overall)


================
Comment at: lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp:160
+  // we must use an untagged address.
+  MemoryRegionInfo::RangeType range(RemoveNonAddressBits(addr), len);
+  range = ExpandToGranule(range);
----------------
omjavaid wrote:
> RemoveNonAddressBits hard-coded but symbols may resolve to higher order bits containing PACs. For now I only came across code pointers with PACs. But if you suspect code regions can be inputs to this function then may be make accommodating changes probably in separate patch.
The end goal here is "memory read <...> --show-tags" and that command will have to use the ABI plugin in any case so the addresses this gets will be PAC/TBI/MTE free already.

So for now this should be addressed by https://reviews.llvm.org/D103626. (once I convince myself which address should be show in the output but that's not for this issue)

I agree that there is a bit of a conflict here with the ABI plugin removing everything vs the MemoryTagManager removing only the tag. However I think that it will work fine in this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112824