[Lldb-commits] [PATCH] D105630: [lldb][AArch64] Refactor memory tag range handling

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added a comment.

Is this final version or you are still doing refactoring. Also can you kindly order its child and parent revs in current tag-write patch series.



================
Comment at: lldb/source/Commands/CommandObjectMemoryTag.cpp:70
     Process *process = m_exe_ctx.GetProcessPtr();
     llvm::Expected<const MemoryTagManager *> tag_manager_or_err =
+        process->GetMemoryTagManager();
----------------
DavidSpickett wrote:
> DavidSpickett wrote:
> > omjavaid wrote:
> > > If we also pull out the SupportsMemoryTagging out of GetMemoryTagManager we can directly return a tag manager pointer here.
> > > 
> > > Also SupportsMemoryTagging may also run once for the life time of a process? We can store this information is a flag or something.
> > > 
> > > 
> > > If we also pull out the SupportsMemoryTagging out of GetMemoryTagManager we can directly return a tag manager pointer here.
> > 
> > Sure, that way `GetMemoryTagManager` does exactly what it says on the tin. I can just make a utility function in `CommandObjectMemoryTag.cpp` to reduce the duplication of checking both, when adding the write command.
> > 
> > Will do that in the next update.
> > 
> > > Also SupportsMemoryTagging may also run once for the life time of a process? We can store this information is a flag or something.
> > 
> > It already is in that for the GDB remote it'll only send a `qSupported` the first time it (or any other property) is asked for. So we go through a few function calls but that's it.
> On second thought I'd rather leave `GetMemoryTagManager` as it is.
> 
> If we split the checks up we'll have the same 2 checks repeated in:
> * `memory tag read`, `memory tag write` (same file so you could make a helper function)
> * `Process::ReadMemoryTags`, `Process::WriteMemoryTags` (again you could make a helper but...)
> 
> That helper would be what `GetMemoryTagManager` is now. Leaving it as is saves duplicating it.
> 
> I thought of simply only checking `SupportsMemoryTagging` in Process::ReadMemoryTags. Problem with that is that you would never get this error if your remote wasn't able to show you MTE memory region flags. And you'd hope the most obvious checks would come first.
> 
> The other reason to split the 2 checks is so `GetMemoryTagManager` could be called from the API to get a manager even if the remote didn't support MTE. Which seems like a very niche use case, perhaps you've got some very old remote but are somehow running an MTE enabled program on it? Seems unlikely.
> 
> If we find a use case later when I work on the API then it'll be easy to switch it around anyway.
Agreed no strong reason to further split GetMemoryTagManager. API use-case may never be utilized and we can always come back and refactor if need arises.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105630