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

[David Spickett via Phabricator via lldb-commits]

DavidSpickett marked 11 inline comments as done.
DavidSpickett added inline comments.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1319
+  // Linux kernel since 2.6.14 has /proc/{pid}/smaps
+  // if CONFIG_PROC_PAGE_MONITOR is enabled
+  auto BufferOrError = getProcFile(GetID(), "smaps");
----------------
labath wrote:
> Just for my own education: Does this mean that we will need to maintain both branches in perpetuity, as it is always possible to build kernels which don't have /smaps ?
Unfortunately yes. The good part is that the header lines remain the same so we can share that code.
(gdb already uses smaps and has a similar fallback route)


================
Comment at: lldb/source/Plugins/Process/Utility/LinuxProcMaps.cpp:134
+  // Note: llvm::regex doesn't do character classes
+  RegularExpression expr("^[ ]*([A-za-z0-9_]+)[ ]*:(.*)");
+  if (expr.Execute(line, &matches)) {
----------------
labath wrote:
> Compiling the regex for each line is pretty wasteful. I don't think a regex is really needed here. I think you could just split the line on the first `:` character and check that lhs does not contain spaces.
Done and merged into ParseLinuxSMapRegions since it was much simpler.

(I couldn't find any hard guarantee that there won't be leading spaces but from reading kernel source it seems
that it might pad values but not names. So "VmFlags:         re" could happen but not "     VmFlags: re")


================
Comment at: lldb/unittests/Process/Utility/CMakeLists.txt:5
+  LINK_LIBS
+     lldbPluginProcessLinux
+  )
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87442