[Lldb-commits] [PATCH] D87442: [lldb] Show flags for memory regions

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

> The thing I'm not so sure about is the verbatim passing of the os-specific flags. That's nice for the purpose of displaying them to the user, but it can make things messy for programatic use, particularly if multiple platforms start using these. What's do you intend to do with these flags? Like how many of them are you going to actually use, and for what purpose?

At this point I only need to get the "mt" flag that shows whether a region has memory tagging enabled. This will be used by lldb internally and anyone checking for memory tagging with "memory region".

Some background on the design...

With the current implementation:

- We don't have to translate the flags per OS to show them.
- Has<flag>() needs to check for OS specific names, which balances things out. (you've got an OS specific switch either way)
- The gdb protocol packet just contains the names as strings.
- OSes that don't present flags as strings directly in their API have to provide string versions.

At first I did have a generically named bitset of flags internally. This means that:

- In the "memory region" output the user can't tell the difference between an unrecognised flag, or a flag that's not enabled. Since we'd need to update lldb to handle the new flag. (a rare occurrence now that I think about it)
- When "memory region" prints we'd need to translate those flags to OS specific names, or use long generic names like "memory tagging: enabled". (I prefer the former so you could compare it directly to the native tools)
- A region object's Has<Flag>() is the same code regardless of OS.
- The gdb protocol packet would contain generic versions of the flag names.

Now that I wrote that the second idea does look cleaner. New flags are going to be infrequent and internal calls won't have to care about what OS they're on to query flags.

Is that the direction you were thinking of? If so I'll rework this to do that instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87442