[Lldb-commits] [PATCH] D86375: Load correct module for linux and android when duplicates exist in minidump.

[Greg Clayton via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGc55db4600b4b: Load correct module for linux and android when 
duplicates exist in minidump. (authored by clayborg).

Changed prior to commit:
  https://reviews.llvm.org/D86375?vs=287853=288124#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86375

Files:
  lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
  lldb/source/Plugins/Process/minidump/MinidumpParser.h
  lldb/source/Plugins/Process/minidump/ProcessMinidump.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
@@ -689,6 +689,130 @@
   EXPECT_EQ(0x1000u, filtered_modules[0]->BaseOfImage);
 }
 
+TEST_F(MinidumpParserTest, MinidumpDuplicateModuleMappedFirst) {
+  ASSERT_THAT_ERROR(SetUpFromYaml(R"(
+--- !minidump
+Streams:
+  - Type:ModuleList
+Modules:
+  - Base of Image:   0x400d
+Size of Image:   0x2000
+Module Name: '/usr/lib/libc.so'
+CodeView Record: ''
+  - Base of Image:   0x400d3000
+Size of Image:   0x1000
+Module Name: '/usr/lib/libc.so'
+CodeView Record: ''
+  - Type:LinuxMaps
+Text: |
+  400d-400d2000 r--p  b3:04 227/usr/lib/libc.so
+  400d2000-400d3000 rw-p  00:00 0
+  400d3000-400d4000 r-xp 0001 b3:04 227/usr/lib/libc.so
+  400d4000-400d5000 rwxp 1000 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 second in the module
+  // list, that it will become the selected module in the filtered list.
+  std::vector filtered_modules =
+  parser->GetFilteredModuleList();
+  ASSERT_EQ(1u, filtered_modules.size());
+  EXPECT_EQ(0x400d3000u, filtered_modules[0]->BaseOfImage);
+}
+
+TEST_F(MinidumpParserTest, MinidumpDuplicateModuleMappedSecond) {
+  ASSERT_THAT_ERROR(SetUpFromYaml(R"(
+--- !minidump
+Streams:
+  - Type:ModuleList
+Modules:
+  - Base of Image:   0x400d
+Size of Image:   0x2000
+Module Name: '/usr/lib/libc.so'
+CodeView Record: ''
+  - Base of Image:   0x400d3000
+Size of Image:   0x1000
+Module Name: '/usr/lib/libc.so'
+CodeView Record: ''
+  - Type:LinuxMaps
+Text: |
+  400d-400d1000 r-xp 0001 b3:04 227/usr/lib/libc.so
+  400d1000-400d2000 rwxp 1000 b3:04 227/usr/lib/libc.so
+  400d2000-400d3000 rw-p  00:00 0
+  400d3000-400d5000 r--p  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.
+  std::vector filtered_modules =
+  parser->GetFilteredModuleList();
+  ASSERT_EQ(1u, filtered_modules.size());
+  EXPECT_EQ(0x400du, filtered_modules[0]->BaseOfImage);
+}
+
+TEST_F(MinidumpParserTest, MinidumpDuplicateModuleSeparateCode) {
+  ASSERT_THAT_ERROR(SetUpFromYaml(R"(
+--- !minidump
+Streams:
+  - Type:ModuleList
+Modules:
+  - Base of Image:   0x400d
+Size of Image:   0x2000
+Module Name: '/usr/lib/libc.so'
+

[Lldb-commits] [PATCH] D86375: Load correct module for linux and android when duplicates exist in minidump.

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

Thanks for the clarification. The new version looks great.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86375

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86375: Load correct module for linux and android when duplicates exist in minidump.

[Greg Clayton via Phabricator via lldb-commits]

clayborg updated this revision to Diff 287853.
clayborg added a comment.

Added a better algorithm for detecting if an address from the module list for 
linux processes with valid /proc//maps file is executable. We now search 
consective addresses that match the path to the module for any that are marked 
as executable. Added tests to test if the mmap'ed file comes first or second, 
and also a test for "-z separate-code" when the first mapping for an executable 
isn't executable, but a subsequent mapping with a matching path is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86375

Files:
  lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
  lldb/source/Plugins/Process/minidump/MinidumpParser.h
  lldb/source/Plugins/Process/minidump/ProcessMinidump.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
@@ -689,6 +689,130 @@
   EXPECT_EQ(0x1000u, filtered_modules[0]->BaseOfImage);
 }
 
+TEST_F(MinidumpParserTest, MinidumpDuplicateModuleMappedFirst) {
+  ASSERT_THAT_ERROR(SetUpFromYaml(R"(
+--- !minidump
+Streams:
+  - Type:ModuleList
+Modules:
+  - Base of Image:   0x400d
+Size of Image:   0x2000
+Module Name: '/usr/lib/libc.so'
+CodeView Record: ''
+  - Base of Image:   0x400d3000
+Size of Image:   0x1000
+Module Name: '/usr/lib/libc.so'
+CodeView Record: ''
+  - Type:LinuxMaps
+Text: |
+  400d-400d2000 r--p  b3:04 227/usr/lib/libc.so
+  400d2000-400d3000 rw-p  00:00 0
+  400d3000-400d4000 r-xp 0001 b3:04 227/usr/lib/libc.so
+  400d4000-400d5000 rwxp 1000 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 second in the module
+  // list, that it will become the selected module in the filtered list.
+  std::vector filtered_modules =
+  parser->GetFilteredModuleList();
+  ASSERT_EQ(1u, filtered_modules.size());
+  EXPECT_EQ(0x400d3000u, filtered_modules[0]->BaseOfImage);
+}
+
+TEST_F(MinidumpParserTest, MinidumpDuplicateModuleMappedSecond) {
+  ASSERT_THAT_ERROR(SetUpFromYaml(R"(
+--- !minidump
+Streams:
+  - Type:ModuleList
+Modules:
+  - Base of Image:   0x400d
+Size of Image:   0x2000
+Module Name: '/usr/lib/libc.so'
+CodeView Record: ''
+  - Base of Image:   0x400d3000
+Size of Image:   0x1000
+Module Name: '/usr/lib/libc.so'
+CodeView Record: ''
+  - Type:LinuxMaps
+Text: |
+  400d-400d1000 r-xp 0001 b3:04 227/usr/lib/libc.so
+  400d1000-400d2000 rwxp 1000 b3:04 227/usr/lib/libc.so
+  400d2000-400d3000 rw-p  00:00 0
+  400d3000-400d5000 r--p  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.
+  std::vector filtered_modules =
+  parser->GetFilteredModuleList();
+  ASSERT_EQ(1u, filtered_modules.size());
+  EXPECT_EQ(0x400du, filtered_modules[0]->BaseOfImage);
+}
+
+TEST_F(MinidumpParserTest, MinidumpDuplicateModuleSeparateCode) {
+  ASSERT_THAT_ERROR(SetUpFromYaml(R"(
+--- !minidump

[Lldb-commits] [PATCH] D86375: Load correct module for linux and android when duplicates exist in minidump.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D86375#2233054 , @labath wrote:

> If I correctly understand what this is doing, then I don't think it's a good 
> idea. The base of an (elf) shared library does not have to be mapped 
> executable. These are the mappings I get for a trivial hello world program 
> (no mmapping of libraries or anything) on my linux machine:
>
>   0040-00401000 r--p  fd:01 2838574
> /tmp/l/a.out
>   00401000-00402000 r-xp 1000 fd:01 2838574
> /tmp/l/a.out
>   00402000-00403000 r--p 2000 fd:01 2838574
> /tmp/l/a.out
>   00403000-00404000 r--p 2000 fd:01 2838574
> /tmp/l/a.out
>   00404000-00405000 rw-p 3000 fd:01 2838574
> /tmp/l/a.out
>   020fb000-0211c000 rw-p  00:00 0  
> [heap]
>   7fe4f5d87000-7fe4f5da9000 r--p  fd:01 2738932
> /lib64/libc-2.31.so
>   7fe4f5da9000-7fe4f5ef3000 r-xp 00022000 fd:01 2738932
> /lib64/libc-2.31.so
>   7fe4f5ef3000-7fe4f5f3d000 r--p 0016c000 fd:01 2738932
> /lib64/libc-2.31.so
>   7fe4f5f3d000-7fe4f5f41000 r--p 001b5000 fd:01 2738932
> /lib64/libc-2.31.so
>   7fe4f5f41000-7fe4f5f43000 rw-p 001b9000 fd:01 2738932
> /lib64/libc-2.31.so
>   ...
>
> Here, the correct base of a.out is 0x0040 and the libc base is 
> 0x7fe4f5d87000. But this patch would make them be detected as 0x00401000 and 
> 0x7fe4f5da9000, respectively.

It actually will do the right thing here. We only prefer the executable mapping 
for multiple entries in the minidump modules list. The minidump will have only 
one load address per module and that address will always be the first mapping 
in the linux proc maps file.

So the only entry for "/tmp/l/a.out" in the minidump would be the one for 
0040, and the only mapping for /lib64/libc-2.31.so would be 7fe4f5d87000.

If we have multiple module entries _and_ if you have separate code, the old 
behavior will continue and it will prefer the mapping with the lowest address 
because neither of the binaries would have the base address marked as 
executable. If we have multiple entries for the exact same path, but with 
different addresses, we will prefer the one that _is_ marked executable 
correctly (which happens on Android for us all the time, can could easily 
happen on Linux with "-z no-separate-code".

> This behavior is controlled by the `-z (no)separate-code`. My machine has 
> `separate-code` as default, but that setting may not be universal, so you may 
> need to pass this flag explicitly to reproduce this. For reference, these are 
> the mappings I get when compiling a.out with `-z noseparate-code` (libc 
> mappings remain unchanged, of course):
>
>   0040-00401000 r-xp  fd:01 2838574
> /tmp/l/a.out
>   00401000-00402000 r--p  fd:01 2838574
> /tmp/l/a.out
>   00402000-00403000 rw-p 1000 fd:01 2838574
> /tmp/l/a.out
>
> It sounds like we need a better heuristic. How about "the number of 
> consecutive mappings with the same name"? User mmapping code is likely going 
> to map the library in a single chunk, but the dynamic linker will typically 
> create multiple mappings (even a trivial executable can have five), so it 
> seems like picking the longest sequence could work...

So this heuristic will work like it did before for "-z separate-code", and will 
fix multiple mappings if "-z no-separate-code" is used. So this is only a win 
in my book since we only check the start address for a shared library _and_ we 
only check for the execute part if we have multiple module entries for the same 
file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86375

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86375: Load correct module for linux and android when duplicates exist in minidump.

[Pavel Labath via Phabricator via lldb-commits]

labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.

If I correctly understand what this is doing, then I don't think it's a good 
idea. The base of an (elf) shared library does not have to be mapped 
executable. These are the mappings I get for a trivial hello world program (no 
mmapping of libraries or anything) on my linux machine:

  0040-00401000 r--p  fd:01 2838574
/tmp/l/a.out
  00401000-00402000 r-xp 1000 fd:01 2838574
/tmp/l/a.out
  00402000-00403000 r--p 2000 fd:01 2838574
/tmp/l/a.out
  00403000-00404000 r--p 2000 fd:01 2838574
/tmp/l/a.out
  00404000-00405000 rw-p 3000 fd:01 2838574
/tmp/l/a.out
  020fb000-0211c000 rw-p  00:00 0  
[heap]
  7fe4f5d87000-7fe4f5da9000 r--p  fd:01 2738932
/lib64/libc-2.31.so
  7fe4f5da9000-7fe4f5ef3000 r-xp 00022000 fd:01 2738932
/lib64/libc-2.31.so
  7fe4f5ef3000-7fe4f5f3d000 r--p 0016c000 fd:01 2738932
/lib64/libc-2.31.so
  7fe4f5f3d000-7fe4f5f41000 r--p 001b5000 fd:01 2738932
/lib64/libc-2.31.so
  7fe4f5f41000-7fe4f5f43000 rw-p 001b9000 fd:01 2738932
/lib64/libc-2.31.so
  ...

Here, the correct base of a.out is 0x0040 and the libc base is 
0x7fe4f5d87000. But this patch would make them be detected as 0x00401000 and 
0x7fe4f5da9000, respectively.

This behavior is controlled by the `-z (no)separate-code`. My machine has 
`separate-code` as default, but that setting may not be universal, so you may 
need to pass this flag explicitly to reproduce this. For reference, these are 
the mappings I get when compiling a.out with `-z noseparate-code` (libc 
mappings remain unchanged, of course):

  0040-00401000 r-xp  fd:01 2838574
/tmp/l/a.out
  00401000-00402000 r--p  fd:01 2838574
/tmp/l/a.out
  00402000-00403000 rw-p 1000 fd:01 2838574
/tmp/l/a.out

It sounds like we need a better heuristic. How about "the number of consecutive 
mappings with the same name"? User mmapping code is likely going to map the 
library in a single chunk, but the dynamic linker will typically create 
multiple mappings (even a trivial executable can have five), so it seems like 
picking the longest sequence could work...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86375

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86375: Load correct module for linux and android when duplicates exist in minidump.

[Greg Clayton via Phabricator via lldb-commits]

clayborg created this revision.
clayborg added reviewers: labath, aadsm, wallace.
Herald added a subscriber: mgrang.
Herald added a project: LLDB.
clayborg requested review of this revision.
Herald added a subscriber: JDevlieghere.

Breakpad creates minidump files that can a module loaded multiple times. We 
found that when a process mmap's the object file for a library, this can 
confuse breakpad into creating multiple modules in the module list. This patch 
fixes the GetFilteredModules() to check the linux maps for permissions and use 
the one that has execute permissions. Typically when people mmap a file into 
memory they don't map it as executable. This helps people to correctly load 
minidump files for post mortem analysis.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86375

Files:
  lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
  lldb/source/Plugins/Process/minidump/MinidumpParser.h
  lldb/source/Plugins/Process/minidump/ProcessMinidump.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
@@ -689,6 +689,42 @@
   EXPECT_EQ(0x1000u, filtered_modules[0]->BaseOfImage);
 }
 
+TEST_F(MinidumpParserTest, MinidumpDuplicateModuleExecutableAddress) {
+  ASSERT_THAT_ERROR(SetUpFromYaml(R"(
+--- !minidump
+Streams:
+  - Type:ModuleList
+Modules:
+  - Base of Image:   0x400d9000
+Size of Image:   0x2000
+Module Name: '/usr/lib/libc.so'
+CodeView Record: ''
+  - Base of Image:   0x400ee000
+Size of Image:   0x1000
+Module Name: '/usr/lib/libc.so'
+CodeView Record: ''
+  - Type:LinuxMaps
+Text: |
+  400d9000-400db000 r--p  b3:04 227/usr/lib/libc.so
+  400dc000-400dd000 rw-p  00:00 0
+  400ee000-400ef000 r-xp 0001 b3:04 227/usr/lib/libc.so
+  400fc000-400fd000 rwxp 1000 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
+  // is marked as executable. 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
+  // memory region of the base of image address and make sure it is executable
+  // and prefer that one.
+  std::vector filtered_modules =
+  parser->GetFilteredModuleList();
+  ASSERT_EQ(1u, filtered_modules.size());
+  EXPECT_EQ(0x400ee000u, filtered_modules[0]->BaseOfImage);
+}
+
 TEST_F(MinidumpParserTest, MinidumpModuleOrder) {
   ASSERT_THAT_ERROR(SetUpFromYaml(R"(
 --- !minidump
@@ -721,4 +757,3 @@
   parser->GetMinidumpFile().getString(filtered_modules[1]->ModuleNameRVA),
   llvm::HasValue("/tmp/b"));
 }
-
Index: lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -338,32 +338,6 @@
   return ArchSpec(triple);
 }
 
-static MemoryRegionInfo GetMemoryRegionInfo(const MemoryRegionInfos ,
-lldb::addr_t load_addr) {
-  MemoryRegionInfo region;
-  auto pos = llvm::upper_bound(regions, load_addr);
-  if (pos != regions.begin() &&
-  std::prev(pos)->GetRange().Contains(load_addr)) {
-return *std::prev(pos);
-  }
-
-  if (pos == regions.begin())
-region.GetRange().SetRangeBase(0);
-  else
-region.GetRange().SetRangeBase(std::prev(pos)->GetRange().GetRangeEnd());
-
-  if (pos == regions.end())
-region.GetRange().SetRangeEnd(UINT64_MAX);
-  else
-region.GetRange().SetRangeEnd(pos->GetRange().GetRangeBase());
-
-  region.SetReadable(MemoryRegionInfo::eNo);
-  region.SetWritable(MemoryRegionInfo::eNo);
-  region.SetExecutable(MemoryRegionInfo::eNo);
-  region.SetMapped(MemoryRegionInfo::eNo);
-  return region;
-}
-
 void ProcessMinidump::BuildMemoryRegions() {
   if (m_memory_regions)
 return;
@@ -388,7 +362,7 @@
   MemoryRegionInfo::RangeType section_range(load_addr,
 section_sp->GetByteSize());
   MemoryRegionInfo region =
-  ::GetMemoryRegionInfo(*m_memory_regions, load_addr);
+  MinidumpParser::GetMemoryRegionInfo(*m_memory_regions, load_addr);
   if (region.GetMapped() != MemoryRegionInfo::eYes &&
   region.GetRange().GetRangeBase() <= section_range.GetRangeBase() &&
   section_range.GetRangeEnd() <= region.GetRange().GetRangeEnd()) {
@@ -409,7 +383,7