From 2b1bdd1079b035c1023db310f31fe52793c8cd9c Mon Sep 17 00:00:00 2001 From: Pirama Arumuga Nainar Date: Mon, 3 Jun 2019 10:56:35 -0700 Subject: Do init_profile_extras constructor tasks only once Bug: http://b/116873221 Currently, every coverage-enabled binary (the main executable or shared library) in the process executes the init_profile_extras constructor leading to redundant initialization. Instead, add a weak global value that gets set once so following constructors don't set a signal handler or create a pthread. It's ok to do this before the constructor succeeds because if pthread creation or signal handler registration failed once, they are unlikely to succeed the second time. This is thread-safe since constructor initialization is single-threaded. The one possibility for a race - the process doesn't have any coverage-enabled library, and multiple threads dlopen() coverage-enabled shared libraries - seems quite unlikely. Test: Verify that only one extra thread is created for a daemon (adbd). Change-Id: Ie969311c2ddac147497c870b5c365b4617b3c60a Merged-In: Ie969311c2ddac147497c870b5c365b4617b3c60a (cherry picked from commit e76c23dc6b7bff0c7404744b9bdde0d02465acdf) --- toolchain-extras/profile-extras.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/toolchain-extras/profile-extras.cpp b/toolchain-extras/profile-extras.cpp index 2d685084..f999a6b2 100644 --- a/toolchain-extras/profile-extras.cpp +++ b/toolchain-extras/profile-extras.cpp @@ -67,6 +67,8 @@ void *property_watch_loop(__unused void *arg) { } } +__attribute__((weak)) int init_profile_extras_once = 0; + // Initialize libprofile-extras: // - Install a signal handler that triggers __gcov_flush on . // - Create a thread that calls __gcov_flush when sysprop @@ -81,6 +83,10 @@ void *property_watch_loop(__unused void *arg) { // We force the linker to include init_profile_extras() by passing // '-uinit_profile_extras' to the linker (in build/soong). __attribute__((constructor)) int init_profile_extras(void) { + if (init_profile_extras_once) + return 0; + init_profile_extras_once = 1; + sighandler_t ret1 = signal(GCOV_FLUSH_SIGNAL, gcov_signal_handler); if (ret1 == SIG_ERR) { return -1; -- cgit v1.2.3 From 6d9db09a456950c168b10a8cddd6f2198206c710 Mon Sep 17 00:00:00 2001 From: Pirama Arumuga Nainar Date: Tue, 4 Jun 2019 15:33:45 -0700 Subject: Refactor property-watch optout logic Bug: http://b/116873221 Refactor the processes without property-watch thread into a list. Test: Build cuttlefish with coverage Change-Id: Id8f0ceab133b961219b1cf77ae02b00eba4cfaf5 Merged-In: Id8f0ceab133b961219b1cf77ae02b00eba4cfaf5 (cherry picked from commit 5b9defd41d61b000c82c84662f02394e89192d93) --- toolchain-extras/profile-extras.cpp | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/toolchain-extras/profile-extras.cpp b/toolchain-extras/profile-extras.cpp index f999a6b2..cdb19e8f 100644 --- a/toolchain-extras/profile-extras.cpp +++ b/toolchain-extras/profile-extras.cpp @@ -67,6 +67,18 @@ void *property_watch_loop(__unused void *arg) { } } +#if defined(__ANDROID_API__) && __ANDROID_API__ >= __ANDROID_API_L__ +static char prop_watch_disabled_procs[][128] = { + "zygote", + "zygote32", + "app_process", + "app_process32", +}; + +static size_t prop_watch_num_disabled_procs = \ + sizeof(prop_watch_disabled_procs) / sizeof(prop_watch_disabled_procs[0]); +#endif + __attribute__((weak)) int init_profile_extras_once = 0; // Initialize libprofile-extras: @@ -98,11 +110,10 @@ __attribute__((constructor)) int init_profile_extras(void) { // getprogname() was added. #if defined(__ANDROID_API__) && __ANDROID_API__ >= __ANDROID_API_L__ const char *prog_basename = basename(getprogname()); - if (strncmp(prog_basename, "zygote", strlen("zygote")) == 0) { - return 0; - } - if (strncmp(prog_basename, "app_process", strlen("app_process")) == 0) { - return 0; + for (size_t i = 0; i < prop_watch_num_disabled_procs; i ++) { + if (strcmp(prog_basename, prop_watch_disabled_procs[i]) == 0) { + return 0; + } } #endif -- cgit v1.2.3 From 6dc0cc21fea69df79f50daf62fb1376fe1db0652 Mon Sep 17 00:00:00 2001 From: Pirama Arumuga Nainar Date: Mon, 1 Jul 2019 22:13:10 -0700 Subject: Wrap getenv and append euid to GCOV_PREFIX The coverage runtime creates coverage files with permissions 0644 and intermediate directories with 0755. This causes conflicts and permissions issues when multiple processes create/access the same file or directory under $GCOV_PREFIX. Some processes also call umask, which further complicates things. It is possible to handle all of these in the coverage runtime library but a foolproof alternative is to have a separate coverage directory per effective userid (euid) by customizing GCOV_PREFIX. This change adds a wrapper to getenv which appends the euid of the current process for the "GCOV_PREFIX" environment variable. Bug: 148178774 Test: Verify that coverage files are written to /data/misc/trace//proc/... instead of /data/misc/trace/proc/... Change-Id: I6be1e748618d84697c354516ab1c734fb33ab5f4 Merged-In: I6be1e748618d84697c354516ab1c734fb33ab5f4 (cherry picked from commit a277ce976288ac0a35a9016970f6e49037eaeadb) --- toolchain-extras/Android.bp | 5 +++++ toolchain-extras/profile-globals.c | 42 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 toolchain-extras/profile-globals.c diff --git a/toolchain-extras/Android.bp b/toolchain-extras/Android.bp index 727f9795..5c839a1e 100644 --- a/toolchain-extras/Android.bp +++ b/toolchain-extras/Android.bp @@ -2,6 +2,7 @@ cc_defaults { name: "libprofile-defaults", srcs: [ "profile-extras.cpp", + "profile-globals.c", ], native_coverage: false, } @@ -24,6 +25,10 @@ cc_library_static { cc_library_static { name: "libprofile-extras_ndk", defaults: ["libprofile-defaults",], + vendor_available: true, + vndk: { + enabled: true, + }, sdk_version: "minimum", } diff --git a/toolchain-extras/profile-globals.c b/toolchain-extras/profile-globals.c new file mode 100644 index 00000000..95bd46dd --- /dev/null +++ b/toolchain-extras/profile-globals.c @@ -0,0 +1,42 @@ +/* + * Copyright (C) 2019 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 + +// This file provides a wrapper for getenv that appends the userid (geteuid()) +// of the current process to GCOV_PREFIX. This avoids conflicts and permissions +// issues when different processes try to create/access the same directories and +// files under $GCOV_PREFIX. +// +// When this file is linked to a binary, the -Wl,--wrap,getenv flag must be +// used. The linker redirects calls to getenv to __wrap_getenv and sets +// __real_getenv to point to libc's getenv. + +char *__real_getenv(const char *name); + +static char modified_gcov_prefix[128]; + +__attribute__((weak)) char *__wrap_getenv(const char *name) { + if (strcmp(name, "GCOV_PREFIX") != 0) { + return __real_getenv(name); + } + + sprintf(modified_gcov_prefix, "%s/%u", __real_getenv(name), geteuid()); + return modified_gcov_prefix; +} -- cgit v1.2.3 From 0fba97d295347637e8f44cc4691a027683f5c031 Mon Sep 17 00:00:00 2001 From: Pirama Arumuga Nainar Date: Wed, 5 Jun 2019 16:39:04 -0700 Subject: Opt adbd and init out of property-watcher thread Bug: http://b/116873221 Bug: http://b/134177005 adbd calls selinux_android_setcon() which will fail if there are multiple threads in the process. The reason for boot failures with coverage-enabled init and property-watcher thread is not clear yet. Test: cuttlefish with coverage for init and adbd boots. Change-Id: I0c44e76cc59d12b963e4d5b862683ead2f816cdc Merged-In: I0c44e76cc59d12b963e4d5b862683ead2f816cdc (cherry picked from commit 87d1a3bfba5bba715eed9d1b254bfa02282bf39f) --- toolchain-extras/profile-extras.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/toolchain-extras/profile-extras.cpp b/toolchain-extras/profile-extras.cpp index cdb19e8f..b7a9318a 100644 --- a/toolchain-extras/profile-extras.cpp +++ b/toolchain-extras/profile-extras.cpp @@ -73,6 +73,8 @@ static char prop_watch_disabled_procs[][128] = { "zygote32", "app_process", "app_process32", + "adbd", + "init", }; static size_t prop_watch_num_disabled_procs = \ -- cgit v1.2.3 From 466d7164cf077377a82c2e598613373ed9cf590b Mon Sep 17 00:00:00 2001 From: Pirama Arumuga Nainar Date: Fri, 26 Jul 2019 14:49:58 -0700 Subject: Create per-uid GCOV_PREFIX dir in the getenv wrapper Bug: http://b/116873221 With https://reviews.llvm.org/D65245, the profile runtime assumes that the directory ${GCOV_PREFIX} is already created. Before returning from the getenv wrapper, create the per-uid ${GCOV_PREFIX}. Test: cuttlefish builds and boots with new Clang. Change-Id: I276e1a92b3cb18ee85bb4451159b131d8ebca4a2 Merged-In: I276e1a92b3cb18ee85bb4451159b131d8ebca4a2 (cherry picked from commit b00f2fc6bb862c1b41c33984830e93149efa339d) --- toolchain-extras/profile-globals.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/toolchain-extras/profile-globals.c b/toolchain-extras/profile-globals.c index 95bd46dd..309f0606 100644 --- a/toolchain-extras/profile-globals.c +++ b/toolchain-extras/profile-globals.c @@ -17,6 +17,7 @@ #include #include #include +#include #include // This file provides a wrapper for getenv that appends the userid (geteuid()) @@ -38,5 +39,6 @@ __attribute__((weak)) char *__wrap_getenv(const char *name) { } sprintf(modified_gcov_prefix, "%s/%u", __real_getenv(name), geteuid()); + mkdir(modified_gcov_prefix, 0777); return modified_gcov_prefix; } -- cgit v1.2.3 From 572460c417e6e0fd9337e5ef6507335d6d40d42b Mon Sep 17 00:00:00 2001 From: Ray Essick Date: Thu, 9 Jan 2020 11:51:27 -0800 Subject: Dump coverage for all objects (a.out + dlopen()'d .so's) with code coverage enabled, the main object as well as each dlopen()ed shared library include their own copies of the hooks for dumping gcov data upon receiving signal GCOV_FLUSH_SIGNAL. Only the last one to register was actually dumped. Here, we chain the signal handlers together so that each one will call the previously registered handler to ensure that each object's coverage data is dumped. Bug: 139313557 Test: dump gcov data via kill -37 on module with dlopen() Change-Id: I376df2df6b65acff95ad5d135a7ba2bf0eac3695 Merged-In: I376df2df6b65acff95ad5d135a7ba2bf0eac3695 (cherry picked from commit b5d273da8824e8dd4bf9c28c50797364b0b47a05) --- toolchain-extras/profile-extras.cpp | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/toolchain-extras/profile-extras.cpp b/toolchain-extras/profile-extras.cpp index b7a9318a..7d78a0a5 100644 --- a/toolchain-extras/profile-extras.cpp +++ b/toolchain-extras/profile-extras.cpp @@ -31,8 +31,16 @@ extern "C" { void __gcov_flush(void); -static void gcov_signal_handler(__unused int signum) { +// storing SIG_ERR helps us detect (unlikely) looping. +static sighandler_t chained_gcov_signal_handler = SIG_ERR; + +static void gcov_signal_handler(int signum) { __gcov_flush(); + if (chained_gcov_signal_handler != SIG_ERR && + chained_gcov_signal_handler != SIG_IGN && + chained_gcov_signal_handler != SIG_DFL) { + (chained_gcov_signal_handler)(signum); + } } static const char kCoveragePropName[] = "debug.coverage.flush"; @@ -101,10 +109,15 @@ __attribute__((constructor)) int init_profile_extras(void) { return 0; init_profile_extras_once = 1; + // is this instance already registered? + if (chained_gcov_signal_handler != SIG_ERR) { + return -1; + } sighandler_t ret1 = signal(GCOV_FLUSH_SIGNAL, gcov_signal_handler); if (ret1 == SIG_ERR) { return -1; } + chained_gcov_signal_handler = ret1; // Do not create thread running property_watch_loop for zygote (it can get // invoked as zygote or app_process). This check is only needed for the -- cgit v1.2.3