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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
