[Lldb-commits] [PATCH] D95601: [lldb][AArch64] Add MTE memory tag reading to lldb-server

Mon Feb 15 14:32:41 PST 2021

omjavaid added a comment.

In D95601#2563613 <https://reviews.llvm.org/D95601#2563613>, @DavidSpickett wrote:

>   To get the review going consider further splitting up these patches into separate chunks containing, for example MemoryTagHandler API and related unit tests, GDB remote addons, ptrace work etc. Also add reference to GDB mailing list discussion where design of qMemTag packet has been discussed.
>
> Will do. I erred on the side of larger patches so you could see the pieces connect in the same review.
>
>   Soft suggestion to rename MemoryTagHandler, IMO it sounds more like a routine rather than a set of helpers used for manipulating memory tags. Also lldb/include/lldb/Target doesnt seem like a good place for helper functions as MemoryTagHandler is not used as Memory Tag container class. May be consider putting it under source/Plugins/Process/Utility
>
> How about "MemoryTagInterface"? It's very similar to a Java "interface" class.

MemoryTagHandler class does indeed look like an interface class  example of an interface class in LLDB code is RegisterInfosInterface which serves as an interface of varies arch dependent register infos. But MemoryTagHandler serves more like a provider or manager rather than a container

May be you can use MemoryTagManager or MemoryTagUtils but then this just a soft comment. You can keep the existing name or change it to interface or anything you feel appropriate.

> I think my reasoning for putting the header in lldb/Target was because it's used in the client and the server, and one of them tends not to include from the latter location. I'll double check.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95601