Check stack pointer to be inside stack when unwinding.

base/debug/stack_trace.cc

Index: base/debug/stack_trace.cc
diff --git a/base/debug/stack_trace.cc b/base/debug/stack_trace.cc
index 1c96a569d9795544231714f0d0b1f9b0da036355..0150abe0a2a655489c8d295a349c66528d78dc44 100644
--- a/base/debug/stack_trace.cc
+++ b/base/debug/stack_trace.cc
@@ -11,6 +11,16 @@
 
 #include "base/macros.h"
 
+#if HAVE_TRACE_STACK_FRAME_POINTERS && \
+    (defined(OS_ANDROID) || defined(OS_LINUX))
+#define HAVE_MINCORE
+#include <sys/mman.h>
+#include <unistd.h>
+#include "base/posix/eintr_wrapper.h"
+#include "base/process/process_metrics.h"
+#endif
+
+
 namespace base {
 namespace debug {
 
@@ -41,9 +51,61 @@ std::string StackTrace::ToString() const {
 
 #if HAVE_TRACE_STACK_FRAME_POINTERS
 
+#ifdef HAVE_MINCORE
+
+// Attempts to advance |address| by PageCount pages, checking that all
+// pages are mapped.
+// Returns true if no unmapped pages were found, and adds PageCount
+// pages to |address| and rounds result down to a page.
+// Returns false if unmapped pages were detected, and sets |address| to
+// the address of the lowest unmapped page.
+template <size_t PageCount>

[Primiano Tucci (use gerrit), 2016/05/31 16:13:06]

why do you need a template function with a lambda inside? Can this function be
simpler?

+bool AdvanceMappedPages(uintptr_t* address) {
+  auto probe = [](uintptr_t address, size_t size) -> bool {
+    uint8_t vec[PageCount];
+    int result = HANDLE_EINTR(

[Primiano Tucci (use gerrit), 2016/05/31 16:13:07]

accordin to its manpage mincore doesn't EINTR (vm operations are typically not
interruptible because they take the map_sem). At most can EAGAIN if the kernel
is temporarily OOM.So maybe you want to do something like
https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/p...

[Dmitry Skiba, 2016/05/31 21:52:18]

Done.

+        mincore(reinterpret_cast<void*>(address), size, vec));
+    if (result == 0) {
+      return true;
+    }
+    // mincore() returns ENOMEM if address range contains unmapped pages.
+    CHECK_EQ(errno, ENOMEM);

[Primiano Tucci (use gerrit), 2016/05/31 16:13:06]

I'd remove this CHECK or make it a DCHECK.
1) you can get a EAGAIN
2) If you fail this estimation worst case you'll end up scanning a reduced stack
which is not undefined behavior (which is what CHECK should protect against).

[Dmitry Skiba, 2016/05/31 21:52:18]

Done.

+    return false;
+  };
+
+  size_t page_size = GetPageSize();
+
+  // Rewind address to the page boundary.
+  *address -= *address % page_size;
+
+  // Probe the whole range first.
+  if (probe(*address, PageCount * page_size)) {
+    *address += PageCount * page_size;
+    return true;
+  }
+
+  // Binary search leftmost page that is not mapped.
+  for (size_t pages = PageCount; pages != 1;) {

[Primiano Tucci (use gerrit), 2016/05/31 16:13:06]

I think this is a bit hard to follow as you do a binary search here and an outer
while below.
Could this be just a single loop in one place so it's easier to read? 

[Dmitry Skiba, 2016/05/31 21:52:18]

I don't see how it can be a single loop. When binary searching I half the range
on each iteration. So the first check that checks the whole range doens't really
belong here. Besides, I need to return true / false depending on the first
check. 

+    size_t half_pages = pages / 2;
+    if (!probe(*address, half_pages * page_size)) {
+      pages = half_pages;
+      continue;
+    }
+    *address += half_pages * page_size;
+    pages -= half_pages;
+  }
+  return false;
+}
+
+#endif  // HAVE_MINCORE
+
+PerThreadStackInfo::PerThreadStackInfo()
+  : start_address(0), start_address_final(false) {}
+
 size_t TraceStackFramePointers(const void** out_trace,
                                size_t max_depth,
-                               size_t skip_initial) {
+                               size_t skip_initial,
+                               PerThreadStackInfo* stack_info) {
   // Usage of __builtin_frame_address() enables frame pointers in this
   // function even if they are not enabled globally. So 'sp' will always
   // be valid.
@@ -58,6 +120,26 @@ size_t TraceStackFramePointers(const void** out_trace,
     sp -= sizeof(uintptr_t);
 #endif
 
+#ifdef HAVE_MINCORE
+    if (stack_info) {
+      // Both sp[0] and s[1] must be valid.
+      uintptr_t max_sp = sp + 2 * sizeof(uintptr_t);
+      if (!stack_info->start_address) {
+        // Initialize start_address so that it satisfies while() below.
+        stack_info->start_address = max_sp - 1;
+      }
+      while (max_sp > stack_info->start_address &&
+             !stack_info->start_address_final) {
+        stack_info->start_address_final = !AdvanceMappedPages<32>(
+            &stack_info->start_address);
+      }
+      if (max_sp > stack_info->start_address &&
+          stack_info->start_address_final) {
+        break;
+      }
+    }
+#endif
+
     if (skip_initial != 0) {
       skip_initial--;
     } else {