Some MozStackWalk() and FramePointerStackWalk() tweaks

RESOLVED FIXED in Firefox 57

Status

()

Core
mozglue
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: njn, Assigned: njn)

Tracking

54 Branch
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

a year ago
I have some tweaks for MozStackWalk() and FramePointerStackWalk().
(Assignee)

Comment 1

a year ago
Created attachment 8890710 [details] [diff] [review]
(part 1) - Split MozStackWalk()

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
aPlatformData).

The patch also removes unnecessary reinterpret_casts for the aClosure argument
at a couple of MozStackWalk() callsites.
Attachment #8890710 - Flags: review?(mh+mozilla)
(Assignee)

Updated

a year ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

a year ago
Created attachment 8890711 [details] [diff] [review]
(part 2) - Tweak FramePointerStackWalk() arguments

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)
(Assignee)

Comment 3

a year ago
Created attachment 8890712 [details] [diff] [review]
(part 3) - Remove the return value from the stack walker functions

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>
> +
> +#define MOZ_STACKWALK_SUPPORTS_WINDOWS 1

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
> -  //   https://bugzilla.mozilla.org/show_bug.cgi?id=717853#c110.
> -  // - 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+
(Assignee)

Comment 7

a year ago
> > +#define MOZ_STACKWALK_SUPPORTS_WINDOWS 1
> 
> 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.

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ee28d0227a1c
https://hg.mozilla.org/mozilla-central/rev/c434e487e804
https://hg.mozilla.org/mozilla-central/rev/a8fd71353cba
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.