Bug 1515229 - Make MozStackWalk/MozWalkTheStack frame skipping more reliable. r=gerald,nika,bobowen,jld

Differential Revision: https://phabricator.services.mozilla.com/D110899

Author: glandium

memory/replace/dmd/DMD.cpp

@@ -663,12 +663,7 @@ static uint32_t gGCStackTraceTableWhenSizeExceeds = 4 * 1024;
     void* stackEnd = pthread_get_stackaddr_np(pthread_self());
     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);
+    MozStackWalk(StackWalkCallback, nullptr, MaxFrames, &tmp);
 #endif
   }
 

memory/replace/phc/PHC.cpp

@@ -232,7 +232,7 @@ void StackTrace::Fill() {
   void* stackEnd = pthread_get_stackaddr_np(pthread_self());
   FramePointerStackWalk(StackWalkCallback, kMaxFrames, this, fp, stackEnd);
 #else
-  MozStackWalk(StackWalkCallback, /* aSkipFrames = */ 0, kMaxFrames, this);
+  MozStackWalk(StackWalkCallback, nullptr, kMaxFrames, this);
 #endif
 }
 

mfbt/Assertions.h

@@ -91,13 +91,13 @@ MOZ_ReportAssertionFailure(const char* aStr, const char* aFilename,
                       "Assertion failure: %s, at %s:%d\n", aStr, aFilename,
                       aLine);
 #  if defined(MOZ_DUMP_ASSERTION_STACK)
-  MozWalkTheStackWithWriter(MOZ_ReportAssertionFailurePrintFrame,
-                            /* aSkipFrames */ 1, /* aMaxFrames */ 0);
+  MozWalkTheStackWithWriter(MOZ_ReportAssertionFailurePrintFrame, CallerPC(),
+                            /* aMaxFrames */ 0);
 #  endif
 #else
   fprintf(stderr, "Assertion failure: %s, at %s:%d\n", aStr, aFilename, aLine);
 #  if defined(MOZ_DUMP_ASSERTION_STACK)
-  MozWalkTheStack(stderr, /* aSkipFrames */ 1, /* aMaxFrames */ 0);
+  MozWalkTheStack(stderr, CallerPC(), /* aMaxFrames */ 0);
 #  endif
   fflush(stderr);
 #endif
@@ -112,7 +112,7 @@ MOZ_MAYBE_UNUSED static MOZ_COLD MOZ_NEVER_INLINE void MOZ_ReportCrash(
 #else
   fprintf(stderr, "Hit MOZ_CRASH(%s) at %s:%d\n", aStr, aFilename, aLine);
 #  if defined(MOZ_DUMP_ASSERTION_STACK)
-  MozWalkTheStack(stderr, /* aSkipFrames */ 1, /* aMaxFrames */ 0);
+  MozWalkTheStack(stderr, CallerPC(), /* aMaxFrames */ 0);
 #  endif
   fflush(stderr);
 #endif

mozglue/misc/StackWalk.cpp

@@ -69,6 +69,26 @@ extern MOZ_EXPORT void* __libc_stack_end;  // from ld-linux.so
 #  include <pthread.h>
 #endif
 
+class FrameSkipper {
+ public:
+  constexpr FrameSkipper() : mPc(0) {}
+  bool ShouldSkipPC(void* aPC) {
+    // Skip frames until we encounter the one we were initialized with,
+    // and then never skip again.
+    if (mPc != 0) {
+      if (mPc != uintptr_t(aPC)) {
+        return true;
+      }
+      mPc = 0;
+    }
+    return false;
+  }
+  explicit FrameSkipper(const void* aPc) : mPc(uintptr_t(aPc)) {}
+
+ private:
+  uintptr_t mPc;
+};
+
 #ifdef XP_WIN
 
 #  include <windows.h>
@@ -92,7 +112,7 @@ struct WalkStackData {
   // Are we walking the stack of the calling thread? Note that we need to avoid
   // calling fprintf and friends if this is false, in order to avoid deadlocks.
   bool walkCallingThread;
-  uint32_t skipFrames;
+  const void* firstFramePC;
   HANDLE thread;
   HANDLE process;
   HANDLE eventStart;
@@ -243,8 +263,7 @@ static void WalkStackMain64(struct WalkStackData* aData) {
   bool firstFrame = true;
 #  endif
 
-  // Skip our own stack walking frames.
-  int skip = (aData->walkCallingThread ? 3 : 0) + aData->skipFrames;
+  FrameSkipper skipper(aData->firstFramePC);
 
   // Now walk the stack.
   while (true) {
@@ -349,7 +368,7 @@ static void WalkStackMain64(struct WalkStackData* aData) {
       break;
     }
 
-    if (skip-- > 0) {
+    if (skipper.ShouldSkipPC((void*)addr)) {
       continue;
     }
 
@@ -384,7 +403,7 @@ static void WalkStackMain64(struct WalkStackData* aData) {
  */
 
 static void DoMozStackWalkThread(MozWalkStackCallback aCallback,
-                                 uint32_t aSkipFrames, uint32_t aMaxFrames,
+                                 const void* aFirstFramePC, uint32_t aMaxFrames,
                                  void* aClosure, HANDLE aThread,
                                  CONTEXT* aContext) {
   struct WalkStackData data;
@@ -401,7 +420,7 @@ static void DoMozStackWalkThread(MozWalkStackCallback aCallback,
     data.walkCallingThread = (threadId == currentThreadId);
   }
 
-  data.skipFrames = aSkipFrames;
+  data.firstFramePC = aFirstFramePC;
   data.thread = targetThread;
   data.process = ::GetCurrentProcess();
   void* local_pcs[1024];
@@ -435,14 +454,17 @@ static void DoMozStackWalkThread(MozWalkStackCallback aCallback,
 MFBT_API void MozStackWalkThread(MozWalkStackCallback aCallback,
                                  uint32_t aMaxFrames, void* aClosure,
                                  HANDLE aThread, CONTEXT* aContext) {
-  DoMozStackWalkThread(aCallback, /* aSkipFrames = */ 0, aMaxFrames, aClosure,
-                       aThread, aContext);
+  // We don't pass a aFirstFramePC because we walk the stack for another
+  // thread.
+  DoMozStackWalkThread(aCallback, nullptr, aMaxFrames, aClosure, aThread,
+                       aContext);
 }
 
-MFBT_API void MozStackWalk(MozWalkStackCallback aCallback, uint32_t aSkipFrames,
-                           uint32_t aMaxFrames, void* aClosure) {
-  DoMozStackWalkThread(aCallback, aSkipFrames, aMaxFrames, aClosure, nullptr,
-                       nullptr);
+MFBT_API void MozStackWalk(MozWalkStackCallback aCallback,
+                           const void* aFirstFramePC, uint32_t aMaxFrames,
+                           void* aClosure) {
+  DoMozStackWalkThread(aCallback, aFirstFramePC ? aFirstFramePC : CallerPC(),
+                       aMaxFrames, aClosure, nullptr, nullptr);
 }
 
 static BOOL CALLBACK callbackEspecial64(PCSTR aModuleName, DWORD64 aModuleBase,
@@ -691,12 +713,13 @@ void DemangleSymbol(const char* aSymbol, char* aBuffer, int aBufLen) {
        (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);
+                                    const void* aFirstFramePC,
+                                    uint32_t aMaxFrames, void* aClosure,
+                                    void** aBp, void* aStackEnd);
 
-MFBT_API void MozStackWalk(MozWalkStackCallback aCallback, uint32_t aSkipFrames,
-                           uint32_t aMaxFrames, void* aClosure) {
+MFBT_API void MozStackWalk(MozWalkStackCallback aCallback,
+                           const void* aFirstFramePC, uint32_t aMaxFrames,
+                           void* aClosure) {
   // Get the frame pointer
   void** bp = (void**)__builtin_frame_address(0);
 
@@ -733,7 +756,7 @@ MFBT_API void MozStackWalk(MozWalkStackCallback aCallback, uint32_t aSkipFrames,
 #    else
 #      error Unsupported configuration
 #    endif
-  DoFramePointerStackWalk(aCallback, aSkipFrames, aMaxFrames, aClosure, bp,
+  DoFramePointerStackWalk(aCallback, aFirstFramePC, aMaxFrames, aClosure, bp,
                           stackEnd);
 }
 
@@ -744,7 +767,7 @@ MFBT_API void MozStackWalk(MozWalkStackCallback aCallback, uint32_t aSkipFrames,
 
 struct unwind_info {
   MozWalkStackCallback callback;
-  int skip;
+  FrameSkipper skipper;
   int maxFrames;
   int numFrames;
   void* closure;
@@ -755,7 +778,7 @@ static _Unwind_Reason_Code unwind_callback(struct _Unwind_Context* context,
   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 (--info->skip < 0) {
+  if (!info->skipper.ShouldSkipPC(pc)) {
     info->numFrames++;
     (*info->callback)(info->numFrames, pc, nullptr, info->closure);
     if (info->maxFrames != 0 && info->numFrames == info->maxFrames) {
@@ -766,11 +789,12 @@ static _Unwind_Reason_Code unwind_callback(struct _Unwind_Context* context,
   return _URC_NO_REASON;
 }
 
-MFBT_API void MozStackWalk(MozWalkStackCallback aCallback, uint32_t aSkipFrames,
-                           uint32_t aMaxFrames, void* aClosure) {
+MFBT_API void MozStackWalk(MozWalkStackCallback aCallback,
+                           const void* aFirstFramePC, uint32_t aMaxFrames,
+                           void* aClosure) {
   unwind_info info;
   info.callback = aCallback;
-  info.skip = aSkipFrames + 1;
+  info.skipper = FrameSkipper(aFirstFramePC ? aFirstFramePC : CallerPC());
   info.maxFrames = aMaxFrames;
   info.numFrames = 0;
   info.closure = aClosure;
@@ -840,8 +864,9 @@ bool MFBT_API MozDescribeCodeAddress(void* aPC,
 
 #else  // unsupported platform.
 
-MFBT_API void MozStackWalk(MozWalkStackCallback aCallback, uint32_t aSkipFrames,
-                           uint32_t aMaxFrames, void* aClosure) {}
+MFBT_API void MozStackWalk(MozWalkStackCallback aCallback,
+                           const void* aFirstFramePC, uint32_t aMaxFrames,
+                           void* aClosure) {}
 
 MFBT_API bool MozDescribeCodeAddress(void* aPC,
                                      MozCodeAddressDetails* aDetails) {
@@ -859,13 +884,14 @@ MFBT_API bool MozDescribeCodeAddress(void* aPC,
 #if defined(XP_WIN) || defined(XP_MACOSX) || defined(XP_LINUX)
 MOZ_ASAN_BLACKLIST
 static void DoFramePointerStackWalk(MozWalkStackCallback aCallback,
-                                    uint32_t aSkipFrames, uint32_t aMaxFrames,
-                                    void* aClosure, void** aBp,
-                                    void* aStackEnd) {
+                                    const void* aFirstFramePC,
+                                    uint32_t aMaxFrames, void* aClosure,
+                                    void** aBp, void* aStackEnd) {
   // Stack walking code courtesy Kipp's "leaky".
 
-  int32_t skip = aSkipFrames;
+  FrameSkipper skipper(aFirstFramePC);
   uint32_t numFrames = 0;
+
   while (aBp) {
     void** next = (void**)*aBp;
     // aBp may not be a frame pointer on i386 if code was compiled with
@@ -885,7 +911,7 @@ static void DoFramePointerStackWalk(MozWalkStackCallback aCallback,
     void* pc = *(aBp + 1);
     aBp += 2;
 #  endif
-    if (--skip < 0) {
+    if (!skipper.ShouldSkipPC(pc)) {
       // 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
@@ -904,8 +930,10 @@ namespace mozilla {
 
 void FramePointerStackWalk(MozWalkStackCallback aCallback, uint32_t aMaxFrames,
                            void* aClosure, void** aBp, void* aStackEnd) {
-  DoFramePointerStackWalk(aCallback, /* aSkipFrames = */ 0, aMaxFrames,
-                          aClosure, aBp, aStackEnd);
+  // We don't pass a aFirstFramePC because we start walking the stack from the
+  // frame at aBp.
+  DoFramePointerStackWalk(aCallback, nullptr, aMaxFrames, aClosure, aBp,
+                          aStackEnd);
 }
 
 }  // namespace mozilla
@@ -914,6 +942,7 @@ void FramePointerStackWalk(MozWalkStackCallback aCallback, uint32_t aMaxFrames,
 
 namespace mozilla {
 MFBT_API void FramePointerStackWalk(MozWalkStackCallback aCallback,
+                                    const void* aFirstFramePC,
                                     uint32_t aMaxFrames, void* aClosure,
                                     void** aBp, void* aStackEnd) {}
 }  // namespace mozilla
@@ -1006,10 +1035,11 @@ static bool WalkTheStackEnabled() {
   return result;
 }
 
-MFBT_API void MozWalkTheStack(FILE* aStream, uint32_t aSkipFrames,
+MFBT_API void MozWalkTheStack(FILE* aStream, const void* aFirstFramePC,
                               uint32_t aMaxFrames) {
   if (WalkTheStackEnabled()) {
-    MozStackWalk(PrintStackFrame, aSkipFrames + 1, aMaxFrames, aStream);
+    MozStackWalk(PrintStackFrame, aFirstFramePC ? aFirstFramePC : CallerPC(),
+                 aMaxFrames, aStream);
   }
 }
 
@@ -1022,9 +1052,10 @@ static void WriteStackFrame(uint32_t aFrameNumber, void* aPC, void* aSP,
 }
 
 MFBT_API void MozWalkTheStackWithWriter(void (*aWriter)(const char*),
-                                        uint32_t aSkipFrames,
+                                        const void* aFirstFramePC,
                                         uint32_t aMaxFrames) {
   if (WalkTheStackEnabled()) {
-    MozStackWalk(WriteStackFrame, aSkipFrames + 1, aMaxFrames, (void*)aWriter);
+    MozStackWalk(WriteStackFrame, aFirstFramePC ? aFirstFramePC : CallerPC(),
+                 aMaxFrames, (void*)aWriter);
   }
 }

mozglue/misc/StackWalk.h

@@ -15,6 +15,24 @@
 
 MOZ_BEGIN_EXTERN_C
 
+/**
+ * Returns the position of the Program Counter for the caller of the current
+ * function. This is meant to be used to feed the aFirstFramePC argument to
+ * MozStackWalk or MozWalkTheStack*, and should be used in the last function
+ * that should be skipped in the trace, and passed down to MozStackWalk or
+ * MozWalkTheStack*, through any intermediaries.
+ *
+ * THIS DOES NOT 100% RELIABLY GIVE THE CALLER PC, but marking functions
+ * calling this macro with MOZ_NEVER_INLINE gets us close. In cases it doesn't
+ * give the caller's PC, it may give the caller of the caller, or its caller,
+ * etc. depending on tail call optimization.
+ *
+ * Past versions of stackwalking relied on passing a constant number of frames
+ * to skip to MozStackWalk or MozWalkTheStack, which fell short in more cases
+ * (inlining of intermediaries, tail call optimization).
+ */
+#define CallerPC() __builtin_extract_return_addr(__builtin_return_address(0))
+
 /**
  * The callback for MozStackWalk and MozStackWalkThread.
  *
@@ -34,18 +52,20 @@ typedef void (*MozWalkStackCallback)(uint32_t aFrameNumber, void* aPC,
  * Call aCallback for each stack frame on the current thread, from
  * the caller of MozStackWalk to main (or above).
  *
- * @param aCallback    Callback function, called once per frame.
- * @param aSkipFrames  Number of initial frames to skip.  0 means that
- *                     the first callback will be for the caller of
- *                     MozStackWalk.
- * @param aMaxFrames   Maximum number of frames to trace.  0 means no limit.
- * @param aClosure     Caller-supplied data passed through to aCallback.
+ * @param aCallback     Callback function, called once per frame.
+ * @param aFirstFramePC Position of the Program Counter where the trace
+ *                      starts from. All frames seen before reaching that
+ *                      address are skipped. Nullptr means that the first
+ *                      callback will be for the caller of MozStackWalk.
+ * @param aMaxFrames    Maximum number of frames to trace.  0 means no limit.
+ * @param aClosure      Caller-supplied data passed through to aCallback.
  *
  * May skip some stack frames due to compiler optimizations or code
  * generation.
  */
-MFBT_API void MozStackWalk(MozWalkStackCallback aCallback, uint32_t aSkipFrames,
-                           uint32_t aMaxFrames, void* aClosure);
+MFBT_API void MozStackWalk(MozWalkStackCallback aCallback,
+                           const void* aFirstFramePC, uint32_t aMaxFrames,
+                           void* aClosure);
 
 typedef struct {
   /*
@@ -147,29 +167,32 @@ MFBT_API int MozFormatCodeAddressDetails(char* aBuffer, uint32_t aBufferSize,
 /**
  * Walk the stack and print the stack trace to the given stream.
  *
- * @param aStream      A stdio stream.
- * @param aSkipFrames  Number of initial frames to skip.  0 means that
- *                     the first callback will be for the caller of
- *                     MozWalkTheStack.
- * @param aMaxFrames   Maximum number of frames to trace.  0 means no limit.
+ * @param aStream       A stdio stream.
+ * @param aFirstFramePC Position of the Program Counter where the trace
+ *                      starts from. All frames seen before reaching that
+ *                      address are skipped. Nullptr means that the first
+ *                      callback will be for the caller of MozWalkTheStack.
+ * @param aMaxFrames    Maximum number of frames to trace.  0 means no limit.
  */
 MFBT_API void MozWalkTheStack(FILE* aStream,
-                              uint32_t aSkipFrames FRAMES_DEFAULT,
+                              const void* aFirstFramePC FRAMES_DEFAULT,
                               uint32_t aMaxFrames FRAMES_DEFAULT);
 
 /**
  * Walk the stack and send each stack trace line to a callback writer.
  * Each line string is null terminated but doesn't contain a '\n' character.
  *
- * @param aWriter      The callback.
- * @param aSkipFrames  Number of initial frames to skip.  0 means that
- *                     the first callback will be for the caller of
- *                     MozWalkTheStack.
- * @param aMaxFrames   Maximum number of frames to trace.  0 means no limit.
+ * @param aWriter       The callback.
+ * @param aFirstFramePC Position of the Program Counter where the trace
+ *                      starts from. All frames seen before reaching that
+ *                      address are skipped. Nullptr means that the first
+ *                      callback will be for the caller of
+ * MozWalkTheStackWithWriter.
+ * @param aMaxFrames    Maximum number of frames to trace.  0 means no limit.
  */
-MFBT_API void MozWalkTheStackWithWriter(void (*aWriter)(const char*),
-                                        uint32_t aSkipFrames FRAMES_DEFAULT,
-                                        uint32_t aMaxFrames FRAMES_DEFAULT);
+MFBT_API void MozWalkTheStackWithWriter(
+    void (*aWriter)(const char*), const void* aFirstFramePC FRAMES_DEFAULT,
+    uint32_t aMaxFrames FRAMES_DEFAULT);
 
 #undef FRAMES_DEFAULT