From 50eb546ec1584c04cf18f1941a042d7c5d045a67 Mon Sep 17 00:00:00 2001 From: Brigid Smith Date: Wed, 18 Jun 2014 14:17:57 -0700 Subject: [PATCH] Removed log.quiet and log = NULL cases from debuggerd. Now the functionality implemented by these semi-confusing cases has been replaced with the same logtype enum behavior that is easier to understand, and cases that used log-looking behavior to print to logcat (when log = NULL) now use the more transparent ALOGE/ALOGD functions. Change-Id: I7e38f2d4ca74a828df4d2266b3ea34edd3c6f5bb --- debuggerd/backtrace.cpp | 3 +- debuggerd/debuggerd.cpp | 82 ++++++++++++++++++++--------------------- debuggerd/tombstone.cpp | 31 ++++++++-------- debuggerd/tombstone.h | 4 +- debuggerd/utility.cpp | 24 ++++++------ debuggerd/utility.h | 6 --- 6 files changed, 69 insertions(+), 81 deletions(-) diff --git a/debuggerd/backtrace.cpp b/debuggerd/backtrace.cpp index d08242e76..50bc16702 100644 --- a/debuggerd/backtrace.cpp +++ b/debuggerd/backtrace.cpp @@ -95,7 +95,7 @@ static void dump_thread( } if (!attached && ptrace(PTRACE_DETACH, tid, 0, 0) != 0) { - LOG_ERROR("ptrace detach from %d failed: %s\n", tid, strerror(errno)); + _LOG(log, logtype::ERROR, "ptrace detach from %d failed: %s\n", tid, strerror(errno)); *detach_failed = true; } } @@ -105,7 +105,6 @@ void dump_backtrace(int fd, int amfd, pid_t pid, pid_t tid, bool* detach_failed, log_t log; log.tfd = fd; log.amfd = amfd; - log.quiet = true; dump_process_header(&log, pid); dump_thread(&log, tid, true, detach_failed, total_sleep_time_usec); diff --git a/debuggerd/debuggerd.cpp b/debuggerd/debuggerd.cpp index dde8f9031..c2ef26458 100644 --- a/debuggerd/debuggerd.cpp +++ b/debuggerd/debuggerd.cpp @@ -61,7 +61,7 @@ static void wait_for_user_action(pid_t pid) { char exe[PATH_MAX]; int count; if ((count = readlink(path, exe, sizeof(exe) - 1)) == -1) { - LOG_ERROR("readlink('%s') failed: %s", path, strerror(errno)); + ALOGE("readlink('%s') failed: %s", path, strerror(errno)); strlcpy(exe, "unknown", sizeof(exe)); } else { exe[count] = '\0'; @@ -78,17 +78,17 @@ static void wait_for_user_action(pid_t pid) { } // Explain how to attach the debugger. - LOG_ERROR( "********************************************************\n" - "* Process %d has been suspended while crashing.\n" - "* To attach gdbserver for a gdb connection on port 5039\n" - "* and start gdbclient:\n" - "*\n" - "* gdbclient %s :5039 %d\n" - "*\n" - "* Wait for gdb to start, then press the VOLUME DOWN key\n" - "* to let the process continue crashing.\n" - "********************************************************\n", - pid, name, pid); + ALOGI("********************************************************\n" + "* Process %d has been suspended while crashing.\n" + "* To attach gdbserver for a gdb connection on port 5039\n" + "* and start gdbclient:\n" + "*\n" + "* gdbclient %s :5039 %d\n" + "*\n" + "* Wait for gdb to start, then press the VOLUME DOWN key\n" + "* to let the process continue crashing.\n" + "********************************************************\n", + pid, name, pid); // Wait for VOLUME DOWN. if (init_getevent() == 0) { @@ -103,7 +103,7 @@ static void wait_for_user_action(pid_t pid) { uninit_getevent(); } - LOG_ERROR("debuggerd resuming process %d", pid); + ALOGI("debuggerd resuming process %d", pid); } static int get_process_info(pid_t tid, pid_t* out_pid, uid_t* out_uid, uid_t* out_gid) { @@ -139,11 +139,11 @@ static int read_request(int fd, debugger_request_t* out_request) { socklen_t len = sizeof(cr); int status = getsockopt(fd, SOL_SOCKET, SO_PEERCRED, &cr, &len); if (status != 0) { - LOG_ERROR("cannot get credentials\n"); + ALOGE("cannot get credentials\n"); return -1; } - XLOG("reading tid\n"); + ALOGV("reading tid\n"); fcntl(fd, F_SETFL, O_NONBLOCK); pollfd pollfds[1]; @@ -152,7 +152,7 @@ static int read_request(int fd, debugger_request_t* out_request) { pollfds[0].revents = 0; status = TEMP_FAILURE_RETRY(poll(pollfds, 1, 3000)); if (status != 1) { - LOG_ERROR("timed out reading tid (from pid=%d uid=%d)\n", cr.pid, cr.uid); + ALOGE("timed out reading tid (from pid=%d uid=%d)\n", cr.pid, cr.uid); return -1; } @@ -160,14 +160,13 @@ static int read_request(int fd, debugger_request_t* out_request) { memset(&msg, 0, sizeof(msg)); status = TEMP_FAILURE_RETRY(read(fd, &msg, sizeof(msg))); if (status < 0) { - LOG_ERROR("read failure? %s (pid=%d uid=%d)\n", strerror(errno), cr.pid, cr.uid); + ALOGE("read failure? %s (pid=%d uid=%d)\n", strerror(errno), cr.pid, cr.uid); return -1; } if (status == sizeof(debugger_msg_t)) { - XLOG("crash request of size %d abort_msg_address=0x%" PRIPTR "\n", - status, msg.abort_msg_address); + ALOGV("crash request of size %d abort_msg_address=%p\n", status, msg.abort_msg_address); } else { - LOG_ERROR("invalid crash request of size %d (from pid=%d uid=%d)\n", status, cr.pid, cr.uid); + ALOGE("invalid crash request of size %d (from pid=%d uid=%d)\n", status, cr.pid, cr.uid); return -1; } @@ -185,7 +184,7 @@ static int read_request(int fd, debugger_request_t* out_request) { struct stat s; snprintf(buf, sizeof buf, "/proc/%d/task/%d", out_request->pid, out_request->tid); if (stat(buf, &s)) { - LOG_ERROR("tid %d does not exist in pid %d. ignoring debug request\n", + ALOGE("tid %d does not exist in pid %d. ignoring debug request\n", out_request->tid, out_request->pid); return -1; } @@ -196,7 +195,7 @@ static int read_request(int fd, debugger_request_t* out_request) { status = get_process_info(out_request->tid, &out_request->pid, &out_request->uid, &out_request->gid); if (status < 0) { - LOG_ERROR("tid %d does not exist. ignoring explicit dump request\n", out_request->tid); + ALOGE("tid %d does not exist. ignoring explicit dump request\n", out_request->tid); return -1; } } else { @@ -217,13 +216,13 @@ static bool should_attach_gdb(debugger_request_t* request) { } static void handle_request(int fd) { - XLOG("handle_request(%d)\n", fd); + ALOGV("handle_request(%d)\n", fd); debugger_request_t request; memset(&request, 0, sizeof(request)); int status = read_request(fd, &request); if (!status) { - XLOG("BOOM: pid=%d uid=%d gid=%d tid=%d\n", + ALOGV("BOOM: pid=%d uid=%d gid=%d tid=%d\n", request.pid, request.uid, request.gid, request.tid); // At this point, the thread that made the request is blocked in @@ -237,12 +236,12 @@ static void handle_request(int fd) { // See details in bionic/libc/linker/debugger.c, in function // debugger_signal_handler(). if (ptrace(PTRACE_ATTACH, request.tid, 0, 0)) { - LOG_ERROR("ptrace attach failed: %s\n", strerror(errno)); + ALOGE("ptrace attach failed: %s\n", strerror(errno)); } else { bool detach_failed = false; bool attach_gdb = should_attach_gdb(&request); if (TEMP_FAILURE_RETRY(write(fd, "\0", 1)) != 1) { - LOG_ERROR("failed responding to client: %s\n", strerror(errno)); + ALOGE("failed responding to client: %s\n", strerror(errno)); } else { char* tombstone_path = NULL; @@ -261,20 +260,20 @@ static void handle_request(int fd) { switch (signal) { case SIGSTOP: if (request.action == DEBUGGER_ACTION_DUMP_TOMBSTONE) { - XLOG("stopped -- dumping to tombstone\n"); + ALOGV("stopped -- dumping to tombstone\n"); tombstone_path = engrave_tombstone(request.pid, request.tid, signal, request.original_si_code, - request.abort_msg_address, true, true, + request.abort_msg_address, true, &detach_failed, &total_sleep_time_usec); } else if (request.action == DEBUGGER_ACTION_DUMP_BACKTRACE) { - XLOG("stopped -- dumping to fd\n"); + ALOGV("stopped -- dumping to fd\n"); dump_backtrace(fd, -1, request.pid, request.tid, &detach_failed, &total_sleep_time_usec); } else { - XLOG("stopped -- continuing\n"); + ALOGV("stopped -- continuing\n"); status = ptrace(PTRACE_CONT, request.tid, 0, 0); if (status) { - LOG_ERROR("ptrace continue failed: %s\n", strerror(errno)); + ALOGE("ptrace continue failed: %s\n", strerror(errno)); } continue; // loop again } @@ -290,7 +289,7 @@ static void handle_request(int fd) { case SIGSTKFLT: #endif case SIGTRAP: - XLOG("stopped -- fatal signal\n"); + ALOGV("stopped -- fatal signal\n"); // Send a SIGSTOP to the process to make all of // the non-signaled threads stop moving. Without // this we get a lot of "ptrace detach failed: @@ -300,13 +299,12 @@ static void handle_request(int fd) { // makes the process less reliable, apparently... tombstone_path = engrave_tombstone(request.pid, request.tid, signal, request.original_si_code, - request.abort_msg_address, !attach_gdb, false, + request.abort_msg_address, !attach_gdb, &detach_failed, &total_sleep_time_usec); break; default: - XLOG("stopped -- unexpected signal\n"); - LOG_ERROR("process stopped due to unexpected signal %d\n", signal); + ALOGE("process stopped due to unexpected signal %d\n", signal); break; } break; @@ -322,14 +320,14 @@ static void handle_request(int fd) { free(tombstone_path); } - XLOG("detaching\n"); + ALOGV("detaching\n"); if (attach_gdb) { // stop the process so we can debug kill(request.pid, SIGSTOP); // detach so we can attach gdbserver if (ptrace(PTRACE_DETACH, request.tid, 0, 0)) { - LOG_ERROR("ptrace detach from %d failed: %s\n", request.tid, strerror(errno)); + ALOGE("ptrace detach from %d failed: %s\n", request.tid, strerror(errno)); detach_failed = true; } @@ -341,7 +339,7 @@ static void handle_request(int fd) { } else { // just detach if (ptrace(PTRACE_DETACH, request.tid, 0, 0)) { - LOG_ERROR("ptrace detach from %d failed: %s\n", request.tid, strerror(errno)); + ALOGE("ptrace detach from %d failed: %s\n", request.tid, strerror(errno)); detach_failed = true; } } @@ -353,7 +351,7 @@ static void handle_request(int fd) { // actual parent won't receive a death notification via wait(2). At this point // there's not much we can do about that. if (detach_failed) { - LOG_ERROR("debuggerd committing suicide to free the zombie!\n"); + ALOGE("debuggerd committing suicide to free the zombie!\n"); kill(getpid(), SIGKILL); } } @@ -399,16 +397,16 @@ static int do_server() { return 1; fcntl(s, F_SETFD, FD_CLOEXEC); - LOG_ERROR("debuggerd: " __DATE__ " " __TIME__ "\n"); + ALOGI("debuggerd: " __DATE__ " " __TIME__ "\n"); for (;;) { sockaddr addr; socklen_t alen = sizeof(addr); - XLOG("waiting for connection\n"); + ALOGV("waiting for connection\n"); int fd = accept(s, &addr, &alen); if (fd < 0) { - XLOG("accept failed: %s\n", strerror(errno)); + ALOGV("accept failed: %s\n", strerror(errno)); continue; } diff --git a/debuggerd/tombstone.cpp b/debuggerd/tombstone.cpp index 37839aab2..725fd54a6 100755 --- a/debuggerd/tombstone.cpp +++ b/debuggerd/tombstone.cpp @@ -395,7 +395,7 @@ static bool dump_sibling_thread_report( DIR* d = opendir(task_path); // Bail early if the task directory cannot be opened if (d == NULL) { - XLOG("Cannot open /proc/%d/task\n", pid); + ALOGE("Cannot open /proc/%d/task\n", pid); return false; } @@ -416,7 +416,7 @@ static bool dump_sibling_thread_report( // Skip this thread if cannot ptrace it if (ptrace(PTRACE_ATTACH, new_tid, 0, 0) < 0) { - LOG_ERROR("ptrace attach to %d failed: %s\n", new_tid, strerror(errno)); + _LOG(log, logtype::ERROR, "ptrace attach to %d failed: %s\n", new_tid, strerror(errno)); continue; } @@ -433,7 +433,7 @@ static bool dump_sibling_thread_report( log->current_tid = log->crashed_tid; if (ptrace(PTRACE_DETACH, new_tid, 0, 0) != 0) { - LOG_ERROR("ptrace detach from %d failed: %s\n", new_tid, strerror(errno)); + _LOG(log, logtype::ERROR, "ptrace detach from %d failed: %s\n", new_tid, strerror(errno)); detach_failed = true; } } @@ -457,7 +457,7 @@ static void dump_log_file(log_t* log, pid_t pid, const char* filename, android_name_to_log_id(filename), O_RDONLY | O_NONBLOCK, tail, pid); if (!logger_list) { - XLOG("Unable to open %s: %s\n", filename, strerror(errno)); + ALOGE("Unable to open %s: %s\n", filename, strerror(errno)); return; } @@ -474,17 +474,17 @@ static void dump_log_file(log_t* log, pid_t pid, const char* filename, // non-blocking EOF; we're done break; } else { - LOG_ERROR("Error while reading log: %s\n", + _LOG(log, logtype::ERROR, "Error while reading log: %s\n", strerror(-actual)); break; } } else if (actual == 0) { - LOG_ERROR("Got zero bytes while reading log: %s\n", + _LOG(log, logtype::ERROR, "Got zero bytes while reading log: %s\n", strerror(errno)); break; } - // NOTE: if you XLOG something here, this will spin forever, + // NOTE: if you ALOGV something here, this will spin forever, // because you will be writing as fast as you're reading. Any // high-frequency debug diagnostics should just be written to // the tombstone file. @@ -690,7 +690,7 @@ static char* find_and_open_tombstone(int* fd) { } if (oldest < 0) { - LOG_ERROR("Failed to find a valid tombstone, default to using tombstone 0.\n"); + ALOGE("Failed to find a valid tombstone, default to using tombstone 0.\n"); oldest = 0; } @@ -698,7 +698,7 @@ static char* find_and_open_tombstone(int* fd) { snprintf(path, sizeof(path), TOMBSTONE_TEMPLATE, oldest); *fd = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0600); if (*fd < 0) { - LOG_ERROR("failed to open tombstone file '%s': %s\n", path, strerror(errno)); + ALOGE("failed to open tombstone file '%s': %s\n", path, strerror(errno)); return NULL; } fchown(*fd, AID_SYSTEM, AID_SYSTEM); @@ -736,7 +736,7 @@ static int activity_manager_connect() { } char* engrave_tombstone(pid_t pid, pid_t tid, int signal, int original_si_code, - uintptr_t abort_msg_address, bool dump_sibling_threads, bool quiet, + uintptr_t abort_msg_address, bool dump_sibling_threads, bool* detach_failed, int* total_sleep_time_usec) { log_t log; @@ -744,11 +744,11 @@ char* engrave_tombstone(pid_t pid, pid_t tid, int signal, int original_si_code, log.crashed_tid = tid; if ((mkdir(TOMBSTONE_DIR, 0755) == -1) && (errno != EEXIST)) { - LOG_ERROR("failed to create %s: %s\n", TOMBSTONE_DIR, strerror(errno)); + _LOG(&log, logtype::ERROR, "failed to create %s: %s\n", TOMBSTONE_DIR, strerror(errno)); } if (chown(TOMBSTONE_DIR, AID_SYSTEM, AID_SYSTEM) == -1) { - LOG_ERROR("failed to change ownership of %s: %s\n", TOMBSTONE_DIR, strerror(errno)); + _LOG(&log, logtype::ERROR, "failed to change ownership of %s: %s\n", TOMBSTONE_DIR, strerror(errno)); } int fd = -1; @@ -756,11 +756,11 @@ char* engrave_tombstone(pid_t pid, pid_t tid, int signal, int original_si_code, if (selinux_android_restorecon(TOMBSTONE_DIR, 0) == 0) { path = find_and_open_tombstone(&fd); } else { - LOG_ERROR("Failed to restore security context, not writing tombstone.\n"); + _LOG(&log, logtype::ERROR, "Failed to restore security context, not writing tombstone.\n"); } - if (fd < 0 && quiet) { - LOG_ERROR("Skipping tombstone write, nothing to do.\n"); + if (fd < 0) { + _LOG(&log, logtype::ERROR, "Skipping tombstone write, nothing to do.\n"); *detach_failed = false; return NULL; } @@ -770,7 +770,6 @@ char* engrave_tombstone(pid_t pid, pid_t tid, int signal, int original_si_code, // being closed. int amfd = activity_manager_connect(); log.amfd = amfd; - log.quiet = quiet; *detach_failed = dump_crash(&log, pid, tid, signal, original_si_code, abort_msg_address, dump_sibling_threads, total_sleep_time_usec); diff --git a/debuggerd/tombstone.h b/debuggerd/tombstone.h index 3574e8459..7e2b2fe60 100644 --- a/debuggerd/tombstone.h +++ b/debuggerd/tombstone.h @@ -25,7 +25,7 @@ * Returns the path of the tombstone, which must be freed using free(). */ char* engrave_tombstone(pid_t pid, pid_t tid, int signal, int original_si_code, uintptr_t abort_msg_address, - bool dump_sibling_threads, bool quiet, - bool* detach_failed, int* total_sleep_time_usec); + bool dump_sibling_threads, bool* detach_failed, + int* total_sleep_time_usec); #endif // _DEBUGGERD_TOMBSTONE_H diff --git a/debuggerd/utility.cpp b/debuggerd/utility.cpp index f0d92203c..a1633448a 100644 --- a/debuggerd/utility.cpp +++ b/debuggerd/utility.cpp @@ -37,7 +37,7 @@ static int write_to_am(int fd, const char* buf, int len) { int written = TEMP_FAILURE_RETRY(write(fd, buf + len - to_write, to_write)); if (written < 0) { // hard failure - LOG_ERROR("AM write failure (%d / %s)\n", errno, strerror(errno)); + ALOGE("AM write failure (%d / %s)\n", errno, strerror(errno)); return -1; } to_write -= written; @@ -57,12 +57,10 @@ bool is_allowed_in_logcat(enum logtype ltype) { } void _LOG(log_t* log, enum logtype ltype, const char* fmt, ...) { - bool write_to_tombstone = log && log->tfd; - bool write_to_logcat = (!log || !log->quiet) && is_allowed_in_logcat(ltype); - if (log != NULL) { - write_to_logcat &= (log->crashed_tid == log->current_tid); - } - bool write_to_activitymanager = log && log->amfd >= 0 && is_allowed_in_logcat(ltype); + bool write_to_tombstone = (log->tfd != -1); + bool write_to_logcat = is_allowed_in_logcat(ltype) + && (log->crashed_tid == log->current_tid); + bool write_to_activitymanager = (log->amfd != -1); char buf[512]; va_list ap; @@ -98,25 +96,25 @@ int wait_for_signal(pid_t tid, int* total_sleep_time_usec) { if (n < 0) { if (errno == EAGAIN) continue; - LOG_ERROR("waitpid failed: %s\n", strerror(errno)); + ALOGE("waitpid failed: %s\n", strerror(errno)); return -1; } else if (n > 0) { - XLOG("waitpid: n=%d status=%08x\n", n, status); + ALOGV("waitpid: n=%d status=%08x\n", n, status); if (WIFSTOPPED(status)) { return WSTOPSIG(status); } else { - LOG_ERROR("unexpected waitpid response: n=%d, status=%08x\n", n, status); + ALOGE("unexpected waitpid response: n=%d, status=%08x\n", n, status); return -1; } } if (*total_sleep_time_usec > max_total_sleep_usec) { - LOG_ERROR("timed out waiting for tid=%d to die\n", tid); + ALOGE("timed out waiting for tid=%d to die\n", tid); return -1; } // not ready yet - XLOG("not ready yet\n"); + ALOGV("not ready yet\n"); usleep(sleep_time_usec); *total_sleep_time_usec += sleep_time_usec; } @@ -126,7 +124,7 @@ void wait_for_stop(pid_t tid, int* total_sleep_time_usec) { siginfo_t si; while (TEMP_FAILURE_RETRY(ptrace(PTRACE_GETSIGINFO, tid, 0, &si)) < 0 && errno == ESRCH) { if (*total_sleep_time_usec > max_total_sleep_usec) { - LOG_ERROR("timed out waiting for tid=%d to stop\n", tid); + ALOGE("timed out waiting for tid=%d to stop\n", tid); break; } diff --git a/debuggerd/utility.h b/debuggerd/utility.h index fdfb2ec65..ee4f03536 100644 --- a/debuggerd/utility.h +++ b/debuggerd/utility.h @@ -26,8 +26,6 @@ typedef struct { int tfd; /* Activity Manager socket file descriptor */ int amfd; - /* if true, does not log anything to the Android logcat or Activity Manager */ - bool quiet; // The tid of the thread that crashed. pid_t crashed_tid; // The tid of the thread we are currently working with. @@ -51,10 +49,6 @@ enum logtype { void _LOG(log_t* log, logtype ltype, const char *fmt, ...) __attribute__ ((format(printf, 3, 4))); -/* Further helpful macros */ -#define LOG_ERROR(fmt...) _LOG(NULL, logtype::ERROR, fmt) -#define XLOG(fmt...) do {} while(0) - int wait_for_signal(pid_t tid, int* total_sleep_time_usec); void wait_for_stop(pid_t tid, int* total_sleep_time_usec);