Merge adjacent mappings with the same name into one module in LinuxDumper.

A=Mike Hommey <mh+mozilla@glandium.org> R=ted at https://bugzilla.mozilla.org/show_bug.cgi?id=637316

git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@781 4c0a9323-5329-0410-9bdc-e9ce6186880e

2 files changed, 117 insertions, 23 deletions

diff --git a/src/client/linux/minidump_writer/linux_dumper.cc b/src/client/linux/minidump_writer/linux_dumper.cc
index 4a4b5e72..88900f9b 100644
--- a/src/client/linux/minidump_writer/linux_dumper.cc
+++ b/src/client/linux/minidump_writer/linux_dumper.cc
@@ -306,25 +306,36 @@ LinuxDumper::EnumerateMappings(wasteful_vector<MappingInfo*>* result) const {
       if (*i2 == ' ') {
         const char* i3 = my_read_hex_ptr(&offset, i2 + 6 /* skip ' rwxp ' */);
         if (*i3 == ' ') {
+          const char* name = NULL;
+          // Only copy name if the name is a valid path name, or if
+          // it's the VDSO image.
+          if (((name = my_strchr(line, '/')) == NULL) &&
+              linux_gate_loc &&
+              reinterpret_cast<void*>(start_addr) == linux_gate_loc) {
+            name = kLinuxGateLibraryName;
+            offset = 0;
+          }
+          // Merge adjacent mappings with the same name into one module,
+          // assuming they're a single library mapped by the dynamic linker
+          if (name && result->size()) {
+            MappingInfo* module = (*result)[result->size() - 1];
+            if ((start_addr == module->start_addr + module->size) &&
+                (my_strlen(name) == my_strlen(module->name)) &&
+                (my_strncmp(name, module->name, my_strlen(name)) == 0)) {
+              module->size = end_addr - module->start_addr;
+              line_reader->PopLine(line_len);
+              continue;
+            }
+          }
           MappingInfo* const module = new(allocator_) MappingInfo;
           memset(module, 0, sizeof(MappingInfo));
           module->start_addr = start_addr;
           module->size = end_addr - start_addr;
           module->offset = offset;
-          const char* name = NULL;
-          // Only copy name if the name is a valid path name, or if
-          // it's the VDSO image.
-          if ((name = my_strchr(line, '/')) != NULL) {
+          if (name != NULL) {
             const unsigned l = my_strlen(name);
             if (l < sizeof(module->name))
               memcpy(module->name, name, l);
-          } else if (linux_gate_loc &&
-                     reinterpret_cast<void*>(module->start_addr) ==
-                     linux_gate_loc) {
-            memcpy(module->name,
-                   kLinuxGateLibraryName,
-                   my_strlen(kLinuxGateLibraryName));
-            module->offset = 0;
           }
           result->push_back(module);
         }
diff --git a/src/client/linux/minidump_writer/linux_dumper_unittest.cc b/src/client/linux/minidump_writer/linux_dumper_unittest.cc
index a3e84dbc..33d9da7c 100644
--- a/src/client/linux/minidump_writer/linux_dumper_unittest.cc
+++ b/src/client/linux/minidump_writer/linux_dumper_unittest.cc
@@ -33,6 +33,7 @@
 #include <unistd.h>
 #include <signal.h>
 #include <stdint.h>
+#include <sys/mman.h>
 #include <sys/poll.h>
 #include <sys/types.h>
 
@@ -47,6 +48,24 @@ using namespace google_breakpad;
 
 namespace {
 typedef testing::Test LinuxDumperTest;
+
+string GetHelperBinary() {
+  // Locate helper binary next to the current binary.
+  char self_path[PATH_MAX];
+  if (readlink("/proc/self/exe", self_path, sizeof(self_path) - 1) == -1) {
+    return "";
+  }
+  string helper_path(self_path);
+  size_t pos = helper_path.rfind('/');
+  if (pos == string::npos) {
+    return "";
+  }
+  helper_path.erase(pos + 1);
+  helper_path += "linux_dumper_unittest_helper";
+
+  return helper_path;
+}
+
 }
 
 TEST(LinuxDumperTest, Setup) {
@@ -76,6 +95,79 @@ TEST(LinuxDumperTest, ThreadList) {
   }
 }
 
+// Helper stack class to close a file descriptor and unmap
+// a mmap'ed mapping.
+class StackHelper {
+public:
+  StackHelper(int fd, char* mapping, size_t size)
+    : fd_(fd), mapping_(mapping), size_(size) {}
+  ~StackHelper() {
+    munmap(mapping_, size_);
+    close(fd_);
+  }
+
+private:
+  int fd_;
+  char* mapping_;
+  size_t size_;
+};
+
+TEST(LinuxDumperTest, MergedMappings) {
+  string helper_path(GetHelperBinary());
+  if (helper_path.empty()) {
+    FAIL() << "Couldn't find helper binary";
+    exit(1);
+  }
+
+  // mmap two segments out of the helper binary, one
+  // enclosed in the other, but with different protections.
+  const size_t kPageSize = sysconf(_SC_PAGESIZE);
+  const size_t kMappingSize = 3 * kPageSize;
+  int fd = open(helper_path.c_str(), O_RDONLY);
+  ASSERT_NE(-1, fd);
+  char* mapping =
+    reinterpret_cast<char*>(mmap(NULL,
+                                 kMappingSize,
+                                 PROT_READ,
+                                 MAP_SHARED,
+                                 fd,
+                                 0));
+  ASSERT_TRUE(mapping);
+
+  const u_int64_t kMappingAddress = reinterpret_cast<u_int64_t>(mapping);
+
+  // Ensure that things get cleaned up.
+  StackHelper helper(fd, mapping, kMappingSize);
+
+  // Carve a page out of the first mapping with different permissions.
+  char* inside_mapping =  reinterpret_cast<char*>(mmap(mapping + 2 *kPageSize,
+                                 kPageSize,
+                                 PROT_NONE,
+                                 MAP_SHARED | MAP_FIXED,
+                                 fd,
+                                 // Map a different offset just to
+                                 // better test real-world conditions.
+                                 kPageSize));
+  ASSERT_TRUE(inside_mapping);
+
+  // Now check that LinuxDumper interpreted the mappings properly.
+  LinuxDumper dumper(getpid());
+  ASSERT_TRUE(dumper.Init());
+  int mapping_count = 0;
+  for (unsigned i = 0; i < dumper.mappings().size(); ++i) {
+    const MappingInfo& mapping = *dumper.mappings()[i];
+    if (strcmp(mapping.name, helper_path.c_str()) == 0) {
+      // This mapping should encompass the entire original mapped
+      // range.
+      EXPECT_EQ(kMappingAddress, mapping.start_addr);
+      EXPECT_EQ(kMappingSize, mapping.size);
+      EXPECT_EQ(0, mapping.offset);
+      mapping_count++;
+    }
+  }
+  EXPECT_EQ(1, mapping_count);
+}
+
 TEST(LinuxDumperTest, VerifyStackReadWithMultipleThreads) {
   static const int kNumberOfThreadsInHelperProgram = 5;
   char kNumberOfThreadsArgument[2];
@@ -89,20 +181,11 @@ TEST(LinuxDumperTest, VerifyStackReadWithMultipleThreads) {
     // In child process.
     close(fds[0]);
 
-    // Locate helper binary next to the current binary.
-    char self_path[PATH_MAX];
-    if (readlink("/proc/self/exe", self_path, sizeof(self_path) - 1) == -1) {
-      FAIL() << "readlink failed: " << strerror(errno);
-      exit(1);
-    }
-    string helper_path(self_path);
-    size_t pos = helper_path.rfind('/');
-    if (pos == string::npos) {
-      FAIL() << "no trailing slash in path: " << helper_path;
+    string helper_path(GetHelperBinary());
+    if (helper_path.empty()) {
+      FAIL() << "Couldn't find helper binary";
       exit(1);
     }
-    helper_path.erase(pos + 1);
-    helper_path += "linux_dumper_unittest_helper";
 
     // Pass the pipe fd and the number of threads as arguments.
     char pipe_fd_string[8];