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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D87442#2334786 <https://reviews.llvm.org/D87442#2334786>, @DavidSpickett wrote:

>> Woa, back up. I though you were just going to add the one flag you need right now... :(
>
> I was going for the showing flags as a feature of the "memory region" command then later adding the memory tagging flag to that.
> I see your point though and yeah I don't need all the flags to unblock mte.
>
> With the perspective I was coming from, adding a set of getter/setter for 20ish flags wasn't an appealing idea:
>
>   bool m_memory_tagged;
>   OptionalBool GetMemoryTagged() const { return m_memory_tagged; }
>   void SetMemoryTagged(bool v) { m_memory_tagged = v; }
>   <x20>
>
> If it's just mt then I just add another set, can always merge it with a flags (plural) interface later if we accumulate more.
>
> So if it makes sense to you, I will:
>
> - add only "mt" flag, with it's own getter/setter
> - keep the protocol changes (but only recognise the 1 flag)
> - keep the extra testing, use of expected etc. where it still applies
> - add tests to run on a memory tagging enabled kernel with qemu
>
> Then we can at least agree in principle, even if this doesn't land until the new tagging commands have also been reviewed. (which is fine by me, but I don't have them ready yet)
>
> Thanks for all your comments so far!

Yep, that sounds like a plan.



================
Comment at: lldb/source/Plugins/Process/Utility/LinuxProcMaps.h:18
 
-typedef std::function<bool(const lldb_private::MemoryRegionInfo &,
-                           const lldb_private::Status &)> LinuxMapCallback;
+typedef llvm::Expected<lldb_private::MemoryRegionInfo> ExpectedMemoryRegionInfo;
+typedef std::function<bool(ExpectedMemoryRegionInfo)> LinuxMapCallback;
----------------
We don't usually typedef expecteds like this, and the result is not much shorter than the original.


================
Comment at: lldb/unittests/Process/Utility/CMakeLists.txt:5
+  LINK_LIBS
+     lldbPluginProcessLinux
+  )
----------------
DavidSpickett wrote:
> labath wrote:
> > Why is this needed?
> It won't link without it. I followed the format of the other tests e.g. unittests/Process/POSIX/CMakeLists.txt
> (you header suggestion does work fine though)
What will not link? This definitely can't be the right solution as lldbPluginProcessLinux is not even being built on non-linux hosts (but Plugins/Process/Utility is).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87442