Closed
Bug 1384819
Opened 7 years ago
Closed 7 years ago
Some MozStackWalk() and FramePointerStackWalk() tweaks
Categories
(Core :: mozglue, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
Details
Attachments
(3 files)
30.03 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
5.95 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
13.51 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
I have some tweaks for MozStackWalk() and FramePointerStackWalk().
Assignee | ||
Comment 1•7 years ago
|
||
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•7 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
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•7 years ago
|
||
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 4•7 years ago
|
||
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 5•7 years ago
|
||
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 6•7 years ago
|
||
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•7 years 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.
Assignee | ||
Comment 8•7 years ago
|
||
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•7 years 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
Closed: 7 years 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.
Description
•