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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
