From 086baf981d696cb80c564df9c34affdcd2458504 Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Tue, 17 Oct 2017 14:12:52 -0700 Subject: [PATCH] Switch libbacktrace new unwinder. Update the backtrace leak tests to share a map since this is the most common way it will be used, and it runs much faster. Remove the CreateNew functions, and references to them. Remove benchmarks of CreateNew functions. Test: Builds, ran new unit tests, ran art tests. Change-Id: I4a25a412f1a74c6d43aebbebbf32ead20ead8f94 --- libbacktrace/Backtrace.cpp | 29 ----- libbacktrace/UnwindMap.cpp | 21 ---- libbacktrace/UnwindStackMap.cpp | 2 +- libbacktrace/backtrace_benchmarks.cpp | 12 -- libbacktrace/backtrace_test.cpp | 105 +----------------- libbacktrace/include/backtrace/Backtrace.h | 2 - libbacktrace/include/backtrace/BacktraceMap.h | 2 - 7 files changed, 7 insertions(+), 166 deletions(-) diff --git a/libbacktrace/Backtrace.cpp b/libbacktrace/Backtrace.cpp index afe518c5b..e18dbf3db 100644 --- a/libbacktrace/Backtrace.cpp +++ b/libbacktrace/Backtrace.cpp @@ -14,7 +14,6 @@ * limitations under the License. */ -#include #include #include #include @@ -135,34 +134,6 @@ Backtrace* Backtrace::Create(pid_t pid, pid_t tid, BacktraceMap* map) { tid = pid; } - if (pid == getpid()) { - return new UnwindCurrent(pid, tid, map); - } else { - return new UnwindPtrace(pid, tid, map); - } -} - -Backtrace* Backtrace::CreateNew(pid_t pid, pid_t tid, BacktraceMap* map) { - if (pid == BACKTRACE_CURRENT_PROCESS) { - pid = getpid(); - if (tid == BACKTRACE_CURRENT_THREAD) { - tid = gettid(); - } - } else if (tid == BACKTRACE_CURRENT_THREAD) { - tid = pid; - } - - if (map == nullptr) { -// This would cause the wrong type of map object to be created, so disallow. -#if defined(__ANDROID__) - __assert2(__FILE__, __LINE__, __PRETTY_FUNCTION__, - "Backtrace::CreateNew() must be called with a real map pointer."); -#else - BACK_LOGE("Backtrace::CreateNew() must be called with a real map pointer."); - abort(); -#endif - } - if (pid == getpid()) { return new UnwindStackCurrent(pid, tid, map); } else { diff --git a/libbacktrace/UnwindMap.cpp b/libbacktrace/UnwindMap.cpp index 0b8232b2f..3cab0d1b0 100644 --- a/libbacktrace/UnwindMap.cpp +++ b/libbacktrace/UnwindMap.cpp @@ -146,24 +146,3 @@ void UnwindMapLocal::FillIn(uintptr_t addr, backtrace_map_t* map) { } } } - -//------------------------------------------------------------------------- -// BacktraceMap create function. -//------------------------------------------------------------------------- -BacktraceMap* BacktraceMap::Create(pid_t pid, bool uncached) { - BacktraceMap* map; - - if (uncached) { - // Force use of the base class to parse the maps when this call is made. - map = new BacktraceMap(pid); - } else if (pid == getpid()) { - map = new UnwindMapLocal(); - } else { - map = new UnwindMapRemote(pid); - } - if (!map->Build()) { - delete map; - return nullptr; - } - return map; -} diff --git a/libbacktrace/UnwindStackMap.cpp b/libbacktrace/UnwindStackMap.cpp index e7e5e4c43..25e5002ba 100644 --- a/libbacktrace/UnwindStackMap.cpp +++ b/libbacktrace/UnwindStackMap.cpp @@ -103,7 +103,7 @@ std::shared_ptr UnwindStackMap::GetProcessMemory() { //------------------------------------------------------------------------- // BacktraceMap create function. //------------------------------------------------------------------------- -BacktraceMap* BacktraceMap::CreateNew(pid_t pid, bool uncached) { +BacktraceMap* BacktraceMap::Create(pid_t pid, bool uncached) { BacktraceMap* map; if (uncached) { diff --git a/libbacktrace/backtrace_benchmarks.cpp b/libbacktrace/backtrace_benchmarks.cpp index 2593e0530..bb4134fec 100644 --- a/libbacktrace/backtrace_benchmarks.cpp +++ b/libbacktrace/backtrace_benchmarks.cpp @@ -149,12 +149,6 @@ static void BM_create_map(benchmark::State& state) { } BENCHMARK(BM_create_map); -static void BM_create_map_new(benchmark::State& state) { - CreateMap(state, BacktraceMap::CreateNew); -} -BENCHMARK(BM_create_map_new); - - using BacktraceCreateFn = decltype(Backtrace::Create); static void CreateBacktrace(benchmark::State& state, BacktraceMap* map, BacktraceCreateFn fn) { @@ -170,10 +164,4 @@ static void BM_create_backtrace(benchmark::State& state) { } BENCHMARK(BM_create_backtrace); -static void BM_create_backtrace_new(benchmark::State& state) { - std::unique_ptr backtrace_map(BacktraceMap::CreateNew(getpid())); - CreateBacktrace(state, backtrace_map.get(), Backtrace::CreateNew); -} -BENCHMARK(BM_create_backtrace_new); - BENCHMARK_MAIN(); diff --git a/libbacktrace/backtrace_test.cpp b/libbacktrace/backtrace_test.cpp index c3e5da09b..0a60ec4f3 100644 --- a/libbacktrace/backtrace_test.cpp +++ b/libbacktrace/backtrace_test.cpp @@ -368,20 +368,6 @@ TEST(libbacktrace, ptrace_trace) { ASSERT_EQ(waitpid(pid, &status, 0), pid); } -TEST(libbacktrace, ptrace_trace_new) { - pid_t pid; - if ((pid = fork()) == 0) { - ASSERT_NE(test_level_one(1, 2, 3, 4, nullptr, nullptr), 0); - _exit(1); - } - VerifyProcTest(pid, BACKTRACE_CURRENT_THREAD, ReadyLevelBacktrace, VerifyLevelDump, - Backtrace::CreateNew, BacktraceMap::CreateNew); - - kill(pid, SIGKILL); - int status; - ASSERT_EQ(waitpid(pid, &status, 0), pid); -} - TEST(libbacktrace, ptrace_max_trace) { pid_t pid; if ((pid = fork()) == 0) { @@ -396,20 +382,6 @@ TEST(libbacktrace, ptrace_max_trace) { ASSERT_EQ(waitpid(pid, &status, 0), pid); } -TEST(libbacktrace, ptrace_max_trace_new) { - pid_t pid; - if ((pid = fork()) == 0) { - ASSERT_NE(test_recursive_call(MAX_BACKTRACE_FRAMES + 10, nullptr, nullptr), 0); - _exit(1); - } - VerifyProcTest(pid, BACKTRACE_CURRENT_THREAD, ReadyMaxBacktrace, VerifyMaxDump, - Backtrace::CreateNew, BacktraceMap::CreateNew); - - kill(pid, SIGKILL); - int status; - ASSERT_EQ(waitpid(pid, &status, 0), pid); -} - static void VerifyProcessIgnoreFrames(Backtrace* bt_all, create_func_t create_func, map_create_func_t map_create_func) { std::unique_ptr map(map_create_func(bt_all->Pid(), false)); @@ -440,20 +412,6 @@ TEST(libbacktrace, ptrace_ignore_frames) { ASSERT_EQ(waitpid(pid, &status, 0), pid); } -TEST(libbacktrace, ptrace_ignore_frames_new) { - pid_t pid; - if ((pid = fork()) == 0) { - ASSERT_NE(test_level_one(1, 2, 3, 4, nullptr, nullptr), 0); - _exit(1); - } - VerifyProcTest(pid, BACKTRACE_CURRENT_THREAD, ReadyLevelBacktrace, VerifyProcessIgnoreFrames, - Backtrace::CreateNew, BacktraceMap::CreateNew); - - kill(pid, SIGKILL); - int status; - ASSERT_EQ(waitpid(pid, &status, 0), pid); -} - // Create a process with multiple threads and dump all of the threads. static void* PtraceThreadLevelRun(void*) { EXPECT_NE(test_level_one(1, 2, 3, 4, nullptr, nullptr), 0); @@ -517,45 +475,6 @@ TEST(libbacktrace, ptrace_threads) { FinishRemoteProcess(pid); } -TEST(libbacktrace, ptrace_threads_new) { - pid_t pid; - if ((pid = fork()) == 0) { - for (size_t i = 0; i < NUM_PTRACE_THREADS; i++) { - pthread_attr_t attr; - pthread_attr_init(&attr); - pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED); - - pthread_t thread; - ASSERT_TRUE(pthread_create(&thread, &attr, PtraceThreadLevelRun, nullptr) == 0); - } - ASSERT_NE(test_level_one(1, 2, 3, 4, nullptr, nullptr), 0); - _exit(1); - } - - // Check to see that all of the threads are running before unwinding. - std::vector threads; - uint64_t start = NanoTime(); - do { - usleep(US_PER_MSEC); - threads.clear(); - GetThreads(pid, &threads); - } while ((threads.size() != NUM_PTRACE_THREADS + 1) && ((NanoTime() - start) <= 5 * NS_PER_SEC)); - ASSERT_EQ(threads.size(), static_cast(NUM_PTRACE_THREADS + 1)); - - ASSERT_TRUE(ptrace(PTRACE_ATTACH, pid, 0, 0) == 0); - WaitForStop(pid); - for (std::vector::const_iterator it = threads.begin(); it != threads.end(); ++it) { - // Skip the current forked process, we only care about the threads. - if (pid == *it) { - continue; - } - VerifyProcTest(pid, *it, ReadyLevelBacktrace, VerifyLevelDump, Backtrace::CreateNew, - BacktraceMap::CreateNew); - } - - FinishRemoteProcess(pid); -} - void VerifyLevelThread(void*) { std::unique_ptr backtrace(Backtrace::Create(getpid(), gettid())); ASSERT_TRUE(backtrace.get() != nullptr); @@ -1663,7 +1582,7 @@ TEST(libbacktrace, unwind_disallow_device_map_local) { munmap(device_map, DEVICE_MAP_SIZE); } -TEST(libbacktrace, unwind_disallow_device_map_remote_new) { +TEST(libbacktrace, unwind_disallow_device_map_remote) { void* device_map; SetupDeviceMap(&device_map); @@ -1672,9 +1591,7 @@ TEST(libbacktrace, unwind_disallow_device_map_remote_new) { CreateRemoteProcess(&pid); // Now create an unwind object. - std::unique_ptr map(BacktraceMap::CreateNew(pid)); - ASSERT_TRUE(map.get() != nullptr); - std::unique_ptr backtrace(Backtrace::CreateNew(pid, pid, map.get())); + std::unique_ptr backtrace(Backtrace::Create(pid, pid)); UnwindFromDevice(backtrace.get(), device_map); @@ -1832,18 +1749,10 @@ TEST(libbacktrace, unwind_remote_through_signal_using_handler) { UnwindThroughSignal(false, Backtrace::Create, BacktraceMap::Create); } -TEST(libbacktrace, unwind_remote_through_signal_using_handler_new) { - UnwindThroughSignal(false, Backtrace::CreateNew, BacktraceMap::CreateNew); -} - TEST(libbacktrace, unwind_remote_through_signal_using_action) { UnwindThroughSignal(true, Backtrace::Create, BacktraceMap::Create); } -TEST(libbacktrace, unwind_remote_through_signal_using_action_new) { - UnwindThroughSignal(true, Backtrace::CreateNew, BacktraceMap::CreateNew); -} - static void TestFrameSkipNumbering(create_func_t create_func, map_create_func_t map_create_func) { std::unique_ptr map(map_create_func(getpid(), false)); std::unique_ptr backtrace(create_func(getpid(), gettid(), map.get())); @@ -1856,19 +1765,17 @@ TEST(libbacktrace, unwind_frame_skip_numbering) { TestFrameSkipNumbering(Backtrace::Create, BacktraceMap::Create); } -TEST(libbacktrace, unwind_frame_skip_numbering_new) { - TestFrameSkipNumbering(Backtrace::CreateNew, BacktraceMap::CreateNew); -} - #if defined(ENABLE_PSS_TESTS) #include "GetPss.h" #define MAX_LEAK_BYTES (32*1024UL) static void CheckForLeak(pid_t pid, pid_t tid) { + std::unique_ptr map(BacktraceMap::Create(pid)); + // Do a few runs to get the PSS stable. for (size_t i = 0; i < 100; i++) { - Backtrace* backtrace = Backtrace::Create(pid, tid); + Backtrace* backtrace = Backtrace::Create(pid, tid, map.get()); ASSERT_TRUE(backtrace != nullptr); ASSERT_TRUE(backtrace->Unwind(0)); ASSERT_EQ(BACKTRACE_UNWIND_NO_ERROR, backtrace->GetError()); @@ -1879,7 +1786,7 @@ static void CheckForLeak(pid_t pid, pid_t tid) { // Loop enough that even a small leak should be detectable. for (size_t i = 0; i < 4096; i++) { - Backtrace* backtrace = Backtrace::Create(pid, tid); + Backtrace* backtrace = Backtrace::Create(pid, tid, map.get()); ASSERT_TRUE(backtrace != nullptr); ASSERT_TRUE(backtrace->Unwind(0)); ASSERT_EQ(BACKTRACE_UNWIND_NO_ERROR, backtrace->GetError()); diff --git a/libbacktrace/include/backtrace/Backtrace.h b/libbacktrace/include/backtrace/Backtrace.h index 274c64bb6..73a58b5ee 100644 --- a/libbacktrace/include/backtrace/Backtrace.h +++ b/libbacktrace/include/backtrace/Backtrace.h @@ -94,8 +94,6 @@ public: // If map is NULL, then create the map and manage it internally. // If map is not NULL, the map is still owned by the caller. static Backtrace* Create(pid_t pid, pid_t tid, BacktraceMap* map = NULL); - // Same as above, but uses a different underlying unwinder. - static Backtrace* CreateNew(pid_t pid, pid_t tid, BacktraceMap* map = NULL); // Create an offline Backtrace object that can be used to do an unwind without a process // that is still running. If cache_file is set to true, then elf information will be cached diff --git a/libbacktrace/include/backtrace/BacktraceMap.h b/libbacktrace/include/backtrace/BacktraceMap.h index 84e71327d..d0783929a 100644 --- a/libbacktrace/include/backtrace/BacktraceMap.h +++ b/libbacktrace/include/backtrace/BacktraceMap.h @@ -56,8 +56,6 @@ public: // Passing a map created with uncached set to true to Backtrace::Create() // is unsupported. static BacktraceMap* Create(pid_t pid, bool uncached = false); - // Same as above, but is compatible with the new unwinder. - static BacktraceMap* CreateNew(pid_t pid, bool uncached = false); static BacktraceMap* Create(pid_t pid, const std::vector& maps);