From a32f064e978ce53d92b4106e794c58ba375e0aa6 Mon Sep 17 00:00:00 2001 From: "tommi@webrtc.org" Date: Sun, 8 Mar 2015 07:38:31 +0000 Subject: [PATCH] Fix build configuration bug with debug builds. The problem we were running into on the Mac 10.9 debug bot in Chrome turned out to be good ol'fashion memory corruption. Part of webrtc was being compiled with _DEBUG, another half without it. This caused the definition of some symbols to be out of sync (notably pthread_mutex_t) and would cause code built from common.gypi, to overwrite memory allocated via common types from base/base.gypi derived code. Fun stuff to track down. This was a problem in particular with base/criticalsection.h since it's inlined into multiple object files but will have different definitions of what a mutex is. TBR=pbos,kjellander BUG= Review URL: https://webrtc-codereview.appspot.com/43659004 Cr-Commit-Position: refs/heads/master@{#8646} git-svn-id: http://webrtc.googlecode.com/svn/trunk@8646 4adac7df-926f-26a2-2b94-8c16560cd09d --- webrtc/base/criticalsection.h | 25 +------------------------ webrtc/build/common.gypi | 30 ++++++++++++++++++++---------- 2 files changed, 21 insertions(+), 34 deletions(-) diff --git a/webrtc/base/criticalsection.h b/webrtc/base/criticalsection.h index c78e818f93..088e72e142 100644 --- a/webrtc/base/criticalsection.h +++ b/webrtc/base/criticalsection.h @@ -27,30 +27,8 @@ #include #endif -#if defined(WEBRTC_MAC) -// TODO(tommi): This is a temporary test to see if critical section objects are -// somehow causing pointer co-member variables that follow a critical section -// variable, are somehow throwing off the alignment and causing crash on -// the Mac 10.9 debug bot: -// http://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests%20(dbg) -#define _MUTEX_ALIGNMENT __attribute__((__aligned__(8))) -#define _STATIC_ASSERT_CRITICAL_SECTION_SIZE() \ - static_assert(sizeof(CriticalSection) % 8 == 0, \ - "Bad size of CriticalSection") - -#else -#define _MUTEX_ALIGNMENT -#define _STATIC_ASSERT_CRITICAL_SECTION_SIZE() -#endif - #ifdef _DEBUG -#if defined(WEBRTC_MAC) -// TODO(tommi): This is related to the comment above. It looks like the -// pthread_t member might be throwing off the mac debug bots (10.9). -#define CS_TRACK_OWNER 0 -#else #define CS_TRACK_OWNER 1 -#endif #endif // _DEBUG #if CS_TRACK_OWNER @@ -90,7 +68,6 @@ class LOCKABLE CriticalSection { class LOCKABLE CriticalSection { public: CriticalSection() { - _STATIC_ASSERT_CRITICAL_SECTION_SIZE(); pthread_mutexattr_t mutex_attribute; pthread_mutexattr_init(&mutex_attribute); pthread_mutexattr_settype(&mutex_attribute, PTHREAD_MUTEX_RECURSIVE); @@ -127,7 +104,7 @@ class LOCKABLE CriticalSection { } private: - _MUTEX_ALIGNMENT pthread_mutex_t mutex_; + pthread_mutex_t mutex_; TRACK_OWNER(pthread_t thread_); }; #endif // WEBRTC_POSIX diff --git a/webrtc/build/common.gypi b/webrtc/build/common.gypi index 05e1ba3ec2..9fec935bc8 100644 --- a/webrtc/build/common.gypi +++ b/webrtc/build/common.gypi @@ -178,6 +178,26 @@ ['rtc_relative_path==1', { 'defines': ['EXPAT_RELATIVE_PATH',], }], + ['os_posix==1', { + 'configurations': { + 'Debug_Base': { + 'defines': [ + # Chromium's build/common.gypi defines _DEBUG for all posix + # _except_ for ios & mac. The size of data types such as + # pthread_mutex_t varies between release and debug builds + # and is controlled via this flag. Since we now share code + # between base/base.gyp and build/common.gypi (this file), + # both gyp(i) files, must consistently set this flag uniformly + # or else we'll run in to hard-to-figure-out problems where + # one compilation unit uses code from another but expects + # differently laid out types. + # For WebRTC, we want it there as well, because ASSERT and + # friends trigger off of it. + '_DEBUG', + ], + }, + }, + }], ['build_with_chromium==1', { 'defines': [ # Changes settings for Chromium build. @@ -202,16 +222,6 @@ ], 'conditions': [ ['os_posix==1', { - 'configurations': { - 'Debug_Base': { - 'defines': [ - # Chromium's build/common.gypi defines this for all posix - # _except_ for ios & mac. We want it there as well, e.g. - # because ASSERT and friends trigger off of it. - '_DEBUG', - ], - }, - }, 'conditions': [ # -Wextra is currently disabled in Chromium's common.gypi. Enable # for targets that can handle it. For Android/arm64 right now