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

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.

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

(In reply to Eric Rahm [:erahm] from comment #8) Done and done, thanks for the heads up.

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

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

(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

(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

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