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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D87442#2300987 <https://reviews.llvm.org/D87442#2300987>, @labath wrote:

> I think this patch for its test coverage.

And by "think", I meant "like". :)

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

>> 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.

Yes, that's pretty much it. I sympathise with wanting to match the native tools, but that's not something lldb is very good at right now (and there's an opposite drive to make lldb behavior consistent across platforms). For now I'd just put on that and have lldb choose an "os-independent" flag name which "happens" to match the linux name (one of the advantages of being first).

That said, with this and @jasonmolenda's change to memory regions, I think the description of a single memory region becomes too complicated to reasonably fit on a single line. I think we might want to change the output of `memory region` to span multiple lines, at which point, the string "memory tagging: enabled" might not look too bad (we could add headings for other properties too).



================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1301-1302
   Status Result;
-  ParseLinuxMapRegions(BufferOrError.get()->getBuffer(),
-                       [&](const MemoryRegionInfo &Info, const Status &ST) {
-                         if (ST.Success()) {
-                           FileSpec file_spec(Info.GetName().GetCString());
-                           FileSystem::Instance().Resolve(file_spec);
-                           m_mem_region_cache.emplace_back(Info, file_spec);
-                           return true;
-                         } else {
-                           m_supports_mem_region = LazyBool::eLazyBoolNo;
-                           LLDB_LOG(log, "failed to parse proc maps: {0}", ST);
-                           Result = ST;
-                           return false;
-                         }
-                       });
-  if (Result.Fail())
-    return Result;
+  LinuxMapCallback callback = [&](const llvm::Optional<MemoryRegionInfo> &Info,
+                                  const Status &ST) {
+    if (ST.Success()) {
----------------
If we're changing the signature, we might as well make this `Expected<MemoryRegionInfo>`.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1323
+      return Result;
+    }
+  } else {
----------------
We normally don't put braces around simple if statements like this: <http://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements>


================
Comment at: lldb/source/Plugins/Process/Utility/LinuxProcMaps.cpp:160
+    // If this line is a property line
+    if (name.find(' ') == llvm::StringRef::npos) {
+      if (region) {
----------------
`name.contains(' ')`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87442