From 899c1bdfa185f1432e4b955a38478e4a984675fb Mon Sep 17 00:00:00 2001 From: Dimitry Ivanov Date: Fri, 10 May 2024 10:02:07 +0000 Subject: [PATCH 1/2] Revert "[Berberis][CrashReporting] Dump guest thread info to tom..." Revert submission 3062926 Reason for revert: We want guest state to be present in all threads - revert to be able to fix the proto field type. Reverted changes: /q/submissionid:3062926 Change-Id: I87b282a0d9caebe4eae2e7d8eca8ec8ebaa3eca6 --- debuggerd/crash_dump.cpp | 39 +------------------ .../include/libdebuggerd/tombstone.h | 8 +--- debuggerd/libdebuggerd/tombstone.cpp | 6 +-- debuggerd/libdebuggerd/tombstone_proto.cpp | 34 ++-------------- .../libdebuggerd/tombstone_proto_to_text.cpp | 37 ++---------------- debuggerd/proto/tombstone.proto | 7 +--- 6 files changed, 14 insertions(+), 117 deletions(-) diff --git a/debuggerd/crash_dump.cpp b/debuggerd/crash_dump.cpp index 4640aebd5..bc00c86a8 100644 --- a/debuggerd/crash_dump.cpp +++ b/debuggerd/crash_dump.cpp @@ -30,7 +30,6 @@ #include #include #include -#include #include #include @@ -82,10 +81,6 @@ using android::base::ErrnoRestorer; using android::base::StringPrintf; using android::base::unique_fd; -// This stores guest architecture. When the architecture is supported, tombstone file will output -// guest state information. -static Architecture g_guest_arch; - static bool pid_contains_tid(int pid_proc_fd, pid_t tid) { struct stat st; std::string task_path = StringPrintf("task/%d", tid); @@ -495,8 +490,6 @@ static void ReadGuestRegisters(std::unique_ptr* regs, pid_t t arm_user_regs.regs[i] = guest_regs.regs_arm.r[i]; } regs->reset(unwindstack::RegsArm::Read(&arm_user_regs)); - - g_guest_arch = Architecture::ARM32; break; } #if defined(__LP64__) @@ -508,8 +501,6 @@ static void ReadGuestRegisters(std::unique_ptr* regs, pid_t t arm64_user_regs.sp = guest_regs.regs_arm64.sp; arm64_user_regs.pc = guest_regs.regs_arm64.ip; regs->reset(unwindstack::RegsArm64::Read(&arm64_user_regs)); - - g_guest_arch = Architecture::ARM64; break; } case NATIVE_BRIDGE_ARCH_RISCV64: { @@ -520,8 +511,6 @@ static void ReadGuestRegisters(std::unique_ptr* regs, pid_t t riscv64_user_regs.regs[i] = guest_regs.regs_riscv64.x[i]; } regs->reset(unwindstack::RegsRiscv64::Read(&riscv64_user_regs, tid)); - - g_guest_arch = Architecture::RISCV64; break; } #endif @@ -786,32 +775,8 @@ int main(int argc, char** argv) { { ATRACE_NAME("engrave_tombstone"); - unwindstack::ArchEnum regs_arch = unwindstack::ARCH_UNKNOWN; - switch (g_guest_arch) { - case Architecture::ARM32: { - regs_arch = unwindstack::ARCH_ARM; - break; - } - case Architecture::ARM64: { - regs_arch = unwindstack::ARCH_ARM64; - break; - } - case Architecture::RISCV64: { - regs_arch = unwindstack::ARCH_RISCV64; - break; - } - default: { - } - } - if (regs_arch == unwindstack::ARCH_UNKNOWN) { - engrave_tombstone(std::move(g_output_fd), std::move(g_proto_fd), &unwinder, thread_info, - g_target_thread, process_info, &open_files, &amfd_data); - } else { - unwindstack::AndroidRemoteUnwinder guest_unwinder(vm_pid, regs_arch); - engrave_tombstone(std::move(g_output_fd), std::move(g_proto_fd), &unwinder, thread_info, - g_target_thread, process_info, &open_files, &amfd_data, &g_guest_arch, - &guest_unwinder); - } + engrave_tombstone(std::move(g_output_fd), std::move(g_proto_fd), &unwinder, thread_info, + g_target_thread, process_info, &open_files, &amfd_data); } } diff --git a/debuggerd/libdebuggerd/include/libdebuggerd/tombstone.h b/debuggerd/libdebuggerd/include/libdebuggerd/tombstone.h index 6371dbe9c..be999e04e 100644 --- a/debuggerd/libdebuggerd/include/libdebuggerd/tombstone.h +++ b/debuggerd/libdebuggerd/include/libdebuggerd/tombstone.h @@ -28,7 +28,6 @@ #include #include "open_files_list.h" -#include "tombstone.pb.h" #include "types.h" // Forward declarations @@ -55,17 +54,14 @@ void engrave_tombstone(android::base::unique_fd output_fd, android::base::unique unwindstack::AndroidUnwinder* unwinder, const std::map& thread_info, pid_t target_thread, const ProcessInfo& process_info, OpenFilesList* open_files, - std::string* amfd_data, Architecture* guest_arch = nullptr, - unwindstack::AndroidUnwinder* guest_unwinder = nullptr); + std::string* amfd_data); void engrave_tombstone_ucontext(int tombstone_fd, int proto_fd, uint64_t abort_msg_address, siginfo_t* siginfo, ucontext_t* ucontext); void engrave_tombstone_proto(Tombstone* tombstone, unwindstack::AndroidUnwinder* unwinder, const std::map& threads, pid_t target_thread, - const ProcessInfo& process_info, const OpenFilesList* open_files, - Architecture* guest_arch, - unwindstack::AndroidUnwinder* guest_unwinder); + const ProcessInfo& process_info, const OpenFilesList* open_files); bool tombstone_proto_to_text( const Tombstone& tombstone, diff --git a/debuggerd/libdebuggerd/tombstone.cpp b/debuggerd/libdebuggerd/tombstone.cpp index 0da2ca7b8..5a416d643 100644 --- a/debuggerd/libdebuggerd/tombstone.cpp +++ b/debuggerd/libdebuggerd/tombstone.cpp @@ -125,12 +125,10 @@ void engrave_tombstone(unique_fd output_fd, unique_fd proto_fd, unwindstack::AndroidUnwinder* unwinder, const std::map& threads, pid_t target_thread, const ProcessInfo& process_info, OpenFilesList* open_files, - std::string* amfd_data, Architecture* guest_arch, - unwindstack::AndroidUnwinder* guest_unwinder) { + std::string* amfd_data) { // Don't copy log messages to tombstone unless this is a development device. Tombstone tombstone; - engrave_tombstone_proto(&tombstone, unwinder, threads, target_thread, process_info, open_files, - guest_arch, guest_unwinder); + engrave_tombstone_proto(&tombstone, unwinder, threads, target_thread, process_info, open_files); if (proto_fd != -1) { if (!tombstone.SerializeToFileDescriptor(proto_fd.get())) { diff --git a/debuggerd/libdebuggerd/tombstone_proto.cpp b/debuggerd/libdebuggerd/tombstone_proto.cpp index 5e170df0b..5546b7bcc 100644 --- a/debuggerd/libdebuggerd/tombstone_proto.cpp +++ b/debuggerd/libdebuggerd/tombstone_proto.cpp @@ -482,8 +482,7 @@ static void dump_thread_backtrace(std::vector& frames, T } static void dump_thread(Tombstone* tombstone, unwindstack::AndroidUnwinder* unwinder, - const ThreadInfo& thread_info, bool memory_dump = false, - unwindstack::AndroidUnwinder* guest_unwinder = nullptr) { + const ThreadInfo& thread_info, bool memory_dump = false) { Thread thread; thread.set_id(thread_info.tid); @@ -510,26 +509,6 @@ static void dump_thread(Tombstone* tombstone, unwindstack::AndroidUnwinder* unwi auto& threads = *tombstone->mutable_threads(); threads[thread_info.tid] = thread; - - if (guest_unwinder) { - if (!thread_info.guest_registers) { - async_safe_format_log(ANDROID_LOG_INFO, LOG_TAG, - "No guest state registers information for tid %d", thread_info.tid); - return; - } - Thread guest_thread; - unwindstack::AndroidUnwinderData guest_data; - guest_data.saved_initial_regs = std::make_optional>(); - if (guest_unwinder->Unwind(thread_info.guest_registers.get(), guest_data)) { - dump_thread_backtrace(guest_data.frames, guest_thread); - } else { - async_safe_format_log(ANDROID_LOG_ERROR, LOG_TAG, - "Unwind guest state registers failed for tid %d: Error %s", - thread_info.tid, guest_data.GetErrorString().c_str()); - } - dump_registers(guest_unwinder, *guest_data.saved_initial_regs, guest_thread, memory_dump); - *tombstone->mutable_guest_thread() = guest_thread; - } } static void dump_mappings(Tombstone* tombstone, unwindstack::Maps* maps, @@ -707,17 +686,10 @@ static void dump_tags_around_fault_addr(Signal* signal, const Tombstone& tombsto void engrave_tombstone_proto(Tombstone* tombstone, unwindstack::AndroidUnwinder* unwinder, const std::map& threads, pid_t target_tid, - const ProcessInfo& process_info, const OpenFilesList* open_files, - Architecture* guest_arch, - unwindstack::AndroidUnwinder* guest_unwinder) { + const ProcessInfo& process_info, const OpenFilesList* open_files) { Tombstone result; result.set_arch(get_arch()); - if (guest_arch != nullptr) { - result.set_guest_arch(*guest_arch); - } else { - result.set_guest_arch(Architecture::NONE); - } result.set_build_fingerprint(android::base::GetProperty("ro.build.fingerprint", "unknown")); result.set_revision(android::base::GetProperty("ro.revision", "unknown")); result.set_timestamp(get_timestamp()); @@ -778,7 +750,7 @@ void engrave_tombstone_proto(Tombstone* tombstone, unwindstack::AndroidUnwinder* dump_abort_message(&result, unwinder->GetProcessMemory(), process_info); dump_crash_details(&result, unwinder->GetProcessMemory(), process_info); // Dump the target thread, but save the memory around the registers. - dump_thread(&result, unwinder, target_thread, /* memory_dump */ true, guest_unwinder); + dump_thread(&result, unwinder, target_thread, /* memory_dump */ true); for (const auto& [tid, thread_info] : threads) { if (tid != target_tid) { diff --git a/debuggerd/libdebuggerd/tombstone_proto_to_text.cpp b/debuggerd/libdebuggerd/tombstone_proto_to_text.cpp index 7b296923b..08c1cc0da 100644 --- a/debuggerd/libdebuggerd/tombstone_proto_to_text.cpp +++ b/debuggerd/libdebuggerd/tombstone_proto_to_text.cpp @@ -79,8 +79,8 @@ static std::string describe_pac_enabled_keys(long value) { return describe_end(value, desc); } -static const char* abi_string(const Architecture& arch) { - switch (arch) { +static const char* abi_string(const Tombstone& tombstone) { + switch (tombstone.arch()) { case Architecture::ARM32: return "arm"; case Architecture::ARM64: @@ -578,37 +578,11 @@ void print_logs(CallbackType callback, const Tombstone& tombstone, int tail) { } } -static void print_guest_thread(CallbackType callback, const Tombstone& tombstone, - const Thread& guest_thread) { - CBS("--- --- --- --- --- --- --- --- --- --- --- --- --- --- --- ---"); - CBS("Guest thread information"); - print_thread_registers(callback, tombstone, guest_thread, true); - - CBS(""); - CB(true, "%d total frames", guest_thread.current_backtrace().size()); - CB(true, "backtrace:"); - print_backtrace(callback, tombstone, guest_thread.current_backtrace(), true); - - print_thread_memory_dump(callback, tombstone, guest_thread); - - CBS(""); - - // No memory maps to print. - if (!tombstone.memory_mappings().empty()) { - print_memory_maps(callback, tombstone); - } else { - CBS("No memory maps found"); - } -} - bool tombstone_proto_to_text(const Tombstone& tombstone, CallbackType callback) { CBL("*** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***"); CBL("Build fingerprint: '%s'", tombstone.build_fingerprint().c_str()); CBL("Revision: '%s'", tombstone.revision().c_str()); - CBL("ABI: '%s'", abi_string(tombstone.arch())); - if (tombstone.guest_arch() != Architecture::NONE) { - CBL("Guest architecture: '%s'", abi_string(tombstone.guest_arch())); - } + CBL("ABI: '%s'", abi_string(tombstone)); CBL("Timestamp: %s", tombstone.timestamp().c_str()); CBL("Process uptime: %ds", tombstone.process_uptime()); @@ -661,10 +635,5 @@ bool tombstone_proto_to_text(const Tombstone& tombstone, CallbackType callback) print_logs(callback, tombstone, 0); - // Process guest thread - if (tombstone.has_guest_thread()) { - print_guest_thread(callback, tombstone, tombstone.guest_thread()); - } - return true; } diff --git a/debuggerd/proto/tombstone.proto b/debuggerd/proto/tombstone.proto index 4ddc54ff3..e70d525ee 100644 --- a/debuggerd/proto/tombstone.proto +++ b/debuggerd/proto/tombstone.proto @@ -22,7 +22,6 @@ message CrashDetail { message Tombstone { Architecture arch = 1; - Architecture guest_arch = 24; string build_fingerprint = 2; string revision = 3; string timestamp = 4; @@ -43,7 +42,6 @@ message Tombstone { repeated Cause causes = 15; map threads = 16; - Thread guest_thread = 25; repeated MemoryMapping memory_mappings = 17; repeated LogBuffer log_buffers = 18; repeated FD open_fds = 19; @@ -51,7 +49,7 @@ message Tombstone { uint32 page_size = 22; bool has_been_16kb_mode = 23; - reserved 26 to 999; + reserved 24 to 999; } enum Architecture { @@ -60,9 +58,8 @@ enum Architecture { X86 = 2; X86_64 = 3; RISCV64 = 4; - NONE = 5; - reserved 6 to 999; + reserved 5 to 999; } message Signal { From cdf499f9cdfe35711ad011d133ccbc6565960c37 Mon Sep 17 00:00:00 2001 From: Dimitry Ivanov Date: Fri, 10 May 2024 10:02:07 +0000 Subject: [PATCH 2/2] Revert "[Berberis][CrashReporting] Extend ThreadInfo to have gue..." Revert submission 3062926 Reason for revert: We want guest state to be present in all threads - revert to be able to fix the proto field type. Reverted changes: /q/submissionid:3062926 Change-Id: I32b745cca95a619b78bdce0a7d948ac479d42f21 --- debuggerd/Android.bp | 12 -- debuggerd/crash_dump.cpp | 115 ------------------ .../libdebuggerd/include/libdebuggerd/types.h | 2 - 3 files changed, 129 deletions(-) diff --git a/debuggerd/Android.bp b/debuggerd/Android.bp index 0c5543ed9..3a882ea8f 100644 --- a/debuggerd/Android.bp +++ b/debuggerd/Android.bp @@ -452,7 +452,6 @@ cc_binary { header_libs: [ "bionic_libc_platform_headers", - "libnative_bridge_support_accessor_headers", ], static_libs: [ @@ -462,8 +461,6 @@ cc_binary { "libtombstone_proto", "libprotobuf-cpp-lite", - - "libnative_bridge_guest_state_accessor", ], shared_libs: [ @@ -479,15 +476,6 @@ cc_binary { // Required for tests. required: ["crash_dump.policy"], - - target: { - android: { - header_libs: [ - "libnative_bridge_support_accessor_headers", // For dlext_namespaces.h - ], - shared_libs: ["libdl_android"], // For android_get_exported_namespace implementation - }, - }, } cc_binary { diff --git a/debuggerd/crash_dump.cpp b/debuggerd/crash_dump.cpp index bc00c86a8..203b4855b 100644 --- a/debuggerd/crash_dump.cpp +++ b/debuggerd/crash_dump.cpp @@ -25,7 +25,6 @@ #include #include -#include #include #include #include @@ -43,7 +42,6 @@ #include #include #include -#include #include #include #include @@ -54,18 +52,7 @@ #include #include -#include -#include -#include #include -#include -#include -#include -#include -#include -#include - -#include #include "libdebuggerd/backtrace.h" #include "libdebuggerd/tombstone.h" @@ -416,107 +403,6 @@ static void InstallSigPipeHandler() { sigaction(SIGPIPE, &action, nullptr); } -static bool PtracePeek(int request, pid_t tid, uintptr_t addr, void* data, std::string err_msg, - uintptr_t* result) { - errno = 0; - *result = ptrace(request, tid, addr, data); - if (errno != 0) { - PLOG(ERROR) << err_msg; - return false; - } - return true; -} - -static bool GetGuestRegistersFromCrashedProcess([[maybe_unused]] pid_t tid, - NativeBridgeGuestRegs* guest_regs) { - auto process_memory = unwindstack::Memory::CreateProcessMemoryCached(tid); - - uintptr_t header_ptr = 0; - uintptr_t base = 0; -#if defined(__x86_64__) - if (!PtracePeek(PTRACE_PEEKUSER, tid, offsetof(user_regs_struct, fs_base), nullptr, - "failed to read thread register for thread " + std::to_string(tid), &base)) { - return false; - } -#elif defined(__aarch64__) - // base is implicitly casted to uint64_t. - struct iovec pt_iov { - .iov_base = &base, .iov_len = sizeof(base), - }; - - if (ptrace(PTRACE_GETREGSET, tid, NT_ARM_TLS, &pt_iov) != 0) { - PLOG(ERROR) << "failed to read thread register for thread " << tid; - } -#else - // TODO(b/339287219): Add case for Riscv host. - return false; -#endif - auto ptr_to_guest_slot = base + TLS_SLOT_NATIVE_BRIDGE_GUEST_STATE * sizeof(uintptr_t); - if (!process_memory->ReadFully(ptr_to_guest_slot, &header_ptr, sizeof(uintptr_t))) { - PLOG(ERROR) << "failed to get guest state TLS slot content for thread " << tid; - return false; - } - - NativeBridgeGuestStateHeader header; - if (!process_memory->ReadFully(header_ptr, &header, sizeof(NativeBridgeGuestStateHeader))) { - PLOG(ERROR) << "failed to get the guest state header for thread " << tid; - return false; - } - if (header.signature != NATIVE_BRIDGE_GUEST_STATE_SIGNATURE) { - // Return when ptr points to unmapped memory or no valid guest state. - return false; - } - auto guest_state_data_copy = std::make_unique(header.guest_state_data_size); - if (!process_memory->ReadFully(reinterpret_cast(header.guest_state_data), - guest_state_data_copy.get(), header.guest_state_data_size)) { - PLOG(ERROR) << "failed to read the guest state data for thread " << tid; - return false; - } - - LoadGuestStateRegisters(guest_state_data_copy.get(), header.guest_state_data_size, guest_regs); - return true; -} - -static void ReadGuestRegisters(std::unique_ptr* regs, pid_t tid) { - NativeBridgeGuestRegs guest_regs; - if (!GetGuestRegistersFromCrashedProcess(tid, &guest_regs)) { - return; - } - - switch (guest_regs.guest_arch) { - case NATIVE_BRIDGE_ARCH_ARM: { - unwindstack::arm_user_regs arm_user_regs = {}; - for (size_t i = 0; i < unwindstack::ARM_REG_LAST; i++) { - arm_user_regs.regs[i] = guest_regs.regs_arm.r[i]; - } - regs->reset(unwindstack::RegsArm::Read(&arm_user_regs)); - break; - } -#if defined(__LP64__) - case NATIVE_BRIDGE_ARCH_ARM64: { - unwindstack::arm64_user_regs arm64_user_regs = {}; - for (size_t i = 0; i < unwindstack::ARM64_REG_R31; i++) { - arm64_user_regs.regs[i] = guest_regs.regs_arm64.x[i]; - } - arm64_user_regs.sp = guest_regs.regs_arm64.sp; - arm64_user_regs.pc = guest_regs.regs_arm64.ip; - regs->reset(unwindstack::RegsArm64::Read(&arm64_user_regs)); - break; - } - case NATIVE_BRIDGE_ARCH_RISCV64: { - unwindstack::riscv64_user_regs riscv64_user_regs = {}; - // RISCV64_REG_PC is at the first position. - riscv64_user_regs.regs[0] = guest_regs.regs_riscv64.ip; - for (size_t i = 1; i < unwindstack::RISCV64_REG_REAL_COUNT; i++) { - riscv64_user_regs.regs[i] = guest_regs.regs_riscv64.x[i]; - } - regs->reset(unwindstack::RegsRiscv64::Read(&riscv64_user_regs, tid)); - break; - } -#endif - } -} - int main(int argc, char** argv) { DefuseSignalHandlers(); InstallSigPipeHandler(); @@ -651,7 +537,6 @@ int main(int argc, char** argv) { } if (thread == g_target_thread) { - ReadGuestRegisters(&info.guest_registers, thread); // Read the thread's registers along with the rest of the crash info out of the pipe. ReadCrashInfo(input_pipe, &siginfo, &info.registers, &process_info, &recoverable_crash); info.siginfo = &siginfo; diff --git a/debuggerd/libdebuggerd/include/libdebuggerd/types.h b/debuggerd/libdebuggerd/include/libdebuggerd/types.h index c799f2448..4d69658e2 100644 --- a/debuggerd/libdebuggerd/include/libdebuggerd/types.h +++ b/debuggerd/libdebuggerd/include/libdebuggerd/types.h @@ -39,8 +39,6 @@ struct ThreadInfo { int signo = 0; siginfo_t* siginfo = nullptr; - - std::unique_ptr guest_registers; }; // This struct is written into a pipe from inside the crashing process.