Closed Bug 1384819 Opened 2 years ago Closed 2 years ago

Some MozStackWalk() and FramePointerStackWalk() tweaks


(Core :: mozglue, enhancement)

54 Branch
Not set



Tracking Status
firefox57 --- fixed


(Reporter: njn, Assigned: njn)



(3 files)

I have some tweaks for MozStackWalk() and FramePointerStackWalk().
MozStackWalk() is different on Windows to the other platforms. It has two extra
arguments, which can be used to walk the stack of a different thread.

This patch makes those differences clearer. Instead of having a single function
and forbidding those two arguments on non-Windows, it removes those arguments
from MozStackWalk, and splits off MozStackWalkThread() which retains them. This
also allows those arguments to have more appropriate types (HANDLE instead of
uintptr_t; CONTEXT* instead of than void*) and names (aContext instead of

The patch also removes unnecessary reinterpret_casts for the aClosure argument
at a couple of MozStackWalk() callsites.
Attachment #8890710 - Flags: review?(mh+mozilla)
Assignee: nobody → n.nethercote
This patch does he following.

- Avoids some unnecessary casting.

- Renames the |bp| parameter as |aBp|.

- Makes the no-op FramePointerStackWalk() signature match the real one.
  (Clearly it's dead code in all built configurations!)
Attachment #8890711 - Flags: review?(mh+mozilla)
Just one caller (in DMD) actually looks at it, and that's in an unimportant way
-- if the return value was false, mLength would be zero anyway.
Attachment #8890712 - Flags: review?(mh+mozilla)
Comment on attachment 8890710 [details] [diff] [review]
(part 1) - Split MozStackWalk()

Review of attachment 8890710 [details] [diff] [review]:

::: mozglue/misc/StackWalk.h
@@ +52,5 @@
> +    (defined(_M_IX86) || defined(_M_AMD64) || defined(_M_IA64))
> +
> +#include <windows.h>
> +

this macro name feels a bit weird, but I don't have a better idea.
Attachment #8890710 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8890711 [details] [diff] [review]
(part 2) - Tweak FramePointerStackWalk() arguments

Review of attachment 8890711 [details] [diff] [review]:

::: memory/replace/dmd/DMD.cpp
@@ +808,5 @@
>      int skipFrames = 1;
>  #else
>      int skipFrames = 2;
>  #endif
> +    bool ok = MozStackWalk(StackWalkCallback, skipFrames, MaxFrames, &tmp);

This change should be in the first patch, right?
Attachment #8890711 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8890712 [details] [diff] [review]
(part 3) - Remove the return value from the stack walker functions

Review of attachment 8890712 [details] [diff] [review]:

::: mozglue/misc/StackWalk.cpp
@@ -1096,5 @@
> -  //   _URC_FAILURE.  See
> -  //
> -  // - If aMaxFrames != 0, we want to stop early, and the only way to do that
> -  //   is to make unwind_callback return something other than _URC_NO_REASON,
> -  //   which causes _Unwind_Backtrace to return a non-success code.

This comment still seems relevant. Although moving it above the _Unwind_Backtrace call would seem adequate.
Attachment #8890712 - Flags: review?(mh+mozilla) → review+
> this macro name feels a bit weird, but I don't have a better idea.

I based it on the existing MOZ_STACKWALK_SUPPORTS_MACOSX and MOZ_STACKWALK_SUPPORTS_LINUX macros in StackWalk.cpp.
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.