From bddaea2b88b0a19d9cc7a4dea772af8e829323b3 Mon Sep 17 00:00:00 2001
From: Narayan Kamath <narayan@google.com>
Date: Thu, 21 Aug 2014 17:38:09 +0100
Subject: [PATCH] Make a couple of map checks debug only.

This cost us close to 80ms in app startup times.

The checks that a reused region was within an already
existent map has been demoted to a debug check.

A couple of other negative checks have been removed
outright because one of them was superflous and the other
wasn't guaranteed to be correct.

bug: 16828525
Change-Id: I00f76de06df0ea4ced40fdcb7825248d4b662045
---
 runtime/mem_map.cc | 40 +++++++++++++++++++++++++++-------------
 runtime/mem_map.h  |  3 ++-
 2 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/runtime/mem_map.cc b/runtime/mem_map.cc
index c281b2200..6cda25884 100644
--- a/runtime/mem_map.cc
+++ b/runtime/mem_map.cc
@@ -131,9 +131,9 @@ uintptr_t MemMap::next_mem_pos_ = GenerateNextMemPos();
 #endif
 
 // Return true if the address range is contained in a single /proc/self/map entry.
-static bool CheckOverlapping(uintptr_t begin,
-                             uintptr_t end,
-                             std::string* error_msg) {
+static bool ContainedWithinExistingMap(uintptr_t begin,
+                                       uintptr_t end,
+                                       std::string* error_msg) {
   std::unique_ptr<BacktraceMap> map(BacktraceMap::Create(getpid(), true));
   if (map.get() == nullptr) {
     *error_msg = StringPrintf("Failed to build process map");
@@ -212,12 +212,25 @@ static bool CheckMapRequest(byte* expected_ptr, void* actual_ptr, size_t byte_co
     PLOG(WARNING) << StringPrintf("munmap(%p, %zd) failed", actual_ptr, byte_count);
   }
 
-  if (!CheckNonOverlapping(expected, limit, error_msg)) {
-    return false;
+  // We call this here so that we can try and generate a full error
+  // message with the overlapping mapping. There's no guarantee that
+  // that there will be an overlap though, since
+  // - The kernel is not *required* to honour expected_ptr unless MAP_FIXED is
+  //   true, even if there is no overlap
+  // - There might have been an overlap at the point of mmap, but the
+  //   overlapping region has since been unmapped.
+  std::string error_detail;
+  CheckNonOverlapping(expected, limit, &error_detail);
+
+  std::ostringstream os;
+  os <<  StringPrintf("Failed to mmap at expected address, mapped at "
+                      "0x%08" PRIxPTR " instead of 0x%08" PRIxPTR,
+                      actual, expected);
+  if (!error_detail.empty()) {
+    os << " : " << error_detail;
   }
 
-  *error_msg = StringPrintf("Failed to mmap at expected address, mapped at "
-                            "0x%08" PRIxPTR " instead of 0x%08" PRIxPTR, actual, expected);
+  *error_msg = os.str();
   return false;
 }
 
@@ -375,19 +388,20 @@ MemMap* MemMap::MapFileAtAddress(byte* expected_ptr, size_t byte_count, int prot
   CHECK_NE(0, flags & (MAP_SHARED | MAP_PRIVATE));
   uintptr_t expected = reinterpret_cast<uintptr_t>(expected_ptr);
   uintptr_t limit = expected + byte_count;
+
+  // Note that we do not allow MAP_FIXED unless reuse == true, i.e we
+  // expect his mapping to be contained within an existing map.
   if (reuse) {
     // reuse means it is okay that it overlaps an existing page mapping.
     // Only use this if you actually made the page reservation yourself.
     CHECK(expected_ptr != nullptr);
-    if (!CheckOverlapping(expected, limit, error_msg)) {
-      return nullptr;
-    }
+
+    DCHECK(ContainedWithinExistingMap(expected, limit, error_msg));
     flags |= MAP_FIXED;
   } else {
     CHECK_EQ(0, flags & MAP_FIXED);
-    if (expected_ptr != nullptr && !CheckNonOverlapping(expected, limit, error_msg)) {
-      return nullptr;
-    }
+    // Don't bother checking for an overlapping region here. We'll
+    // check this if required after the fact inside CheckMapRequest.
   }
 
   if (byte_count == 0) {
diff --git a/runtime/mem_map.h b/runtime/mem_map.h
index 872c63b19..9bfcd96d7 100644
--- a/runtime/mem_map.h
+++ b/runtime/mem_map.h
@@ -77,7 +77,8 @@ class MemMap {
   // mapping. "reuse" allows us to create a view into an existing
   // mapping where we do not take ownership of the memory.
   //
-  // On success, returns returns a MemMap instance.  On failure, returns a NULL;
+  // On success, returns returns a MemMap instance.  On failure, returns a
+  // nullptr;
   static MemMap* MapFileAtAddress(byte* addr, size_t byte_count, int prot, int flags, int fd,
                                   off_t start, bool reuse, const char* filename,
                                   std::string* error_msg);
-- 
2.11.0