1384819 - Some MozStackWalk() and FramePointerStackWalk() tweaks

Status: RESOLVED FIXED

(Core :: mozglue, enhancement)

Description

[Nicholas Nethercote [inactive]]

I have some tweaks for MozStackWalk() and FramePointerStackWalk().

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (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.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (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!)

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (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.

Comment 4

[Mike Hommey [:glandium]]

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.

Comment 5

[Mike Hommey [:glandium]]

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?

Comment 6

[Mike Hommey [:glandium]]

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.

Comment 7

[Nicholas Nethercote [inactive]]

> > +#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 8

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee28d0227a1c5a97962f115140f0794c5f82e126
Bug 1384819 (part 1) - Split MozStackWalk(). r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c434e487e80496f02fdb1522f06a7d8ab0f4ce44
Bug 1384819 (part 2) - Tweak FramePointerStackWalk() arguments. r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8fd71353cbac3d71b8c1a9c7572b5d10d16c9c2
Bug 1384819 (part 3) - Remove the return value from the stack walker functions. r=glandium.

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ee28d0227a1c
https://hg.mozilla.org/mozilla-central/rev/c434e487e804
https://hg.mozilla.org/mozilla-central/rev/a8fd71353cba