Move nsStackwalk to mozglue

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8616348 [details] [diff] [review]
patch
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
Blocks: 1172186
(Assignee)

Comment 2

3 years ago
Created attachment 8616955 [details] [diff] [review]
patch
Attachment #8616348 - Attachment is obsolete: true
Attachment #8616955 - Flags: review?(Ms2ger)
To me, this goes well beyond what's supposed to be in mfbt. It should go in mozglue instead.
(Assignee)

Comment 4

3 years ago
I don't want the profiler to depend on mozglue however.

How about moving this into tools/profiler then?
You don't have to depend on mozglue as a whole, even if it's in mozglue. In fact, mfbt *is* in mozglue, yet, you can depend on mfbt without depending on mozglue. The same can be done with other subparts of mozglue, as long as they are independent.
(Assignee)

Comment 6

3 years ago
Yea I didn't mean the library itself. Just what people's expectation are. If the code is in mfbt/* I wont try to include things from mozglue/*.
(Assignee)

Comment 7

3 years ago
Comment on attachment 8616955 [details] [diff] [review]
patch

Review of attachment 8616955 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/base/nsStackWalk.cpp
@@ -184,5 @@
> -#endif
> -
> -#if defined(_WIN32) && (defined(_M_IX86) || defined(_M_AMD64) || defined(_M_IA64)) // WIN32 x86 stack walking code
> -
> -#include "nscore.h"

Note to self: remove all nscore.h

Comment 8

3 years ago
Comment on attachment 8616955 [details] [diff] [review]
patch

Review of attachment 8616955 [details] [diff] [review]:
-----------------------------------------------------------------

Note: you'll need to update DMD's moz.build as well (this patch doesn't compile w/ DMD enabled).

::: mfbt/StackWalk.h
@@ +14,5 @@
> +#include <stdint.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif

I wonder if we can ditch the extern C and put all of this in the mozilla namespace. It doesn't look like we're using StackWalking in C code, but maybe I'm missing something.
I think the primary C user was nsTraceMalloc, which has been removed.

Updated

3 years ago
Blocks: 1173017
(Assignee)

Comment 10

3 years ago
(In reply to Eric Rahm [:erahm] from comment #8)

Done and done, thanks for the heads up.
(Assignee)

Comment 11

3 years ago
Created attachment 8617786 [details] [diff] [review]
patch
Attachment #8616955 - Attachment is obsolete: true
Attachment #8616955 - Flags: review?(Ms2ger)
Attachment #8617786 - Flags: review?(mh+mozilla)
(Assignee)

Updated

3 years ago
Summary: Move nsStackwalk to MFBT → Move nsStackwalk to mozglue
(Assignee)

Updated

3 years ago
Component: MFBT → mozglue
(Assignee)

Comment 12

3 years ago
Created attachment 8621277 [details] [diff] [review]
patch v2
Attachment #8617786 - Attachment is obsolete: true
Attachment #8617786 - Flags: review?(mh+mozilla)
Attachment #8621277 - Flags: review?(mh+mozilla)
Comment on attachment 8621277 [details] [diff] [review]
patch v2

Review of attachment 8621277 [details] [diff] [review]:
-----------------------------------------------------------------

::: memory/replace/dmd/DMD.cpp
@@ +746,5 @@
>    StackTrace tmp;
>    {
>      AutoUnlockState unlock;
>      uint32_t skipFrames = 2;
> +    rv = MozStackWalk(StackWalkCallback, skipFrames,

Might as well not store the result in a variable.

::: mozglue/misc/StackWalk.cpp
@@ +901,5 @@
>      void* pc = *(bp + 1);
>      bp += 2;
>  #endif
>      if (IsCriticalAddress(pc)) {
> +      return false;

Nicholas, since DMD is apparently the only one looking for this value, what do you think of removing the distinction with other failure cases here?

@@ +1020,5 @@
>    // - 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.
>    if (info.isCriticalAbort) {
> +    return false;

same here.

::: mozglue/misc/StackWalk.h
@@ +148,5 @@
>  
> +namespace mozilla {
> +
> +MFBT_API bool
> +MozFramePointerStackWalk(MozWalkStackCallback aCallback, uint32_t aSkipFrames,

Moz prefix for something in the mozilla namespace is overkill, in fact, since there aren't any C callers left, just put it all in the mozilla namespace, and remove the prefixes.

::: mozglue/misc/TimeStamp.cpp
@@ +8,5 @@
>   * Implementation of the OS-independent methods of the TimeStamp class
>   */
>  
>  #include "mozilla/TimeStamp.h"
> +#include <stdio.h>

Seems like the changes to this file are unrelated and meant to go to the other bug.
Attachment #8621277 - Flags: review?(n.nethercote)
Attachment #8621277 - Flags: review?(mh+mozilla)
Attachment #8621277 - Flags: feedback+
Comment on attachment 8621277 [details] [diff] [review]
patch v2

Review of attachment 8621277 [details] [diff] [review]:
-----------------------------------------------------------------

> Nicholas, since DMD is apparently the only one looking for this value, what do you think
> of removing the distinction with other failure cases here?

That's ok. The NS_ERROR_UNEXPECTED case in DMD.cpp can be merged with the NS_ERROR_{NOT_IMPLEMENTED,ERROR_FAILURE} case.
Attachment #8621277 - Flags: review?(n.nethercote)
(Assignee)

Comment 15

3 years ago
(In reply to Mike Hommey [:glandium] from comment #13)
> ::: mozglue/misc/StackWalk.h
> @@ +148,5 @@
> >  
> > +namespace mozilla {
> > +
> > +MFBT_API bool
> > +MozFramePointerStackWalk(MozWalkStackCallback aCallback, uint32_t aSkipFrames,
> 
> Moz prefix for something in the mozilla namespace is overkill, in fact,
> since there aren't any C callers left, just put it all in the mozilla
> namespace, and remove the prefixes.
> 

Good point, but it's another cleanup and doesn't need to happen part of the move/this bug. I'm out of time on this bug.
(Assignee)

Comment 16

3 years ago
Created attachment 8621628 [details] [diff] [review]
patch v3
Attachment #8621277 - Attachment is obsolete: true
Attachment #8621628 - Flags: review?(mh+mozilla)
(In reply to Benoit Girard (:BenWa) from comment #15)
> (In reply to Mike Hommey [:glandium] from comment #13)
> > ::: mozglue/misc/StackWalk.h
> > @@ +148,5 @@
> > >  
> > > +namespace mozilla {
> > > +
> > > +MFBT_API bool
> > > +MozFramePointerStackWalk(MozWalkStackCallback aCallback, uint32_t aSkipFrames,
> > 
> > Moz prefix for something in the mozilla namespace is overkill, in fact,
> > since there aren't any C callers left, just put it all in the mozilla
> > namespace, and remove the prefixes.
> > 
> 
> Good point, but it's another cleanup and doesn't need to happen part of the
> move/this bug. I'm out of time on this bug.

Except you're also *adding* a Moz prefix to something that didn't have any prefix before

Comment 18

3 years ago
(In reply to Benoit Girard (:BenWa) from comment #15)
> (In reply to Mike Hommey [:glandium] from comment #13)
> > ::: mozglue/misc/StackWalk.h
> > @@ +148,5 @@
> > >  
> > > +namespace mozilla {
> > > +
> > > +MFBT_API bool
> > > +MozFramePointerStackWalk(MozWalkStackCallback aCallback, uint32_t aSkipFrames,
> > 
> > Moz prefix for something in the mozilla namespace is overkill, in fact,
> > since there aren't any C callers left, just put it all in the mozilla
> > namespace, and remove the prefixes.
> > 
> 
> Good point, but it's another cleanup and doesn't need to happen part of the
> move/this bug. I'm out of time on this bug.

Something like the following should do the trick:
> sed -E -e 's/Moz(WalkTheStackCallback|StackWalk|CodeAddressDetails|DescribeCodeAddress|FormatCodeAddress|FormatCodeAddressDetails)/\1/g' .hg/patches/Stackwalk
(Assignee)

Comment 19

3 years ago
(In reply to Mike Hommey [:glandium] from comment #17)
> Except you're also *adding* a Moz prefix to something that didn't have any
> prefix before

It *had* a prefix, it was NS_ (except FramePointerStackWalk but that's to be consistent). This patch isn't making the code worse, it's already making it better. There's no point in delaying this patch over this trivial detail.

I'm not opposing to this change in particular. It's just annoying having over a week long turn around times and then still having to change more trivial stuff for effectively just moving two files in our tree (this and timestamp). I had more ambitious plans to better clean up the profiler but this sort of trivial changes back-and-fort already means that I've already spent all my time to work on non GFX platform.

And as simple as these changes are, tweaking namespaces and whatnot, requires another local build, a try round trip, + another review turn around time. It's not just a regex in practice. So that's pushing things a few calendar days here, another few calendar days there and now we're at 2 calendar weeks.

I'll make the change in good faith but in general I think forcing unrelated clean-ups like this is not pragmatic and prevents more useful work from occurring.
> It *had* a prefix, it was NS_ (except FramePointerStackWalk but that's to be consistent).

I was precisely talking about FramePointerStackWalk.
Also note the patch was not r-ed over this.
Comment on attachment 8621628 [details] [diff] [review]
patch v3

Review of attachment 8621628 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/misc/StackWalk.h
@@ +148,5 @@
>  
> +namespace mozilla {
> +
> +MFBT_API bool
> +MozFramePointerStackWalk(MozWalkStackCallback aCallback, uint32_t aSkipFrames,

Don't rename this one.
Attachment #8621628 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 23

3 years ago
Created attachment 8623804 [details] [diff] [review]
patch v4 r=glandium
Attachment #8621628 - Attachment is obsolete: true
Attachment #8623804 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d8c58dd8dabc
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41

Updated

3 years ago
Blocks: 1179550

Updated

3 years ago
Depends on: 1194890
You need to log in before you can comment on or make changes to this bug.