Bug 1698719 - Remove aSkipFrames argument to both FramePointerStackWalk and MozStackWalkThread. r=gsvelto,gerald
authorMike Hommey <mh@glandium.org>
Wed, 17 Mar 2021 00:21:39 +0000
changeset 571518 fcff76160855cb8c8045938bab31f4a87fad95f9
parent 571517 6328fc731c222bba0198e6785ea06146a3776402
child 571519 719843f5a24757802149c6ecc406a51a5811c017
push id38293
push usermalexandru@mozilla.com
push dateWed, 17 Mar 2021 09:53:31 +0000
treeherdermozilla-central@9ad67cd4d216 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgsvelto, gerald
bugs1698719, 1515229
milestone88.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 1698719 - Remove aSkipFrames argument to both FramePointerStackWalk and MozStackWalkThread. r=gsvelto,gerald In the case of FramePointerStackwalk, the caller gives a pointer to the top-most frame to walk from. There isn't really a reason to give a number of frames to skip, as the right frame pointer could be given in the first place if that was really necessary. And in practice, it's hasn't been used so far. In the case of MozStackWalkThread, the caller presumably doesn't know what the thread the stack is being walked for is doing, and it would be a guesswork to pass a valid number of frames to skip. In practice, it's also not used. The aSkipFrames is already a footgun on MozStackWalk (and we're going to change that in bug 1515229), we don't need to keep a footgun on these other stack walking methods. Differential Revision: https://phabricator.services.mozilla.com/D108563
memory/replace/dmd/DMD.cpp
memory/replace/phc/PHC.cpp
mozglue/baseprofiler/core/platform.cpp
mozglue/misc/StackWalk.cpp
mozglue/misc/StackWalk.h
tools/profiler/core/platform.cpp
--- a/memory/replace/dmd/DMD.cpp
+++ b/memory/replace/dmd/DMD.cpp
@@ -638,18 +638,17 @@ static uint32_t gGCStackTraceTableWhenSi
     // FramePointerStackWalk() on Win32: Registers::SyncPopulate() for the
     // frame pointer, and GetStackTop() for the stack end.
     CONTEXT context;
     RtlCaptureContext(&context);
     void** fp = reinterpret_cast<void**>(context.Ebp);
 
     PNT_TIB pTib = reinterpret_cast<PNT_TIB>(NtCurrentTeb());
     void* stackEnd = static_cast<void*>(pTib->StackBase);
-    FramePointerStackWalk(StackWalkCallback, /* skipFrames = */ 0, MaxFrames,
-                          &tmp, fp, stackEnd);
+    FramePointerStackWalk(StackWalkCallback, MaxFrames, &tmp, fp, stackEnd);
 #elif defined(XP_MACOSX)
     // This avoids MozStackWalk(), which has become unusably slow on Mac due to
     // changes in libunwind.
     //
     // This code is cribbed from the Gecko Profiler, which also uses
     // FramePointerStackWalk() on Mac: Registers::SyncPopulate() for the frame
     // pointer, and GetStackTop() for the stack end.
     void** fp;
@@ -657,18 +656,17 @@ static uint32_t gGCStackTraceTableWhenSi
     asm(
         // Dereference %rbp to get previous %rbp
         "movq (%%rbp), %0\n\t"
         : "=r"(fp));
 #  else
     asm("ldr %0, [x29]\n\t" : "=r"(fp));
 #  endif
     void* stackEnd = pthread_get_stackaddr_np(pthread_self());
-    FramePointerStackWalk(StackWalkCallback, /* skipFrames = */ 0, MaxFrames,
-                          &tmp, fp, stackEnd);
+    FramePointerStackWalk(StackWalkCallback, MaxFrames, &tmp, fp, stackEnd);
 #else
 #  if defined(XP_WIN) && defined(_M_X64)
     int skipFrames = 1;
 #  else
     int skipFrames = 2;
 #  endif
     MozStackWalk(StackWalkCallback, skipFrames, MaxFrames, &tmp);
 #endif
--- a/memory/replace/phc/PHC.cpp
+++ b/memory/replace/phc/PHC.cpp
@@ -211,33 +211,31 @@ void StackTrace::Fill() {
   // FramePointerStackWalk() on Win32: Registers::SyncPopulate() for the
   // frame pointer, and GetStackTop() for the stack end.
   CONTEXT context;
   RtlCaptureContext(&context);
   void** fp = reinterpret_cast<void**>(context.Ebp);
 
   PNT_TIB pTib = reinterpret_cast<PNT_TIB>(NtCurrentTeb());
   void* stackEnd = static_cast<void*>(pTib->StackBase);
-  FramePointerStackWalk(StackWalkCallback, /* aSkipFrames = */ 0, kMaxFrames,
-                        this, fp, stackEnd);
+  FramePointerStackWalk(StackWalkCallback, kMaxFrames, this, fp, stackEnd);
 #elif defined(XP_MACOSX)
   // This avoids MozStackWalk(), which has become unusably slow on Mac due to
   // changes in libunwind.
   //
   // This code is cribbed from the Gecko Profiler, which also uses
   // FramePointerStackWalk() on Mac: Registers::SyncPopulate() for the frame
   // pointer, and GetStackTop() for the stack end.
   void** fp;
   asm(
       // Dereference %rbp to get previous %rbp
       "movq (%%rbp), %0\n\t"
       : "=r"(fp));
   void* stackEnd = pthread_get_stackaddr_np(pthread_self());
-  FramePointerStackWalk(StackWalkCallback, /* skipFrames = */ 0, kMaxFrames,
-                        this, fp, stackEnd);
+  FramePointerStackWalk(StackWalkCallback, kMaxFrames, this, fp, stackEnd);
 #else
   MozStackWalk(StackWalkCallback, /* aSkipFrames = */ 0, kMaxFrames, this);
 #endif
 }
 
 //---------------------------------------------------------------------------
 // Logging
 //---------------------------------------------------------------------------
--- a/mozglue/baseprofiler/core/platform.cpp
+++ b/mozglue/baseprofiler/core/platform.cpp
@@ -1422,18 +1422,18 @@ static void DoFramePointerBacktrace(PSLo
   // but it doesn't matter because StackWalkCallback() doesn't use the frame
   // number argument.
   StackWalkCallback(/* frameNum */ 0, aRegs.mPC, aRegs.mSP, &aNativeStack);
 
   uint32_t maxFrames = uint32_t(MAX_NATIVE_FRAMES - aNativeStack.mCount);
 
   const void* stackEnd = aRegisteredThread.StackTop();
   if (aRegs.mFP >= aRegs.mSP && aRegs.mFP <= stackEnd) {
-    FramePointerStackWalk(StackWalkCallback, /* skipFrames */ 0, maxFrames,
-                          &aNativeStack, reinterpret_cast<void**>(aRegs.mFP),
+    FramePointerStackWalk(StackWalkCallback, maxFrames, &aNativeStack,
+                          reinterpret_cast<void**>(aRegs.mFP),
                           const_cast<void*>(stackEnd));
   }
 }
 #endif
 
 #if defined(USE_MOZ_STACK_WALK)
 static void DoMozStackWalkBacktrace(PSLockRef aLock,
                                     const RegisteredThread& aRegisteredThread,
@@ -1448,18 +1448,18 @@ static void DoMozStackWalkBacktrace(PSLo
   // it doesn't matter because StackWalkCallback() doesn't use the frame number
   // argument.
   StackWalkCallback(/* frameNum */ 0, aRegs.mPC, aRegs.mSP, &aNativeStack);
 
   uint32_t maxFrames = uint32_t(MAX_NATIVE_FRAMES - aNativeStack.mCount);
 
   HANDLE thread = GetThreadHandle(aRegisteredThread.GetPlatformData());
   MOZ_ASSERT(thread);
-  MozStackWalkThread(StackWalkCallback, /* skipFrames */ 0, maxFrames,
-                     &aNativeStack, thread, /* context */ nullptr);
+  MozStackWalkThread(StackWalkCallback, maxFrames, &aNativeStack, thread,
+                     /* context */ nullptr);
 }
 #endif
 
 #ifdef USE_EHABI_STACKWALK
 static void DoEHABIBacktrace(PSLockRef aLock,
                              const RegisteredThread& aRegisteredThread,
                              const Registers& aRegs,
                              NativeStack& aNativeStack) {
--- a/mozglue/misc/StackWalk.cpp
+++ b/mozglue/misc/StackWalk.cpp
@@ -371,17 +371,17 @@ static void WalkStackMain64(struct WalkS
 /**
  * Walk the stack, translating PC's found into strings and recording the
  * chain in aBuffer. For this to work properly, the DLLs must be rebased
  * so that the address in the file agrees with the address in memory.
  * Otherwise StackWalk will return FALSE when it hits a frame in a DLL
  * whose in memory address doesn't match its in-file address.
  */
 
-MFBT_API void MozStackWalkThread(MozWalkStackCallback aCallback,
+static void DoMozStackWalkThread(MozWalkStackCallback aCallback,
                                  uint32_t aSkipFrames, uint32_t aMaxFrames,
                                  void* aClosure, HANDLE aThread,
                                  CONTEXT* aContext) {
   struct WalkStackData data;
 
   InitializeDbgHelpCriticalSection();
 
   HANDLE targetThread = aThread;
@@ -420,20 +420,27 @@ MFBT_API void MozStackWalkThread(MozWalk
     WalkStackMain64(&data);
   }
 
   for (uint32_t i = 0; i < data.pc_count; ++i) {
     (*aCallback)(i + 1, data.pcs[i], data.sps[i], aClosure);
   }
 }
 
+MFBT_API void MozStackWalkThread(MozWalkStackCallback aCallback,
+                                 uint32_t aMaxFrames, void* aClosure,
+                                 HANDLE aThread, CONTEXT* aContext) {
+  DoMozStackWalkThread(aCallback, /* aSkipFrames = */ 0, aMaxFrames, aClosure,
+                       aThread, aContext);
+}
+
 MFBT_API void MozStackWalk(MozWalkStackCallback aCallback, uint32_t aSkipFrames,
                            uint32_t aMaxFrames, void* aClosure) {
-  MozStackWalkThread(aCallback, aSkipFrames, aMaxFrames, aClosure, nullptr,
-                     nullptr);
+  DoMozStackWalkThread(aCallback, aSkipFrames, aMaxFrames, aClosure, nullptr,
+                       nullptr);
 }
 
 static BOOL CALLBACK callbackEspecial64(PCSTR aModuleName, DWORD64 aModuleBase,
                                         ULONG aModuleSize, PVOID aUserContext) {
   BOOL retval = TRUE;
   DWORD64 addr = *(DWORD64*)aUserContext;
 
   /*
@@ -671,16 +678,21 @@ void DemangleSymbol(const char* aSymbol,
 }
 
 }  // namespace mozilla
 
 // {x86, ppc} x {Linux, Mac} stackwalking code.
 #  if ((defined(__i386) || defined(PPC) || defined(__ppc__)) && \
        (MOZ_STACKWALK_SUPPORTS_MACOSX || MOZ_STACKWALK_SUPPORTS_LINUX))
 
+static void DoFramePointerStackWalk(MozWalkStackCallback aCallback,
+                                    uint32_t aSkipFrames, uint32_t aMaxFrames,
+                                    void* aClosure, void** aBp,
+                                    void* aStackEnd);
+
 MFBT_API void MozStackWalk(MozWalkStackCallback aCallback, uint32_t aSkipFrames,
                            uint32_t aMaxFrames, void* aClosure) {
   // Get the frame pointer
   void** bp = (void**)__builtin_frame_address(0);
 
   void* stackEnd;
 #    if HAVE___LIBC_STACK_END
   stackEnd = __libc_stack_end;
@@ -709,18 +721,18 @@ MFBT_API void MozStackWalk(MozWalkStackC
     static const uintptr_t kMaxStackSize = 8 * 1024 * 1024;
     uintptr_t maxStackStart = uintptr_t(-1) - kMaxStackSize;
     uintptr_t stackStart = std::max(maxStackStart, uintptr_t(bp));
     stackEnd = reinterpret_cast<void*>(stackStart + kMaxStackSize);
   }
 #    else
 #      error Unsupported configuration
 #    endif
-  FramePointerStackWalk(aCallback, aSkipFrames, aMaxFrames, aClosure, bp,
-                        stackEnd);
+  DoFramePointerStackWalk(aCallback, aSkipFrames, aMaxFrames, aClosure, bp,
+                          stackEnd);
 }
 
 #  elif defined(HAVE__UNWIND_BACKTRACE)
 
 // libgcc_s.so symbols _Unwind_Backtrace@@GCC_3.3 and _Unwind_GetIP@@GCC_3.0
 #    include <unwind.h>
 
 struct unwind_info {
@@ -833,21 +845,21 @@ MFBT_API bool MozDescribeCodeAddress(voi
   aDetails->function[0] = '\0';
   aDetails->foffset = 0;
   return false;
 }
 
 #endif
 
 #if defined(XP_WIN) || defined(XP_MACOSX) || defined(XP_LINUX)
-namespace mozilla {
 MOZ_ASAN_BLACKLIST
-void FramePointerStackWalk(MozWalkStackCallback aCallback, uint32_t aSkipFrames,
-                           uint32_t aMaxFrames, void* aClosure, void** aBp,
-                           void* aStackEnd) {
+static void DoFramePointerStackWalk(MozWalkStackCallback aCallback,
+                                    uint32_t aSkipFrames, uint32_t aMaxFrames,
+                                    void* aClosure, void** aBp,
+                                    void* aStackEnd) {
   // Stack walking code courtesy Kipp's "leaky".
 
   int32_t skip = aSkipFrames;
   uint32_t numFrames = 0;
   while (aBp) {
     void** next = (void**)*aBp;
     // aBp may not be a frame pointer on i386 if code was compiled with
     // -fomit-frame-pointer, so do some sanity checks.
@@ -875,25 +887,33 @@ void FramePointerStackWalk(MozWalkStackC
       (*aCallback)(numFrames, pc, aBp, aClosure);
       if (aMaxFrames != 0 && numFrames == aMaxFrames) {
         break;
       }
     }
     aBp = next;
   }
 }
+
+namespace mozilla {
+
+void FramePointerStackWalk(MozWalkStackCallback aCallback, uint32_t aMaxFrames,
+                           void* aClosure, void** aBp, void* aStackEnd) {
+  DoFramePointerStackWalk(aCallback, /* aSkipFrames = */ 0, aMaxFrames,
+                          aClosure, aBp, aStackEnd);
+}
+
 }  // namespace mozilla
 
 #else
 
 namespace mozilla {
 MFBT_API void FramePointerStackWalk(MozWalkStackCallback aCallback,
-                                    uint32_t aSkipFrames, uint32_t aMaxFrames,
-                                    void* aClosure, void** aBp,
-                                    void* aStackEnd) {}
+                                    uint32_t aMaxFrames, void* aClosure,
+                                    void** aBp, void* aStackEnd) {}
 }  // namespace mozilla
 
 #endif
 
 MFBT_API void MozFormatCodeAddressDetails(
     char* aBuffer, uint32_t aBufferSize, uint32_t aFrameNumber, void* aPC,
     const MozCodeAddressDetails* aDetails) {
   MozFormatCodeAddress(aBuffer, aBufferSize, aFrameNumber, aPC,
--- a/mozglue/misc/StackWalk.h
+++ b/mozglue/misc/StackWalk.h
@@ -52,29 +52,27 @@ MFBT_API void MozStackWalk(MozWalkStackC
 #  define MOZ_STACKWALK_SUPPORTS_WINDOWS 1
 
 /**
  * Like MozStackWalk, but walks the stack for another thread.
  * Call aCallback for each stack frame on the current thread, from
  * the caller of MozStackWalk to main (or above).
  *
  * @param aCallback    Same as for MozStackWalk().
- * @param aSkipFrames  Same as for MozStackWalk().
  * @param aMaxFrames   Same as for MozStackWalk().
  * @param aClosure     Same as for MozStackWalk().
  * @param aThread      The handle of the thread whose stack is to be walked.
  *                     If 0, walks the current thread.
  * @param aContext     A CONTEXT, presumably obtained with GetThreadContext()
  *                     after suspending the thread with SuspendThread(). If
  *                     null, the CONTEXT will be re-obtained.
  */
 MFBT_API void MozStackWalkThread(MozWalkStackCallback aCallback,
-                                 uint32_t aSkipFrames, uint32_t aMaxFrames,
-                                 void* aClosure, HANDLE aThread,
-                                 CONTEXT* aContext);
+                                 uint32_t aMaxFrames, void* aClosure,
+                                 HANDLE aThread, CONTEXT* aContext);
 
 #else
 
 #  define MOZ_STACKWALK_SUPPORTS_WINDOWS 0
 
 #endif
 
 typedef struct {
@@ -159,19 +157,18 @@ MFBT_API void MozFormatCodeAddress(char*
  */
 MFBT_API void MozFormatCodeAddressDetails(
     char* aBuffer, uint32_t aBufferSize, uint32_t aFrameNumber, void* aPC,
     const MozCodeAddressDetails* aDetails);
 
 namespace mozilla {
 
 MFBT_API void FramePointerStackWalk(MozWalkStackCallback aCallback,
-                                    uint32_t aSkipFrames, uint32_t aMaxFrames,
-                                    void* aClosure, void** aBp,
-                                    void* aStackEnd);
+                                    uint32_t aMaxFrames, void* aClosure,
+                                    void** aBp, void* aStackEnd);
 
 #if defined(XP_LINUX) || defined(XP_FREEBSD)
 MFBT_API void DemangleSymbol(const char* aSymbol, char* aBuffer, int aBufLen);
 #endif
 
 }  // namespace mozilla
 
 #endif
--- a/tools/profiler/core/platform.cpp
+++ b/tools/profiler/core/platform.cpp
@@ -1977,18 +1977,18 @@ static void DoFramePointerBacktrace(PSLo
   // but it doesn't matter because StackWalkCallback() doesn't use the frame
   // number argument.
   StackWalkCallback(/* frameNum */ 0, aRegs.mPC, aRegs.mSP, &aNativeStack);
 
   uint32_t maxFrames = uint32_t(MAX_NATIVE_FRAMES - aNativeStack.mCount);
 
   const void* stackEnd = aRegisteredThread.StackTop();
   if (aRegs.mFP >= aRegs.mSP && aRegs.mFP <= stackEnd) {
-    FramePointerStackWalk(StackWalkCallback, /* skipFrames */ 0, maxFrames,
-                          &aNativeStack, reinterpret_cast<void**>(aRegs.mFP),
+    FramePointerStackWalk(StackWalkCallback, maxFrames, &aNativeStack,
+                          reinterpret_cast<void**>(aRegs.mFP),
                           const_cast<void*>(stackEnd));
   }
 }
 #endif
 
 #if defined(USE_MOZ_STACK_WALK)
 static void DoMozStackWalkBacktrace(PSLockRef aLock,
                                     const RegisteredThread& aRegisteredThread,
@@ -2003,18 +2003,18 @@ static void DoMozStackWalkBacktrace(PSLo
   // it doesn't matter because StackWalkCallback() doesn't use the frame number
   // argument.
   StackWalkCallback(/* frameNum */ 0, aRegs.mPC, aRegs.mSP, &aNativeStack);
 
   uint32_t maxFrames = uint32_t(MAX_NATIVE_FRAMES - aNativeStack.mCount);
 
   HANDLE thread = GetThreadHandle(aRegisteredThread.GetPlatformData());
   MOZ_ASSERT(thread);
-  MozStackWalkThread(StackWalkCallback, /* skipFrames */ 0, maxFrames,
-                     &aNativeStack, thread, /* context */ nullptr);
+  MozStackWalkThread(StackWalkCallback, maxFrames, &aNativeStack, thread,
+                     /* context */ nullptr);
 }
 #endif
 
 #ifdef USE_EHABI_STACKWALK
 static void DoEHABIBacktrace(PSLockRef aLock,
                              const RegisteredThread& aRegisteredThread,
                              const Registers& aRegs,
                              NativeStack& aNativeStack) {