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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think this patch for its test coverage.

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?



================
Comment at: lldb/source/API/SBMemoryRegionInfo.cpp:127
+  Stream &strm = flags.ref();
+  strm.Printf("%s", m_opaque_up->GetFlags().c_str());
+
----------------
`strm << m_opaque_up->GetFlags()`


================
Comment at: lldb/source/Commands/CommandObjectMemory.cpp:1736
+            result.AppendMessageWithFormatv("flags: {0}",
+                                            range_info.GetFlags().c_str());
+          }
----------------
drop `.c_str()`


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1308
+      m_mem_region_cache.emplace_back(Info, file_spec);
+      Result = ST;
+      return true;
----------------
Is this needed. Given that the parsing will stop at the first error, `Result` should always be empty (success) at this point)...


================
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");
----------------
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 ?


================
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)) {
----------------
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.


================
Comment at: lldb/source/Plugins/Process/Utility/LinuxProcMaps.cpp:19
 
+enum MapKind { eMaps, eSMaps };
+
----------------
omjavaid wrote:
> May be consider converting this into a class enum.
And get rid of the leading `e`.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2498
+      response.PutCString("flags:");
+      response.PutCString(flags_str.c_str());
+      response.PutChar(';');
----------------
drop `.c_str()`


================
Comment at: lldb/source/Target/MemoryRegionInfo.cpp:21
+  while (flags.size()) {
+    flags = flags.drop_while(iswspace);
+    std::tie(flag, flags) = flags.split(' ');
----------------
`flags = flags.ltrim();`


================
Comment at: lldb/unittests/Process/Utility/CMakeLists.txt:5
+  LINK_LIBS
+     lldbPluginProcessLinux
+  )
----------------
Why is this needed?


================
Comment at: lldb/unittests/Process/Utility/CMakeLists.txt:8-9
+
+target_include_directories(LinuxProcMapsTests PRIVATE
+  ${LLDB_SOURCE_DIR}/source/Plugins/Process/Utility)
----------------
Just include the file as `Plugins/Process/Utility/LinuxProcMaps.h`


================
Comment at: lldb/unittests/Process/Utility/LinuxProcMapsTest.cpp:38
+  void SetUp() override {
+    callback = [&](const MemoryRegionInfo &Info, const Status &ST) {
+      if (ST.Success()) {
----------------
`&`-capture here is dangerous. Capture what you need (this?) explicitly.


================
Comment at: lldb/unittests/Process/Utility/LinuxProcMapsTest.cpp:50-51
+  void check_regions(LinuxProcMapsTestParams params) {
+    ASSERT_EQ(std::get<1>(params).size(), regions.size());
+    ASSERT_EQ(std::get<1>(params), regions);
+    ASSERT_EQ(std::get<2>(params), err_str);
----------------
`ASSERT_THAT(std::get<1>(params), testing::ContainerEq(regions));`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87442