From 9e87f2f876243225deef37645ddceaa5d225cb41 Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Wed, 17 Sep 2014 14:59:24 -0700 Subject: [PATCH 1/4] Revert "Revert "Added a bionic systrace class and tracing to pthread_mutex.cpp."" This reverts commit 26c1420fbb68916d66a8621b5efe8bb25cfdad7b. --- libc/Android.mk | 1 + libc/bionic/bionic_systrace.cpp | 107 ++++++++++++++++++++++++++++++++ libc/bionic/pthread_mutex.cpp | 12 ++++ libc/private/bionic_systrace.h | 35 +++++++++++ 4 files changed, 155 insertions(+) create mode 100644 libc/bionic/bionic_systrace.cpp create mode 100644 libc/private/bionic_systrace.h diff --git a/libc/Android.mk b/libc/Android.mk index 5052cb10d..5f5add22b 100644 --- a/libc/Android.mk +++ b/libc/Android.mk @@ -89,6 +89,7 @@ libc_bionic_src_files := \ bionic/access.cpp \ bionic/assert.cpp \ bionic/atof.cpp \ + bionic/bionic_systrace.cpp \ bionic/bionic_time_conversions.cpp \ bionic/brk.cpp \ bionic/c16rtomb.cpp \ diff --git a/libc/bionic/bionic_systrace.cpp b/libc/bionic/bionic_systrace.cpp new file mode 100644 index 000000000..b8e892e72 --- /dev/null +++ b/libc/bionic/bionic_systrace.cpp @@ -0,0 +1,107 @@ +/* + * Copyright (C) 2014 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. + */ + +#include +#include +#include +#include + +#include "private/bionic_systrace.h" +#include "private/libc_logging.h" + +#define _REALLY_INCLUDE_SYS__SYSTEM_PROPERTIES_H_ +#include + +#define WRITE_OFFSET 32 + +static const prop_info* g_pinfo = NULL; +static uint32_t g_serial = -1; +static uint64_t g_tags = 0; +static int g_trace_marker_fd = -1; + +static bool should_trace() { + // If g_pinfo is null, this means that systrace hasn't been run and it's safe to + // assume that no trace writing will need to take place. However, to avoid running + // this costly find check each time, we set it to a non-tracing value so that next + // time, it will just check the serial to see if the value has been changed. + // this function also deals with the bootup case, during which the call to property + // set will fail if the property server hasn't yet started. + if (g_pinfo == NULL) { + g_pinfo = __system_property_find("debug.atrace.tags.enableflags"); + if (g_pinfo == NULL) { + __system_property_set("debug.atrace.tags.enableflags", "0"); + g_pinfo = __system_property_find("debug.atrace.tags.enableflags"); + if (g_pinfo == NULL) { + return false; + } + } + } + + // Find out which tags have been enabled on the command line and set + // the value of tags accordingly. If the value of the property changes, + // the serial will also change, so the costly system_property_read function + // can be avoided by calling the much cheaper system_property_serial + // first. The values within pinfo may change, but its location is guaranteed + // not to move. + const uint32_t cur_serial = __system_property_serial(g_pinfo); + if (cur_serial != g_serial) { + g_serial = cur_serial; + char value[PROP_VALUE_MAX]; + __system_property_read(g_pinfo, 0, value); + g_tags = strtoull(value, NULL, 0); + } + + // Finally, verify that this tag value enables bionic tracing. + return ((g_tags & ATRACE_TAG_BIONIC) != 0); +} + +ScopedTrace::ScopedTrace(const char* message) { + if (!should_trace()) { + return; + } + + if (g_trace_marker_fd == -1) { + g_trace_marker_fd = open("/sys/kernel/debug/tracing/trace_marker", O_WRONLY | O_CLOEXEC); + if (g_trace_marker_fd == -1) { + __libc_fatal("Could not open kernel trace file: %s\n", strerror(errno)); + } + } + + // If bionic tracing has been enabled, then write the message to the + // kernel trace_marker. + int length = strlen(message); + char buf[length + WRITE_OFFSET]; + size_t len = snprintf(buf, length + WRITE_OFFSET, "B|%d|%s", getpid(), message); + ssize_t wbytes = TEMP_FAILURE_RETRY(write(g_trace_marker_fd, buf, len)); + + // Error while writing + if (static_cast(wbytes) != len) { + __libc_fatal("Could not write to kernel trace file: %s\n", strerror(errno)); + } +} + +ScopedTrace::~ScopedTrace() { + if (!should_trace()) { + return; + } + + ssize_t wbytes = TEMP_FAILURE_RETRY(write(g_trace_marker_fd, "E", 1)); + + // Error while writing + if (static_cast(wbytes) != 1) { + __libc_fatal("Could not write to kernel trace file: %s\n", strerror(errno)); + } +} diff --git a/libc/bionic/pthread_mutex.cpp b/libc/bionic/pthread_mutex.cpp index 546166166..e00ffb437 100644 --- a/libc/bionic/pthread_mutex.cpp +++ b/libc/bionic/pthread_mutex.cpp @@ -39,6 +39,8 @@ #include "private/bionic_futex.h" #include "private/bionic_tls.h" +#include "private/bionic_systrace.h" + extern void pthread_debug_mutex_lock_check(pthread_mutex_t *mutex); extern void pthread_debug_mutex_unlock_check(pthread_mutex_t *mutex); @@ -333,6 +335,10 @@ static inline void _normal_lock(pthread_mutex_t* mutex, int shared) { * that the mutex is in state 2 when we go to sleep on it, which * guarantees a wake-up call. */ + + ScopedTrace trace("Contending for pthread mutex"); + + while (__bionic_swap(locked_contended, &mutex->value) != unlocked) { __futex_wait_ex(&mutex->value, shared, locked_contended, NULL); } @@ -473,6 +479,8 @@ int pthread_mutex_lock(pthread_mutex_t* mutex) { mvalue = mutex->value; } + ScopedTrace trace("Contending for pthread mutex"); + for (;;) { int newval; @@ -626,6 +634,8 @@ static int __pthread_mutex_timedlock(pthread_mutex_t* mutex, const timespec* abs return 0; } + ScopedTrace trace("Contending for timed pthread mutex"); + // Loop while needed. while (__bionic_swap(locked_contended, &mutex->value) != unlocked) { if (__timespec_from_absolute(&ts, abs_timeout, clock) < 0) { @@ -658,6 +668,8 @@ static int __pthread_mutex_timedlock(pthread_mutex_t* mutex, const timespec* abs mvalue = mutex->value; } + ScopedTrace trace("Contending for timed pthread mutex"); + while (true) { // If the value is 'unlocked', try to acquire it directly. // NOTE: put state to 2 since we know there is contention. diff --git a/libc/private/bionic_systrace.h b/libc/private/bionic_systrace.h new file mode 100644 index 000000000..ad9ff7f71 --- /dev/null +++ b/libc/private/bionic_systrace.h @@ -0,0 +1,35 @@ +/* + * Copyright (C) 2014 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 BIONIC_SYSTRACE_H +#define BIONIC_SYSTRACE_H + +#include "bionic_macros.h" + +// Tracing class for bionic. To begin a trace at a specified point: +// ScopedTrace("Trace message"); +// The trace will end when the contructor goes out of scope. + +class ScopedTrace { + public: + explicit ScopedTrace(const char* message); + ~ScopedTrace(); + + private: + DISALLOW_COPY_AND_ASSIGN(ScopedTrace); +}; + +#endif From ebb6b4a4d3fab87800b912c9d6ea917f5359f8af Mon Sep 17 00:00:00 2001 From: Dan Albert Date: Wed, 13 Aug 2014 11:25:01 -0700 Subject: [PATCH 2/4] Hide ScopedTrace. (cherry-pick of f2c1e7ee78a167ff323b9f45d20532d064d6778d.) Bug: 11156955 Change-Id: I6cddc868d1c6503e30f1ffcf460f45670631d64a --- libc/private/bionic_systrace.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libc/private/bionic_systrace.h b/libc/private/bionic_systrace.h index ad9ff7f71..0b4560f92 100644 --- a/libc/private/bionic_systrace.h +++ b/libc/private/bionic_systrace.h @@ -23,7 +23,7 @@ // ScopedTrace("Trace message"); // The trace will end when the contructor goes out of scope. -class ScopedTrace { +class __LIBC_HIDDEN__ ScopedTrace { public: explicit ScopedTrace(const char* message); ~ScopedTrace(); From 21ce3f506f3b54e4f57a37847375cef9f8aff57f Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Tue, 26 Aug 2014 16:20:59 -0700 Subject: [PATCH 3/4] More cases where libc should use O_CLOEXEC. Change-Id: Idfa111aeebc5deca2399dae919e8b72eb54c23c0 (cherry picked from commit f73183f1a34df22b62a3d0bbf82e18d5797c9cde) --- libc/bionic/bionic_systrace.cpp | 2 +- libc/bionic/dirent.cpp | 2 +- libc/bionic/malloc_debug_qemu.cpp | 2 +- libc/bionic/pthread_setname_np.cpp | 2 +- libc/bionic/system_properties.cpp | 21 ++------------------- 5 files changed, 6 insertions(+), 23 deletions(-) diff --git a/libc/bionic/bionic_systrace.cpp b/libc/bionic/bionic_systrace.cpp index b8e892e72..f5be41553 100644 --- a/libc/bionic/bionic_systrace.cpp +++ b/libc/bionic/bionic_systrace.cpp @@ -74,7 +74,7 @@ ScopedTrace::ScopedTrace(const char* message) { } if (g_trace_marker_fd == -1) { - g_trace_marker_fd = open("/sys/kernel/debug/tracing/trace_marker", O_WRONLY | O_CLOEXEC); + g_trace_marker_fd = open("/sys/kernel/debug/tracing/trace_marker", O_CLOEXEC | O_WRONLY); if (g_trace_marker_fd == -1) { __libc_fatal("Could not open kernel trace file: %s\n", strerror(errno)); } diff --git a/libc/bionic/dirent.cpp b/libc/bionic/dirent.cpp index 7abc7f3ec..5e1c7a565 100644 --- a/libc/bionic/dirent.cpp +++ b/libc/bionic/dirent.cpp @@ -78,7 +78,7 @@ DIR* fdopendir(int fd) { } DIR* opendir(const char* path) { - int fd = open(path, O_RDONLY | O_DIRECTORY); + int fd = open(path, O_CLOEXEC | O_DIRECTORY | O_RDONLY); return (fd != -1) ? __allocate_DIR(fd) : NULL; } diff --git a/libc/bionic/malloc_debug_qemu.cpp b/libc/bionic/malloc_debug_qemu.cpp index b3b604d86..2f4949b12 100644 --- a/libc/bionic/malloc_debug_qemu.cpp +++ b/libc/bionic/malloc_debug_qemu.cpp @@ -606,7 +606,7 @@ extern "C" bool malloc_debug_initialize(HashTable*, const MallocDebug* malloc_di * the memory mapped spaces into writes to an I/O port that emulator * "listens to" on the other end. Note that until we open and map that * device, logging to emulator's stdout will not be available. */ - int fd = open("/dev/qemu_trace", O_RDWR); + int fd = open("/dev/qemu_trace", O_CLOEXEC | O_RDWR); if (fd < 0) { error_log("Unable to open /dev/qemu_trace"); return false; diff --git a/libc/bionic/pthread_setname_np.cpp b/libc/bionic/pthread_setname_np.cpp index 1ddf81044..7b2fa6b0a 100644 --- a/libc/bionic/pthread_setname_np.cpp +++ b/libc/bionic/pthread_setname_np.cpp @@ -67,7 +67,7 @@ int pthread_setname_np(pthread_t t, const char* thread_name) { } char comm_name[sizeof(TASK_COMM_FMT) + 8]; snprintf(comm_name, sizeof(comm_name), TASK_COMM_FMT, tid); - int fd = open(comm_name, O_WRONLY); + int fd = open(comm_name, O_CLOEXEC | O_WRONLY); if (fd == -1) { return errno; } diff --git a/libc/bionic/system_properties.cpp b/libc/bionic/system_properties.cpp index ad69cf5f9..411d6bf34 100644 --- a/libc/bionic/system_properties.cpp +++ b/libc/bionic/system_properties.cpp @@ -201,14 +201,6 @@ static int map_prop_area_rw() return -1; } - // TODO: Is this really required ? Does android run on any kernels that - // don't support O_CLOEXEC ? - const int ret = fcntl(fd, F_SETFD, FD_CLOEXEC); - if (ret < 0) { - close(fd); - return -1; - } - if (ftruncate(fd, PA_SIZE) < 0) { close(fd); return -1; @@ -271,18 +263,9 @@ static int map_fd_ro(const int fd) { static int map_prop_area() { - int fd(open(property_filename, O_RDONLY | O_NOFOLLOW | O_CLOEXEC)); - if (fd >= 0) { - /* For old kernels that don't support O_CLOEXEC */ - const int ret = fcntl(fd, F_SETFD, FD_CLOEXEC); - if (ret < 0) { - close(fd); - return -1; - } - } - + int fd = open(property_filename, O_CLOEXEC | O_NOFOLLOW | O_RDONLY); bool close_fd = true; - if ((fd < 0) && (errno == ENOENT)) { + if (fd == -1 && errno == ENOENT) { /* * For backwards compatibility, if the file doesn't * exist, we use the environment to get the file descriptor. From 8fa377eb6afdea4b03b6a0d112471f7ee988fb96 Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Wed, 17 Sep 2014 15:25:35 -0700 Subject: [PATCH 4/4] Fix an unintended difference between aosp/master and lmp-dev-plus-aosp. Whitespace difference presumably introduced while fixing a merge conflict. Bug: 17549448 Change-Id: If688330bf1dbaed23ba687d843d9744d6261023b --- libc/Android.mk | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/libc/Android.mk b/libc/Android.mk index 5f5add22b..56e98768b 100644 --- a/libc/Android.mk +++ b/libc/Android.mk @@ -865,8 +865,7 @@ LOCAL_CONLYFLAGS := $(libc_common_conlyflags) LOCAL_CPPFLAGS := $(libc_common_cppflags) LOCAL_C_INCLUDES := $(libc_common_c_includes) LOCAL_MODULE := libc_cxa -# GCC refuses to hide new/delete -LOCAL_CLANG := true +LOCAL_CLANG := true # GCC refuses to hide new/delete LOCAL_ADDITIONAL_DEPENDENCIES := $(libc_common_additional_dependencies) LOCAL_SYSTEM_SHARED_LIBRARIES :=