Bug 1384814 - Remove critical address machinery from Mac implementation of MozStackWalk(). r=glandium.
authorNicholas Nethercote <nnethercote@mozilla.com>
Tue, 03 Oct 2017 13:53:14 +1100
changeset 384132 c3729a7809983b9b69b1e3b79a0582345398ef93
parent 384131 e484df87b3ee915f29b0f37f47e045f7e1f092cd
child 384133 11fe0a2895aab26c57bcfe61b3041d7837e954cd
child 384179 919a4eb296213bcb1bda2e16ff0d5eeb2c03a5e8
push id32620
push userarchaeopteryx@coole-files.de
push dateTue, 03 Oct 2017 09:44:09 +0000
treeherdermozilla-central@11fe0a2895aa [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersglandium
bugs1384814
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1384814 - Remove critical address machinery from Mac implementation of MozStackWalk(). r=glandium. It seemingly hasn't been needed since Mac OS 10.7. A diagnostic assertion that has been in place for a while hasn't caught any uses of it.
memory/replace/dmd/DMD.cpp
mozglue/misc/StackWalk.cpp
mozglue/misc/StackWalk.h
xpcom/base/nsTraceRefcnt.cpp
--- a/memory/replace/dmd/DMD.cpp
+++ b/memory/replace/dmd/DMD.cpp
@@ -1556,24 +1556,16 @@ Options::ModeString() const
     return "(unknown DMD mode)";
   }
 }
 
 //---------------------------------------------------------------------------
 // DMD start-up
 //---------------------------------------------------------------------------
 
-#ifdef XP_MACOSX
-static void
-NopStackWalkCallback(uint32_t aFrameNumber, void* aPc, void* aSp,
-                     void* aClosure)
-{
-}
-#endif
-
 static void
 prefork()
 {
   if (gStateLock) {
     gStateLock->Lock();
   }
 }
 
@@ -1612,27 +1604,16 @@ Init(const malloc_table_t* aMallocTable)
     StatusMsg("$DMD = '%s'\n", e);
   } else {
     StatusMsg("$DMD is undefined\n");
   }
 
   // Parse $DMD env var.
   gOptions = InfallibleAllocPolicy::new_<Options>(e);
 
-#ifdef XP_MACOSX
-  // On Mac OS X we need to call StackWalkInitCriticalAddress() very early
-  // (prior to the creation of any mutexes, apparently) otherwise we can get
-  // hangs when getting stack traces (bug 821577).  But
-  // StackWalkInitCriticalAddress() isn't exported from xpcom/, so instead we
-  // just call MozStackWalk, because that calls StackWalkInitCriticalAddress().
-  // See the comment above StackWalkInitCriticalAddress() for more details.
-  (void)MozStackWalk(NopStackWalkCallback, /* skipFrames */ 0,
-                     /* maxFrames */ 1, nullptr);
-#endif
-
   gStateLock = InfallibleAllocPolicy::new_<Mutex>();
 
   gBernoulli = (FastBernoulliTrial*)
     InfallibleAllocPolicy::malloc_(sizeof(FastBernoulliTrial));
   ResetBernoulli();
 
   DMD_CREATE_TLS_INDEX(gTlsIndex);
 
--- a/mozglue/misc/StackWalk.cpp
+++ b/mozglue/misc/StackWalk.cpp
@@ -10,25 +10,16 @@
 #include "mozilla/Assertions.h"
 #include "mozilla/IntegerPrintfMacros.h"
 #include "mozilla/StackWalk.h"
 
 #include <string.h>
 
 using namespace mozilla;
 
-// The presence of this address is the stack must stop the stack walk. If
-// there is no such address, the structure will be {nullptr, true}.
-struct CriticalAddress
-{
-  void* mAddr;
-  bool mInit;
-};
-static CriticalAddress gCriticalAddress;
-
 // for _Unwind_Backtrace from libcxxrt or libunwind
 // cxxabi.h from libcxxrt implicitly includes unwind.h first
 #if defined(HAVE__UNWIND_BACKTRACE) && !defined(_GNU_SOURCE)
 #define _GNU_SOURCE
 #endif
 
 #if defined(HAVE_DLOPEN) || defined(XP_DARWIN)
 #include <dlfcn.h>
@@ -60,137 +51,16 @@ extern MOZ_EXPORT void* __libc_stack_end
 #endif
 
 #ifdef ANDROID
 #include <algorithm>
 #include <unistd.h>
 #include <pthread.h>
 #endif
 
-#if MOZ_STACKWALK_SUPPORTS_MACOSX
-#include <pthread.h>
-#include <sys/errno.h>
-#ifdef MOZ_WIDGET_COCOA
-#include <CoreServices/CoreServices.h>
-#endif
-
-typedef void
-malloc_logger_t(uint32_t aType,
-                uintptr_t aArg1, uintptr_t aArg2, uintptr_t aArg3,
-                uintptr_t aResult, uint32_t aNumHotFramesToSkip);
-extern malloc_logger_t* malloc_logger;
-
-static void
-stack_callback(uint32_t aFrameNumber, void* aPc, void* aSp, void* aClosure)
-{
-  const char* name = static_cast<char*>(aClosure);
-  Dl_info info;
-
-  // On Leopard dladdr returns the wrong value for "new_sem_from_pool". The
-  // stack shows up as having two pthread_cond_wait$UNIX2003 frames. The
-  // correct one is the first that we find on our way up, so the
-  // following check for gCriticalAddress.mAddr is critical.
-  if (gCriticalAddress.mAddr || dladdr(aPc, &info) == 0  ||
-      !info.dli_sname || strcmp(info.dli_sname, name) != 0) {
-    return;
-  }
-  gCriticalAddress.mAddr = aPc;
-}
-
-static void
-my_malloc_logger(uint32_t aType,
-                 uintptr_t aArg1, uintptr_t aArg2, uintptr_t aArg3,
-                 uintptr_t aResult, uint32_t aNumHotFramesToSkip)
-{
-  static bool once = false;
-  if (once) {
-    return;
-  }
-  once = true;
-
-  // On Leopard dladdr returns the wrong value for "new_sem_from_pool". The
-  // stack shows up as having two pthread_cond_wait$UNIX2003 frames.
-  const char* name = "new_sem_from_pool";
-  MozStackWalk(stack_callback, /* skipFrames */ 0, /* maxFrames */ 0,
-               const_cast<char*>(name));
-}
-
-// This is called from NS_LogInit() and from the stack walking functions, but
-// only the first call has any effect.  We need to call this function from both
-// places because it must run before any mutexes are created, and also before
-// any objects whose refcounts we're logging are created.  Running this
-// function during NS_LogInit() ensures that we meet the first criterion, and
-// running this function during the stack walking functions ensures we meet the
-// second criterion.
-MFBT_API void
-StackWalkInitCriticalAddress()
-{
-  if (gCriticalAddress.mInit) {
-    return;
-  }
-  gCriticalAddress.mInit = true;
-  // We must not do work when 'new_sem_from_pool' calls realloc, since
-  // it holds a non-reentrant spin-lock and we will quickly deadlock.
-  // new_sem_from_pool is not directly accessible using dlsym, so
-  // we force a situation where new_sem_from_pool is on the stack and
-  // use dladdr to check the addresses.
-
-  // malloc_logger can be set by external tools like 'Instruments' or 'leaks'
-  malloc_logger_t* old_malloc_logger = malloc_logger;
-  malloc_logger = my_malloc_logger;
-
-  pthread_cond_t cond;
-  int r = pthread_cond_init(&cond, 0);
-  MOZ_ASSERT(r == 0);
-  pthread_mutex_t mutex;
-  r = pthread_mutex_init(&mutex, 0);
-  MOZ_ASSERT(r == 0);
-  r = pthread_mutex_lock(&mutex);
-  MOZ_ASSERT(r == 0);
-  struct timespec abstime = { 0, 1 };
-  r = pthread_cond_timedwait_relative_np(&cond, &mutex, &abstime);
-
-  // restore the previous malloc logger
-  malloc_logger = old_malloc_logger;
-
-  // XXX: the critical address machinery appears to have been unnecessary since
-  // Mac OS 10.7 (the minimum version we currently support is 10.9). See bug
-  // 1384814 for details.
-  MOZ_DIAGNOSTIC_ASSERT(!gCriticalAddress.mAddr);
-
-  MOZ_ASSERT(r == ETIMEDOUT);
-  r = pthread_mutex_unlock(&mutex);
-  MOZ_ASSERT(r == 0);
-  r = pthread_mutex_destroy(&mutex);
-  MOZ_ASSERT(r == 0);
-  r = pthread_cond_destroy(&cond);
-  MOZ_ASSERT(r == 0);
-}
-
-static bool
-IsCriticalAddress(void* aPC)
-{
-  return gCriticalAddress.mAddr == aPC;
-}
-#else
-static bool
-IsCriticalAddress(void* aPC)
-{
-  return false;
-}
-// We still initialize gCriticalAddress.mInit so that this code behaves
-// the same on all platforms. Otherwise a failure to init would be visible
-// only on OS X.
-MFBT_API void
-StackWalkInitCriticalAddress()
-{
-  gCriticalAddress.mInit = true;
-}
-#endif
-
 #if MOZ_STACKWALK_SUPPORTS_WINDOWS
 
 #include <windows.h>
 #include <process.h>
 #include <stdio.h>
 #include <malloc.h>
 #include "mozilla/ArrayUtils.h"
 #include "mozilla/Atomics.h"
@@ -619,17 +489,16 @@ WalkStackThread(void* aData)
  * whose in memory address doesn't match its in-file address.
  */
 
 MFBT_API void
 MozStackWalkThread(MozWalkStackCallback aCallback, uint32_t aSkipFrames,
                    uint32_t aMaxFrames, void* aClosure,
                    HANDLE aThread, CONTEXT* aContext)
 {
-  StackWalkInitCriticalAddress();
   static HANDLE myProcess = nullptr;
   HANDLE myThread;
   DWORD walkerReturn;
   struct WalkStackData data;
 
   InitializeDbgHelpCriticalSection();
 
   // EnsureWalkThreadReady's _beginthreadex takes a heap lock and must be
@@ -992,18 +861,16 @@ void DemangleSymbol(const char* aSymbol,
 // {x86, ppc} x {Linux, Mac} stackwalking code.
 #if ((defined(__i386) || defined(PPC) || defined(__ppc__)) && \
      (MOZ_STACKWALK_SUPPORTS_MACOSX || MOZ_STACKWALK_SUPPORTS_LINUX))
 
 MFBT_API void
 MozStackWalk(MozWalkStackCallback aCallback, uint32_t aSkipFrames,
              uint32_t aMaxFrames, void* aClosure)
 {
-  StackWalkInitCriticalAddress();
-
   // Get the frame pointer
   void** bp = (void**)__builtin_frame_address(0);
 
   void* stackEnd;
 #if HAVE___LIBC_STACK_END
   stackEnd = __libc_stack_end;
 #elif defined(XP_DARWIN)
   stackEnd = pthread_get_stackaddr_np(pthread_self());
@@ -1054,38 +921,31 @@ struct unwind_info
 };
 
 static _Unwind_Reason_Code
 unwind_callback(struct _Unwind_Context* context, void* closure)
 {
   unwind_info* info = static_cast<unwind_info*>(closure);
   void* pc = reinterpret_cast<void*>(_Unwind_GetIP(context));
   // TODO Use something like '_Unwind_GetGR()' to get the stack pointer.
-  if (IsCriticalAddress(pc)) {
-    // We just want to stop the walk, so any error code will do.  Using
-    // _URC_NORMAL_STOP would probably be the most accurate, but it is not
-    // defined on Android for ARM.
-    return _URC_FOREIGN_EXCEPTION_CAUGHT;
-  }
   if (--info->skip < 0) {
     info->numFrames++;
     (*info->callback)(info->numFrames, pc, nullptr, info->closure);
     if (info->maxFrames != 0 && info->numFrames == info->maxFrames) {
       // Again, any error code that stops the walk will do.
       return _URC_FOREIGN_EXCEPTION_CAUGHT;
     }
   }
   return _URC_NO_REASON;
 }
 
 MFBT_API void
 MozStackWalk(MozWalkStackCallback aCallback, uint32_t aSkipFrames,
              uint32_t aMaxFrames, void* aClosure)
 {
-  StackWalkInitCriticalAddress();
   unwind_info info;
   info.callback = aCallback;
   info.skip = aSkipFrames + 1;
   info.maxFrames = aMaxFrames;
   info.numFrames = 0;
   info.closure = aClosure;
 
   // We ignore the return value from _Unwind_Backtrace. There are three main
@@ -1188,19 +1048,16 @@ FramePointerStackWalk(MozWalkStackCallba
 #if (defined(__ppc__) && defined(XP_MACOSX)) || defined(__powerpc64__)
     // ppc mac or powerpc64 linux
     void* pc = *(aBp + 2);
     aBp += 3;
 #else // i386 or powerpc32 linux
     void* pc = *(aBp + 1);
     aBp += 2;
 #endif
-    if (IsCriticalAddress(pc)) {
-      return;
-    }
     if (--skip < 0) {
       // Assume that the SP points to the BP of the function
       // it called. We can't know the exact location of the SP
       // but this should be sufficient for our use the SP
       // to order elements on the stack.
       numFrames++;
       (*aCallback)(numFrames, pc, aBp, aClosure);
       if (aMaxFrames != 0 && numFrames == aMaxFrames) {
--- a/mozglue/misc/StackWalk.h
+++ b/mozglue/misc/StackWalk.h
@@ -169,16 +169,9 @@ namespace mozilla {
 
 MFBT_API void
 FramePointerStackWalk(MozWalkStackCallback aCallback, uint32_t aSkipFrames,
                       uint32_t aMaxFrames, void* aClosure, void** aBp,
                       void* aStackEnd);
 
 } // namespace mozilla
 
-/**
- * Initialize the critical sections for this platform so that we can
- * abort stack walks when needed.
- */
-MFBT_API void
-StackWalkInitCriticalAddress(void);
-
 #endif
--- a/xpcom/base/nsTraceRefcnt.cpp
+++ b/xpcom/base/nsTraceRefcnt.cpp
@@ -916,17 +916,16 @@ WalkTheStackSavingLocations(std::vector<
 //----------------------------------------------------------------------
 
 EXPORT_XPCOM_API(void)
 NS_LogInit()
 {
   NS_SetMainThread();
 
   // FIXME: This is called multiple times, we should probably not allow that.
-  StackWalkInitCriticalAddress();
   if (++gInitCount) {
     nsTraceRefcnt::SetActivityIsLegal(true);
   }
 }
 
 EXPORT_XPCOM_API(void)
 NS_LogTerm()
 {