[Lldb-commits] [PATCH] D94629: [LLDB] MinidumpParser: Prefer executable module even at higher address

[Joseph Tremoulet via Phabricator via lldb-commits]

JosephTremoulet updated this revision to Diff 316498.
JosephTremoulet added a comment.

- Fix comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94629

Files:
  lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
  lldb/unittests/Process/minidump/MinidumpParserTest.cpp


Index: lldb/unittests/Process/minidump/MinidumpParserTest.cpp
===================================================================
--- lldb/unittests/Process/minidump/MinidumpParserTest.cpp
+++ lldb/unittests/Process/minidump/MinidumpParserTest.cpp
@@ -792,6 +792,47 @@
   EXPECT_EQ(0x400d0000u, filtered_modules[0]->BaseOfImage);
 }
 
+TEST_F(MinidumpParserTest, MinidumpDuplicateModuleMappedSecondHigh) {
+  ASSERT_THAT_ERROR(SetUpFromYaml(R"(
+--- !minidump
+Streams:
+  - Type:            ModuleList
+    Modules:
+      - Base of Image:   0x400d3000
+        Size of Image:   0x00002000
+        Module Name:     '/usr/lib/libc.so'
+        CodeView Record: ''
+      - Base of Image:   0x400d0000
+        Size of Image:   0x00001000
+        Module Name:     '/usr/lib/libc.so'
+        CodeView Record: ''
+  - Type:            LinuxMaps
+    Text:             |
+      400d0000-400d2000 r--p 00000000 b3:04 227        /usr/lib/libc.so
+      400d2000-400d3000 rw-p 00000000 00:00 0
+      400d3000-400d4000 r-xp 00010000 b3:04 227        /usr/lib/libc.so
+      400d4000-400d5000 rwxp 00001000 b3:04 227        /usr/lib/libc.so
+...
+)"),
+                    llvm::Succeeded());
+  // If we have a module mentioned twice in the module list, and we have full
+  // linux maps for all of the memory regions, make sure we pick the one that
+  // has a consecutive region with a matching path that has executable
+  // permissions. If clients open an object file with mmap, breakpad can create
+  // multiple mappings for a library errnoneously and the lowest address isn't
+  // always the right address. In this case we check the consective memory
+  // regions whose path matches starting at the base of image address and make
+  // sure one of the regions is executable and prefer that one.
+  //
+  // This test will make sure that if the executable is first in the module
+  // list, that it will remain the correctly selected module in the filtered
+  // list, even if the non-executable module was loaded at a lower base address.
+  std::vector<const minidump::Module *> filtered_modules =
+      parser->GetFilteredModuleList();
+  ASSERT_EQ(1u, filtered_modules.size());
+  EXPECT_EQ(0x400d3000u, filtered_modules[0]->BaseOfImage);
+}
+
 TEST_F(MinidumpParserTest, MinidumpDuplicateModuleSeparateCode) {
   ASSERT_THAT_ERROR(SetUpFromYaml(R"(
 --- !minidump
Index: lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
===================================================================
--- lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
+++ lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
@@ -391,19 +391,23 @@
       filtered_modules.push_back(&module);
     } else {
       // We have a duplicate module entry. Check the linux regions to see if
-      // the module we already have is not really a mapped executable. If it
-      // isn't check to see if the current duplicate module entry is a real
-      // mapped executable, and if so, replace it. This can happen when a
-      // process mmap's in the file for an executable in order to read bytes
-      // from the executable file. A memory region mapping will exist for the
-      // mmap'ed version and for the loaded executable, but only one will have
-      // a consecutive region that is executable in the memory regions.
+      // either module is not really a mapped executable. If one but not the
+      // other is a real mapped executable, prefer the executable one. This
+      // can happen when a process mmap's in the file for an executable in
+      // order to read bytes from the executable file. A memory region mapping
+      // will exist for the mmap'ed version and for the loaded executable, but
+      // only one will have a consecutive region that is executable in the
+      // memory regions.
       auto dup_module = filtered_modules[iter->second];
       ConstString name(*ExpectedName);
-      if (!CheckForLinuxExecutable(name, linux_regions,
-                                   dup_module->BaseOfImage) &&
-          CheckForLinuxExecutable(name, linux_regions, module.BaseOfImage)) {
-        filtered_modules[iter->second] = &module;
+      bool is_executable =
+          CheckForLinuxExecutable(name, linux_regions, module.BaseOfImage);
+      bool dup_is_executable =
+          CheckForLinuxExecutable(name, linux_regions, dup_module->BaseOfImage);
+
+      if (is_executable != dup_is_executable) {
+        if (is_executable)
+          filtered_modules[iter->second] = &module;
         continue;
       }
       // This module has been seen. Modules are sometimes mentioned multiple


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D94629.316498.patch
Type: text/x-patch
Size: 4620 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20210113/ae5a3e14/attachment-0001.bin>