This is an archive of the discontinued LLVM Phabricator instance.

[lldb][AArch64] Add memory tag writing to lldb
ClosedPublic

Authored by DavidSpickett on Jun 30 2021, 3:51 AM.

Details

Summary

This adds memory tag writing to Process and the
GDB remote code. Supporting work for the
"memory tag write" command. (to follow)

Process WriteMemoryTags is similair to ReadMemoryTags.
It will pack the tags then call DoWriteMemoryTags.
That function will send the QMemTags packet to the gdb-remote.

The QMemTags packet follows the GDB specification in:
https://sourceware.org/gdb/current/onlinedocs/gdb/General-Query-Packets.html#General-Query-Packets

Note that lldb-server will be treating partial writes as
complete failures. So lldb doesn't need to handle the partial
write case in any special way.

Diff Detail

Event Timeline

DavidSpickett created this revision.Jun 30 2021, 3:51 AM
DavidSpickett requested review of this revision.Jun 30 2021, 3:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2021, 3:51 AM
omjavaid added inline comments.Jul 6 2021, 3:35 PM
lldb/include/lldb/Target/MemoryTagManager.h
35 ↗(On Diff #355500)

I was wondering if you can explain reason for hosting this struct. Is there a association between MemoryTagManager and Tag Range.

I think same tag manager was associated with the whole of process address space? so why host tag manager pointer along with the range when we already have a pointer to process. This implies there could be different tag managers for different ranges? Our initial implementation introduced per architecture tag manager and for Process AArch64 we can use AArch64 Tag Manager for all our tag ranges. This appears to have over complicated range expansion.

DavidSpickett added inline comments.Jul 7 2021, 1:27 AM
lldb/include/lldb/Target/MemoryTagManager.h
35 ↗(On Diff #355500)

The comment here is a bit misleading re. the "validity" of the manager. The manager itself is valid if you've got tagging, wherever it might be, you're right there.

The commit message has a better justification:

To save the commands then redoing that alignment
both memory tag manager methods now return a struct
of the range we checked and the manager itself.

So this is not "this manager is valid for this range", it's "here's a manager and an aligned range". Since we have to align the original range, then check the memory flags, then finally return the manager. Might as well give them the range too.

I agree that the comment here doesn't explain that well, I'll make it more precise.

omjavaid added inline comments.Jul 7 2021, 3:04 AM
lldb/include/lldb/Target/MemoryTagManager.h
35 ↗(On Diff #355500)

The question I have is why we have to return the manager pointer with our expanded ranges. Tag ranges are per-process rather than per-tag-manager and we can get tag manager by keeping a reference to it in the process class then why return a manager alongside the expanded range.

lldb/source/Target/Process.cpp
6071

The point I was trying to establish above is that GetMemoryTagManagerImpl function here may remain as it was i-e GetMemoryTagManager. It could only run once on Process object creation. We know our process architecture so we can host a pointer to relevent tag manager in Process class. All the tag ranges corresponding to the current process address space should be able to get a pointer to MemoryTagManager as was being done previously.

6089

Main operation of this function is doing the expansion of ranges according to granule while the function name suggests that we are getting memory unique tag manager for addr, granules. Name doesnt even suggest that we are actually here to do range alignment/expansion.

DavidSpickett added inline comments.Jul 7 2021, 3:27 AM
lldb/source/Target/Process.cpp
6071

I see what you mean. I'm going to try this on top of main as a new change, then I'll refactor this based on that if it works out.

DavidSpickett added inline comments.Jul 8 2021, 1:59 AM
lldb/source/Target/Process.cpp
6071

It could only run once on Process object creation.

Turns out that the arch object, where the plugin is stored, changes 2/3 times between process creation and loading the program file. So we could store the pointer but we'd have to invalidate it on all these events. Ultimately we'd spend the time saved doing invalidation.

Which is probably why things like Target::GetCallableLoadAddress just get a new pointer each time.

You gave me some good ideas about simplifying this whole process though so I'm still going to work on that.

DavidSpickett marked an inline comment as not done.Jul 8 2021, 1:59 AM

Rebase onto the earlier refactoring patch.

Now GetMemoryTagManager just deals with the manager,
no range handling. So this patch just adds WriteMemoryTags.

"memory tag write" will use MakeTaggedRange from
the memory tag manager instead of relying on the methods
I had here before.

DavidSpickett marked an inline comment as done.Jul 14 2021, 1:40 AM
DavidSpickett edited the summary of this revision. (Show Details)Jul 14 2021, 1:42 AM

Rebase, update use of SendPacketAndWaitForResponse.

omjavaid accepted this revision.Jul 23 2021, 2:39 AM
This revision is now accepted and ready to land.Jul 23 2021, 2:39 AM
This revision was landed with ongoing or failed builds.Jul 27 2021, 7:18 AM
This revision was automatically updated to reflect the committed changes.