DavidSpickett added inline comments.
================
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:
> 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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D105630/new/
https://reviews.llvm.org/D105630
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
