Commit 2f7d5874 authored by Siddhartha's avatar Siddhartha Committed by Commit Bot

Android: Scan stack for PCs if unwinding fails

We have tried to unwind the stack frame and the failure rate of
unwinding android framework frames with libunwind is too high to get
meaningful data. So, scan the stack to find all chrome PCs if unwinding
fails. We can try to sanitize the stack on the server side.

BUG=859260

Change-Id: I689b59f03f6b088576e45667fbad878c67940ba2
Reviewed-on: https://chromium-review.googlesource.com/c/1308717
Commit-Queue: ssid <ssid@chromium.org>
Reviewed-by: default avatarMike Wittman <wittman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605159}
parent 04c1ebb7
...@@ -8,6 +8,7 @@ ...@@ -8,6 +8,7 @@
#include <sys/types.h> #include <sys/types.h>
#include "base/android/apk_assets.h" #include "base/android/apk_assets.h"
#include "base/android/library_loader/anchor_functions.h"
#if !defined(ARCH_CPU_ARMEL) #if !defined(ARCH_CPU_ARMEL)
#error This file should not be built for this architecture. #error This file should not be built for this architecture.
...@@ -63,10 +64,6 @@ extern "C" { ...@@ -63,10 +64,6 @@ extern "C" {
// of the instruction in binary from PC. // of the instruction in binary from PC.
extern char __executable_start; extern char __executable_start;
// The address |_etext| gives the end of the .text section in the binary. This
// value is more accurate than parsing the memory map since the mapped
// regions are usualy larger than the .text section.
extern char _etext;
} }
namespace base { namespace base {
...@@ -127,6 +124,11 @@ CFIBacktraceAndroid* CFIBacktraceAndroid::GetInitializedInstance() { ...@@ -127,6 +124,11 @@ CFIBacktraceAndroid* CFIBacktraceAndroid::GetInitializedInstance() {
return instance; return instance;
} }
// static
bool CFIBacktraceAndroid::is_chrome_address(uintptr_t pc) {
return pc >= base::android::kStartOfText && pc < executable_end_addr();
}
// static // static
uintptr_t CFIBacktraceAndroid::executable_start_addr() { uintptr_t CFIBacktraceAndroid::executable_start_addr() {
return reinterpret_cast<uintptr_t>(&__executable_start); return reinterpret_cast<uintptr_t>(&__executable_start);
...@@ -134,7 +136,7 @@ uintptr_t CFIBacktraceAndroid::executable_start_addr() { ...@@ -134,7 +136,7 @@ uintptr_t CFIBacktraceAndroid::executable_start_addr() {
// static // static
uintptr_t CFIBacktraceAndroid::executable_end_addr() { uintptr_t CFIBacktraceAndroid::executable_end_addr() {
return reinterpret_cast<uintptr_t>(&_etext); return base::android::kEndOfText;
} }
CFIBacktraceAndroid::CFIBacktraceAndroid() CFIBacktraceAndroid::CFIBacktraceAndroid()
......
...@@ -36,9 +36,7 @@ class BASE_EXPORT CFIBacktraceAndroid { ...@@ -36,9 +36,7 @@ class BASE_EXPORT CFIBacktraceAndroid {
static CFIBacktraceAndroid* GetInitializedInstance(); static CFIBacktraceAndroid* GetInitializedInstance();
// Returns true if the given program counter |pc| is mapped in chrome library. // Returns true if the given program counter |pc| is mapped in chrome library.
static bool is_chrome_address(uintptr_t pc) { static bool is_chrome_address(uintptr_t pc);
return pc >= executable_start_addr() && pc < executable_end_addr();
}
// Returns the start and end address of the current library. // Returns the start and end address of the current library.
static uintptr_t executable_start_addr(); static uintptr_t executable_start_addr();
......
...@@ -100,6 +100,7 @@ size_t TraceStackWithContext( ...@@ -100,6 +100,7 @@ size_t TraceStackWithContext(
const size_t max_depth) { const size_t max_depth) {
size_t depth = 0; size_t depth = 0;
unw_word_t ip = 0, sp = 0; unw_word_t ip = 0, sp = 0;
bool try_stack_search = true;
unw_get_reg(cursor, UNW_REG_SP, &sp); unw_get_reg(cursor, UNW_REG_SP, &sp);
const uintptr_t initial_sp = sp; const uintptr_t initial_sp = sp;
uintptr_t previous_sp = 0; uintptr_t previous_sp = 0;
...@@ -138,6 +139,7 @@ size_t TraceStackWithContext( ...@@ -138,6 +139,7 @@ size_t TraceStackWithContext(
unw_get_reg(cursor, UNW_ARM_LR, &lr); unw_get_reg(cursor, UNW_ARM_LR, &lr);
depth += depth +=
cfi_unwinder->Unwind(ip, sp, lr, out_trace + depth, max_depth - depth); cfi_unwinder->Unwind(ip, sp, lr, out_trace + depth, max_depth - depth);
try_stack_search = false;
} }
if (depth >= max_depth) if (depth >= max_depth)
return depth; return depth;
...@@ -151,10 +153,31 @@ size_t TraceStackWithContext( ...@@ -151,10 +153,31 @@ size_t TraceStackWithContext(
continue; continue;
depth += cfi_unwinder->Unwind(marker->pc, marker->sp, /*lr=*/0, depth += cfi_unwinder->Unwind(marker->pc, marker->sp, /*lr=*/0,
out_trace + depth, max_depth - depth); out_trace + depth, max_depth - depth);
try_stack_search = false;
if (depth >= max_depth) if (depth >= max_depth)
break; break;
} }
// We tried all possible ways to unwind and failed. So, scan the stack to find
// all chrome address and add them to stack trace. This would give us a lot of
// false frames on the trace. The idea is to try to sanitize the trace on
// server side or try unwinding after each search. The current version just
// sends back all PCs untill we figure out what is the best way to sanitize
// the stack trace.
if (try_stack_search) {
uintptr_t* stack = reinterpret_cast<uintptr_t*>(sp);
// Add a nullptr to differentiate addresses found by unwinding and scanning.
out_trace[depth++] = nullptr;
while (depth < max_depth &&
reinterpret_cast<uintptr_t>(stack) < stack_segment_base) {
if (CFIBacktraceAndroid::is_chrome_address(
reinterpret_cast<uintptr_t>(*stack))) {
out_trace[depth++] = reinterpret_cast<void*>(*stack);
}
++stack;
}
}
if (depth == 0) if (depth == 0)
RecordUnwindResult(SamplingProfilerUnwindResult::kFirstFrameUnmapped); RecordUnwindResult(SamplingProfilerUnwindResult::kFirstFrameUnmapped);
return depth; return depth;
......
...@@ -71,10 +71,14 @@ TEST_F(StackUnwinderTest, UnwindOtherThread) { ...@@ -71,10 +71,14 @@ TEST_F(StackUnwinderTest, UnwindOtherThread) {
size_t result = size_t result =
unwinder->TraceStack(tid, stack_buffer.get(), frames, kMaxStackFrames); unwinder->TraceStack(tid, stack_buffer.get(), frames, kMaxStackFrames);
EXPECT_GT(result, 0u); EXPECT_GT(result, 0u);
bool current_function_found = false;
for (size_t i = 0; i < result; ++i) { for (size_t i = 0; i < result; ++i) {
uintptr_t addr = reinterpret_cast<uintptr_t>(frames[i]); uintptr_t addr = reinterpret_cast<uintptr_t>(frames[i]);
EXPECT_TRUE(unwinder->IsAddressMapped(addr)); EXPECT_TRUE(unwinder->IsAddressMapped(addr));
if (addr >= test_pc && addr < test_pc + 100)
current_function_found = true;
} }
EXPECT_TRUE(current_function_found);
unwind_finished_event->Signal(); unwind_finished_event->Signal();
}; };
...@@ -88,8 +92,6 @@ TEST_F(StackUnwinderTest, UnwindOtherThread) { ...@@ -88,8 +92,6 @@ TEST_F(StackUnwinderTest, UnwindOtherThread) {
// While the background thread is trying to unwind make some slow framework // While the background thread is trying to unwind make some slow framework
// calls (malloc) so that the current thread can be stopped in framework // calls (malloc) so that the current thread can be stopped in framework
// library functions on stack. // library functions on stack.
// TODO(ssid): Test for reliable unwinding through non-chrome and chrome
// frames.
while (true) { while (true) {
std::vector<int> temp; std::vector<int> temp;
temp.reserve(kMaxStackFrames); temp.reserve(kMaxStackFrames);
......
...@@ -72,8 +72,10 @@ class TracingProfileBuilder ...@@ -72,8 +72,10 @@ class TracingProfileBuilder
// For chrome address we do not have symbols on the binary. So, just write // For chrome address we do not have symbols on the binary. So, just write
// the offset address. For addresses on framework libraries, symbolize // the offset address. For addresses on framework libraries, symbolize
// and write the function name. // and write the function name.
if (base::trace_event::CFIBacktraceAndroid::is_chrome_address( if (frame.instruction_pointer == 0) {
frame.instruction_pointer)) { frame_name = "Scanned";
} else if (base::trace_event::CFIBacktraceAndroid::is_chrome_address(
frame.instruction_pointer)) {
frame_name = GetFrameNameFromOffsetAddr( frame_name = GetFrameNameFromOffsetAddr(
frame.instruction_pointer - frame.instruction_pointer -
base::trace_event::CFIBacktraceAndroid::executable_start_addr()); base::trace_event::CFIBacktraceAndroid::executable_start_addr());
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment