DavidSpickett added inline comments.

================
Comment at: lldb/include/lldb/Target/MemoryTagManager.h:35
+  // alignment/expansion over again.
+  struct TagManagerWithRange {
+    const MemoryTagManager *manager;
----------------
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.


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

Reply via email to