DavidSpickett added a comment.
> It will become a bit more complicated for a reader who have no knowledge
> about memory tag range and disjoint region.
I've added/updated some of the comments in `MakeTaggedRange`. Maybe you can
highlight any bits that aren't clear and I'll make sure they get comments too.
================
Comment at: lldb/include/lldb/Target/MemoryTagManager.h:66
+ // If so, return a modified range. This will be expanded to be
+ // granule aligned and have an untagged base address.
+ virtual llvm::Expected<TagRange> MakeTaggedRange(
----------------
omjavaid wrote:
> I couldnt understand this point in comment "have an untagged base address"
This is noted for 2 reasons:
* Callers don't need to remove tags before displaying the range (memory tag
read).
* There's potentially 2 different tags, begin/end, so removing them is the
easiest way to make (or rather not make) that choice.
You could keep the base tagged the same as `addr` but the code works out
simpler if you don't and all it means is that `memory tag read` has to remove
the tags itself. If it turns out that most callers want it tagged then sure
I'll change the behaviour.
================
Comment at: lldb/source/Commands/CommandObjectMemoryTag.cpp:70
Process *process = m_exe_ctx.GetProcessPtr();
llvm::Expected<const MemoryTagManager *> tag_manager_or_err =
+ process->GetMemoryTagManager();
----------------
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.
================
Comment at: lldb/source/Commands/CommandObjectMemoryTag.cpp:71
llvm::Expected<const MemoryTagManager *> tag_manager_or_err =
- process->GetMemoryTagManager(start_addr, end_addr);
+ process->GetMemoryTagManager();
----------------
omjavaid wrote:
> should we process pointer validity check before this call?
Covered by `eCommandRequiresProcess` in the constructor.
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
