From edccd84763c96fedb88b66f413de534fd302d882 Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Wed, 6 Sep 2017 14:15:28 -0700 Subject: [PATCH] Fix UnwindTest repeatability. - Rewrite some of the UnwindTest tests to properly wait for the process to be ready. - Add a TestScopedPidReaper to make sure that fork process get killed even if the test fails. Add this to all tests that fail. - Create a quiesce function to be used by all of the tests that will wait after attaching to a process. Bug: 65287279 Test: Ran unit tests on hikey960 board and on host repeatedly. Change-Id: I57084120396f34d8dfb852f3d814bef2056f1b54 --- libunwindstack/tests/MemoryRemoteTest.cpp | 31 ++------ libunwindstack/tests/TestUtils.h | 55 +++++++++++++ libunwindstack/tests/UnwindTest.cpp | 96 ++++++++++++----------- 3 files changed, 111 insertions(+), 71 deletions(-) create mode 100644 libunwindstack/tests/TestUtils.h diff --git a/libunwindstack/tests/MemoryRemoteTest.cpp b/libunwindstack/tests/MemoryRemoteTest.cpp index f8965b2ce..a66d0c550 100644 --- a/libunwindstack/tests/MemoryRemoteTest.cpp +++ b/libunwindstack/tests/MemoryRemoteTest.cpp @@ -22,7 +22,6 @@ #include #include #include -#include #include #include @@ -34,32 +33,18 @@ #include #include "MemoryFake.h" +#include "TestUtils.h" namespace unwindstack { class MemoryRemoteTest : public ::testing::Test { protected: - static uint64_t NanoTime() { - struct timespec t = { 0, 0 }; - clock_gettime(CLOCK_MONOTONIC, &t); - return static_cast(t.tv_sec * NS_PER_SEC + t.tv_nsec); - } - static bool Attach(pid_t pid) { if (ptrace(PTRACE_ATTACH, pid, 0, 0) == -1) { return false; } - uint64_t start = NanoTime(); - siginfo_t si; - while (TEMP_FAILURE_RETRY(ptrace(PTRACE_GETSIGINFO, pid, 0, &si)) < 0 && errno == ESRCH) { - if ((NanoTime() - start) > 10 * NS_PER_SEC) { - printf("%d: Failed to stop after 10 seconds.\n", pid); - return false; - } - usleep(30); - } - return true; + return TestQuiescePid(pid); } static bool Detach(pid_t pid) { @@ -79,6 +64,7 @@ TEST_F(MemoryRemoteTest, read) { exit(1); } ASSERT_LT(0, pid); + TestScopedPidReaper reap(pid); ASSERT_TRUE(Attach(pid)); @@ -91,9 +77,6 @@ TEST_F(MemoryRemoteTest, read) { } ASSERT_TRUE(Detach(pid)); - - kill(pid, SIGKILL); - ASSERT_EQ(pid, wait(nullptr)); } TEST_F(MemoryRemoteTest, read_fail) { @@ -111,6 +94,7 @@ TEST_F(MemoryRemoteTest, read_fail) { exit(1); } ASSERT_LT(0, pid); + TestScopedPidReaper reap(pid); ASSERT_TRUE(Attach(pid)); @@ -132,9 +116,6 @@ TEST_F(MemoryRemoteTest, read_fail) { ASSERT_EQ(0, munmap(src, pagesize)); ASSERT_TRUE(Detach(pid)); - - kill(pid, SIGKILL); - ASSERT_EQ(pid, wait(nullptr)); } TEST_F(MemoryRemoteTest, read_overflow) { @@ -152,6 +133,7 @@ TEST_F(MemoryRemoteTest, read_illegal) { exit(1); } ASSERT_LT(0, pid); + TestScopedPidReaper reap(pid); ASSERT_TRUE(Attach(pid)); @@ -162,9 +144,6 @@ TEST_F(MemoryRemoteTest, read_illegal) { ASSERT_FALSE(remote.Read(0, dst.data(), 100)); ASSERT_TRUE(Detach(pid)); - - kill(pid, SIGKILL); - ASSERT_EQ(pid, wait(nullptr)); } } // namespace unwindstack diff --git a/libunwindstack/tests/TestUtils.h b/libunwindstack/tests/TestUtils.h new file mode 100644 index 000000000..8c31aa6e5 --- /dev/null +++ b/libunwindstack/tests/TestUtils.h @@ -0,0 +1,55 @@ +/* + * Copyright (C) 2017 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef _LIBUNWINDSTACK_TESTS_TEST_UTILS_H +#define _LIBUNWINDSTACK_TESTS_TEST_UTILS_H + +#include +#include +#include +#include + +namespace unwindstack { + +class TestScopedPidReaper { + public: + TestScopedPidReaper(pid_t pid) : pid_(pid) {} + ~TestScopedPidReaper() { + kill(pid_, SIGKILL); + waitpid(pid_, nullptr, 0); + } + + private: + pid_t pid_; +}; + +inline bool TestQuiescePid(pid_t pid) { + siginfo_t si; + bool ready = false; + // Wait for up to 5 seconds. + for (size_t i = 0; i < 5000; i++) { + if (ptrace(PTRACE_GETSIGINFO, pid, 0, &si) == 0) { + ready = true; + break; + } + usleep(1000); + } + return ready; +} + +} // namespace unwindstack + +#endif // _LIBUNWINDSTACK_TESTS_TEST_UTILS_H diff --git a/libunwindstack/tests/UnwindTest.cpp b/libunwindstack/tests/UnwindTest.cpp index a0c45bdf3..a4f920ad9 100644 --- a/libunwindstack/tests/UnwindTest.cpp +++ b/libunwindstack/tests/UnwindTest.cpp @@ -15,10 +15,9 @@ */ #include -#include - #include #include +#include #include #include #include @@ -39,14 +38,24 @@ #include #include +#include "TestUtils.h" + namespace unwindstack { -static std::atomic_bool g_ready(false); -static volatile bool g_ready_for_remote = false; -static volatile bool g_signal_ready_for_remote = false; -static std::atomic_bool g_finish(false); +static std::atomic_bool g_ready; +static volatile bool g_ready_for_remote; +static volatile bool g_signal_ready_for_remote; +static std::atomic_bool g_finish; static std::atomic_uintptr_t g_ucontext; +static void ResetGlobals() { + g_ready = false; + g_ready_for_remote = false; + g_signal_ready_for_remote = false; + g_finish = false; + g_ucontext = 0; +} + static std::vector kFunctionOrder{"InnerFunction", "MiddleFunction", "OuterFunction"}; static std::vector kFunctionSignalOrder{"SignalInnerFunction", "SignalMiddleFunction", @@ -156,48 +165,52 @@ extern "C" void OuterFunction(bool local) { MiddleFunction(local); } -TEST(UnwindTest, local) { +class UnwindTest : public ::testing::Test { + public: + void SetUp() override { ResetGlobals(); } +}; + +TEST_F(UnwindTest, local) { OuterFunction(true); } void WaitForRemote(pid_t pid, uint64_t addr, bool leave_attached, bool* completed) { *completed = false; // Need to sleep before attempting first ptrace. Without this, on the - // host it becomes impossible to attach and ptrace set errno to EPERM. + // host it becomes impossible to attach and ptrace sets errno to EPERM. usleep(1000); - for (size_t i = 0; i < 100; i++) { - ASSERT_EQ(0, ptrace(PTRACE_ATTACH, pid, 0, 0)); - for (size_t j = 0; j < 100; j++) { - siginfo_t si; - if (ptrace(PTRACE_GETSIGINFO, pid, 0, &si) == 0) { - MemoryRemote memory(pid); - // Read the remote value to see if we are ready. - bool value; - if (memory.Read(addr, &value, sizeof(value)) && value) { - *completed = true; - break; - } + for (size_t i = 0; i < 1000; i++) { + if (ptrace(PTRACE_ATTACH, pid, 0, 0) == 0) { + ASSERT_TRUE(TestQuiescePid(pid)) + << "Waiting for process to quiesce failed: " << strerror(errno); + + MemoryRemote memory(pid); + // Read the remote value to see if we are ready. + bool value; + if (memory.Read(addr, &value, sizeof(value)) && value) { + *completed = true; } - usleep(1000); + if (!*completed || !leave_attached) { + ASSERT_EQ(0, ptrace(PTRACE_DETACH, pid, 0, 0)); + } + if (*completed) { + break; + } + } else { + ASSERT_EQ(ESRCH, errno) << "ptrace attach failed with unexpected error: " << strerror(errno); } - if (leave_attached && *completed) { - break; - } - ASSERT_EQ(0, ptrace(PTRACE_DETACH, pid, 0, 0)); - if (*completed) { - break; - } - usleep(1000); + usleep(5000); } } -TEST(UnwindTest, remote) { +TEST_F(UnwindTest, remote) { pid_t pid; if ((pid = fork()) == 0) { OuterFunction(false); exit(0); } ASSERT_NE(-1, pid); + TestScopedPidReaper reap(pid); bool completed; WaitForRemote(pid, reinterpret_cast(&g_ready_for_remote), true, &completed); @@ -210,13 +223,11 @@ TEST(UnwindTest, remote) { VerifyUnwind(pid, &maps, regs.get(), kFunctionOrder); - ASSERT_EQ(0, ptrace(PTRACE_DETACH, pid, 0, 0)); - - kill(pid, SIGKILL); - ASSERT_EQ(pid, wait(nullptr)); + ASSERT_EQ(0, ptrace(PTRACE_DETACH, pid, 0, 0)) + << "ptrace detach failed with unexpected error: " << strerror(errno); } -TEST(UnwindTest, from_context) { +TEST_F(UnwindTest, from_context) { std::atomic_int tid(0); std::thread thread([&]() { tid = syscall(__NR_gettid); @@ -263,10 +274,6 @@ TEST(UnwindTest, from_context) { } static void RemoteThroughSignal(unsigned int sa_flags) { - g_ready = false; - g_signal_ready_for_remote = false; - g_finish = false; - pid_t pid; if ((pid = fork()) == 0) { struct sigaction act, oldact; @@ -279,6 +286,7 @@ static void RemoteThroughSignal(unsigned int sa_flags) { exit(0); } ASSERT_NE(-1, pid); + TestScopedPidReaper reap(pid); bool completed; WaitForRemote(pid, reinterpret_cast(&g_ready_for_remote), false, &completed); @@ -294,17 +302,15 @@ static void RemoteThroughSignal(unsigned int sa_flags) { VerifyUnwind(pid, &maps, regs.get(), kFunctionSignalOrder); - ASSERT_EQ(0, ptrace(PTRACE_DETACH, pid, 0, 0)); - - kill(pid, SIGKILL); - ASSERT_EQ(pid, wait(nullptr)); + ASSERT_EQ(0, ptrace(PTRACE_DETACH, pid, 0, 0)) + << "ptrace detach failed with unexpected error: " << strerror(errno); } -TEST(UnwindTest, remote_through_signal) { +TEST_F(UnwindTest, remote_through_signal) { RemoteThroughSignal(0); } -TEST(UnwindTest, remote_through_signal_sa_siginfo) { +TEST_F(UnwindTest, remote_through_signal_sa_siginfo) { RemoteThroughSignal(SA_SIGINFO); }