1172216 - Move nsStackwalk to mozglue

Status: RESOLVED FIXED

(Core :: mozglue, defect)

Comment 1

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 2

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 3

[Mike Hommey [:glandium]]

To me, this goes well beyond what's supposed to be in mfbt. It should go in mozglue instead.

Comment 4

[Benoit Girard (:BenWa)]

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

How about moving this into tools/profiler then?

Comment 5

[Mike Hommey [:glandium]]

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.

Comment 6

[Benoit Girard (:BenWa)]

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/*.

Comment 7

[Benoit Girard (:BenWa)]

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

[Eric Rahm [:erahm]]

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.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think the primary C user was nsTraceMalloc, which has been removed.

Comment 10

[Benoit Girard (:BenWa)]

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

Done and done, thanks for the heads up.

Comment 11

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 12

[Benoit Girard (:BenWa)]

Attachment: patch v2

Comment 13

[Mike Hommey [:glandium]]

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.

Comment 14

[Nicholas Nethercote [inactive]]

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.

Comment 15

[Benoit Girard (:BenWa)]

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

Comment 16

[Benoit Girard (:BenWa)]

Attachment: patch v3

Comment 17

[Mike Hommey [:glandium]]

(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

[Eric Rahm [:erahm]]

(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

Comment 19

[Benoit Girard (:BenWa)]

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

Comment 20

[Mike Hommey [:glandium]]

> It *had* a prefix, it was NS_ (except FramePointerStackWalk but that's to be consistent).

I was precisely talking about FramePointerStackWalk.

Comment 21

[Mike Hommey [:glandium]]

Also note the patch was not r-ed over this.

Comment 22

[Mike Hommey [:glandium]]

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.

Comment 23

[Benoit Girard (:BenWa)]

Attachment: patch v4 r=glandium

Comment 24

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8c58dd8dabc

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d8c58dd8dabc