omjavaid added inline comments.
================
Comment at: lldb/include/lldb/Target/MemoryTagManager.h:35
+ // alignment/expansion over again.
+ struct TagManagerWithRange {
+ const MemoryTagManager *manager;
----------------
DavidSpickett wrote:
> omjavaid wrote:
> > 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.
> >
> >
> 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.
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.
================
Comment at: lldb/source/Target/Process.cpp:6089
+llvm::Expected<MemoryTagManager::TagManagerWithRange>
+Process::GetMemoryTagManagerForGranules(lldb::addr_t addr, size_t granules) {
+ return GetMemoryTagManagerImpl(
----------------
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.
================
Comment at: lldb/source/Target/Process.cpp:6108
+ range_callback) {
Architecture *arch = GetTarget().GetArchitecturePlugin();
const MemoryTagManager *tag_manager =
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D105181/new/
https://reviews.llvm.org/D105181
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
