From fb3b85a81a022f6fb8220e2766b9dd56b8e76cbb Mon Sep 17 00:00:00 2001 From: xtao Date: Tue, 5 Feb 2019 13:33:06 +0800 Subject: [PATCH] 1) Add MACRO GPR_HAS_FEATURE; 2) Add test code within GRPC_ASAN_ENABLED for gpr_mu/cv mem-leak detection. --- include/grpc/impl/codegen/port_platform.h | 17 ++++- include/grpc/impl/codegen/sync_posix.h | 18 ++++++ src/core/lib/gpr/sync_posix.cc | 79 +++++++++++++++++++++-- 3 files changed, 108 insertions(+), 6 deletions(-) diff --git a/include/grpc/impl/codegen/port_platform.h b/include/grpc/impl/codegen/port_platform.h index 0da45acab57..af114de4153 100644 --- a/include/grpc/impl/codegen/port_platform.h +++ b/include/grpc/impl/codegen/port_platform.h @@ -534,6 +534,14 @@ typedef unsigned __int64 uint64_t; #endif #endif /* GPR_HAS_ATTRIBUTE */ +#ifndef GPR_HAS_FEATURE +#ifdef __has_feature +#define GPR_HAS_FEATURE(a) __has_feature(a) +#else +#define GPR_HAS_FEATURE(a) 0 +#endif +#endif /* GPR_HAS_FEATURE */ + #ifndef GPR_ATTRIBUTE_NOINLINE #if GPR_HAS_ATTRIBUTE(noinline) || (defined(__GNUC__) && !defined(__clang__)) #define GPR_ATTRIBUTE_NOINLINE __attribute__((noinline)) @@ -569,10 +577,15 @@ typedef unsigned __int64 uint64_t; /* GRPC_TSAN_ENABLED will be defined, when compiled with thread sanitizer. */ #if defined(__SANITIZE_THREAD__) #define GRPC_TSAN_ENABLED -#elif defined(__has_feature) -#if __has_feature(thread_sanitizer) +#elif GPR_HAS_FEATURE(thread_sanitizer) #define GRPC_TSAN_ENABLED #endif + +/* GRPC_ASAN_ENABLED will be defined, when compiled with address sanitizer. */ +#if defined(__SANITIZE_ADDRESS__) +#define GRPC_ASAN_ENABLED +#elif GPR_HAS_FEATURE(address_sanitizer) +#define GRPC_ASAN_ENABLED #endif /* GRPC_ALLOW_EXCEPTIONS should be 0 or 1 if exceptions are allowed or not */ diff --git a/include/grpc/impl/codegen/sync_posix.h b/include/grpc/impl/codegen/sync_posix.h index d927046c53e..cf95e4c6345 100644 --- a/include/grpc/impl/codegen/sync_posix.h +++ b/include/grpc/impl/codegen/sync_posix.h @@ -25,8 +25,26 @@ #include +#ifdef GRPC_ASAN_ENABLED +/* The member |leak_checker| is used to check whether there is memory leak + * that may be caused by upper layer logic which missing the |gpr_xx_destroy| + * call to this object before freeing. + * This issue was reported at https://github.com/grpc/grpc/issues/17563 + * and discussed at https://github.com/grpc/grpc/pull/17586 + */ +typedef struct { + pthread_mutex_t mutex; + int *leak_checker; +} gpr_mu; + +typedef struct { + pthread_cond_t cond_var; + int *leak_checker; +} gpr_cv; +#else typedef pthread_mutex_t gpr_mu; typedef pthread_cond_t gpr_cv; +#endif typedef pthread_once_t gpr_once; #define GPR_ONCE_INIT PTHREAD_ONCE_INIT diff --git a/src/core/lib/gpr/sync_posix.cc b/src/core/lib/gpr/sync_posix.cc index c09a7598acb..a915bbfad95 100644 --- a/src/core/lib/gpr/sync_posix.cc +++ b/src/core/lib/gpr/sync_posix.cc @@ -17,6 +17,7 @@ */ #include +#include #ifdef GPR_POSIX_SYNC @@ -72,27 +73,58 @@ gpr_atm gpr_counter_atm_add = 0; #endif void gpr_mu_init(gpr_mu* mu) { +#ifdef GRPC_ASAN_ENABLED + GPR_ASSERT(pthread_mutex_init(&mu->mutex, nullptr) == 0); + mu->leak_checker = (int*)gpr_malloc(sizeof(*mu->leak_checker)); + GPR_ASSERT(mu->leak_checker != nullptr); + /* Initial it with a magic number, make no sense, just use the memory. + * This only take effect when ASAN enabled, so, + * if memory allocation failed, let it crash. + */ + *mu->leak_checker = 0x12F34D0; +#else GPR_ASSERT(pthread_mutex_init(mu, nullptr) == 0); +#endif } -void gpr_mu_destroy(gpr_mu* mu) { GPR_ASSERT(pthread_mutex_destroy(mu) == 0); } +void gpr_mu_destroy(gpr_mu* mu) { +#ifdef GRPC_ASAN_ENABLED + GPR_ASSERT(pthread_mutex_destroy(&mu->mutex) == 0); + gpr_free(mu->leak_checker); +#else + GPR_ASSERT(pthread_mutex_destroy(mu) == 0); +#endif +} void gpr_mu_lock(gpr_mu* mu) { #ifdef GPR_LOW_LEVEL_COUNTERS GPR_ATM_INC_COUNTER(gpr_mu_locks); #endif GPR_TIMER_SCOPE("gpr_mu_lock", 0); +#ifdef GRPC_ASAN_ENABLED + GPR_ASSERT(pthread_mutex_lock(&mu->mutex) == 0); +#else GPR_ASSERT(pthread_mutex_lock(mu) == 0); +#endif } void gpr_mu_unlock(gpr_mu* mu) { GPR_TIMER_SCOPE("gpr_mu_unlock", 0); +#ifdef GRPC_ASAN_ENABLED + GPR_ASSERT(pthread_mutex_unlock(&mu->mutex) == 0); +#else GPR_ASSERT(pthread_mutex_unlock(mu) == 0); +#endif } int gpr_mu_trylock(gpr_mu* mu) { GPR_TIMER_SCOPE("gpr_mu_trylock", 0); - int err = pthread_mutex_trylock(mu); + int err = 0; +#ifdef GRPC_ASAN_ENABLED + err = pthread_mutex_trylock(&mu->mutex); +#else + err = pthread_mutex_trylock(mu); +#endif GPR_ASSERT(err == 0 || err == EBUSY); return err == 0; } @@ -105,10 +137,29 @@ void gpr_cv_init(gpr_cv* cv) { #if GPR_LINUX GPR_ASSERT(pthread_condattr_setclock(&attr, CLOCK_MONOTONIC) == 0); #endif // GPR_LINUX + +#ifdef GRPC_ASAN_ENABLED + GPR_ASSERT(pthread_cond_init(&cv->cond_var, &attr) == 0); + cv->leak_checker = (int*)gpr_malloc(sizeof(*cv->leak_checker)); + GPR_ASSERT(cv->leak_checker != nullptr); + /* Initial it with a magic number, make no sense, just use the memory. + * This only take effect when ASAN enabled, so, + * if memory allocation failed, let it crash. + */ + *cv->leak_checker = 0x12F34D0; +#else GPR_ASSERT(pthread_cond_init(cv, &attr) == 0); +#endif } -void gpr_cv_destroy(gpr_cv* cv) { GPR_ASSERT(pthread_cond_destroy(cv) == 0); } +void gpr_cv_destroy(gpr_cv* cv) { +#ifdef GRPC_ASAN_ENABLED + GPR_ASSERT(pthread_cond_destroy(&cv->cond_var) == 0); + gpr_free(cv->leak_checker); +#else + GPR_ASSERT(pthread_cond_destroy(cv) == 0); +#endif +} // For debug of the timer manager crash only. // TODO (mxyan): remove after bug is fixed. @@ -169,7 +220,11 @@ int gpr_cv_wait(gpr_cv* cv, gpr_mu* mu, gpr_timespec abs_deadline) { #endif if (gpr_time_cmp(abs_deadline, gpr_inf_future(abs_deadline.clock_type)) == 0) { +#ifdef GRPC_ASAN_ENABLED + err = pthread_cond_wait(&cv->cond_var, &mu->mutex); +#else err = pthread_cond_wait(cv, mu); +#endif } else { struct timespec abs_deadline_ts; #if GPR_LINUX @@ -181,7 +236,13 @@ int gpr_cv_wait(gpr_cv* cv, gpr_mu* mu, gpr_timespec abs_deadline) { #endif // GPR_LINUX abs_deadline_ts.tv_sec = static_cast(abs_deadline.tv_sec); abs_deadline_ts.tv_nsec = abs_deadline.tv_nsec; + +#ifdef GRPC_ASAN_ENABLED + err = pthread_cond_timedwait(&cv->cond_var, &mu->mutex, &abs_deadline_ts); +#else err = pthread_cond_timedwait(cv, mu, &abs_deadline_ts); +#endif + #ifdef GRPC_DEBUG_TIMER_MANAGER // For debug of the timer manager crash only. // TODO (mxyan): remove after bug is fixed. @@ -226,10 +287,20 @@ int gpr_cv_wait(gpr_cv* cv, gpr_mu* mu, gpr_timespec abs_deadline) { return err == ETIMEDOUT; } -void gpr_cv_signal(gpr_cv* cv) { GPR_ASSERT(pthread_cond_signal(cv) == 0); } +void gpr_cv_signal(gpr_cv* cv) { +#ifdef GRPC_ASAN_ENABLED + GPR_ASSERT(pthread_cond_signal(&cv->cond_var) == 0); +#else + GPR_ASSERT(pthread_cond_signal(cv) == 0); +#endif +} void gpr_cv_broadcast(gpr_cv* cv) { +#ifdef GRPC_ASAN_ENABLED + GPR_ASSERT(pthread_cond_broadcast(&cv->cond_var) == 0); +#else GPR_ASSERT(pthread_cond_broadcast(cv) == 0); +#endif } /*----------------------------------------*/