[Lldb-commits] [PATCH] D105630: [lldb][AArch64] Refactor memory tag range handling

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added a comment.

I think this approach is a lot better given that the process of tagged ranged calculation deserved a separation and may be more explanation too. I guess somewhere in the comments of same function you can explain interaction between memory regions and tagged ranges. It will become a bit more complicated for a reader who have no knowledge about memory tag range and disjoint region.



================
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(
----------------
I couldnt understand this point in comment  "have an untagged base address" 


================
Comment at: lldb/source/Commands/CommandObjectMemoryTag.cpp:70
     Process *process = m_exe_ctx.GetProcessPtr();
     llvm::Expected<const MemoryTagManager *> tag_manager_or_err =
+        process->GetMemoryTagManager();
----------------
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.




================
Comment at: lldb/source/Commands/CommandObjectMemoryTag.cpp:71
     llvm::Expected<const MemoryTagManager *> tag_manager_or_err =
-        process->GetMemoryTagManager(start_addr, end_addr);
+        process->GetMemoryTagManager();
 
----------------
should we process pointer validity check before this call?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105630/new/

https://reviews.llvm.org/D105630