1063455 - Consider creating a define for MOZ_STACKWALKING in configure.in

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Bob Owen (:bobowen)]

I had to add some #ifs around the use of NS_StackWalk for bug 1018966 and I noticed that the logic (which I copied :) ) that decides if nsStackTrace.cpp is built, is used in a couple of places.

I think it makes sense to define something like MOZ_STACKWALKING to reproduce the logic at [1].
This could then be used there and in the other places.
It may even be useful for the other places that use NS_StackWalk even though they seem to have different logic protecting them at the moment. 

[1] http://dxr.mozilla.org/mozilla-central/source/xpcom/base/moz.build#125

Comment 1

[Bob Owen (:bobowen)]

Ben Smedberg didn't have a strong opinion either way on this.

Do you think it is worth doing?

Comment 2

[Nathan Froyd [:froydnj]]

I am a fan of doing things Once And Only Once, yes. :)

Comment 3

[Bob Owen (:bobowen)]

https://tbpl.mozilla.org/?tree=Try&rev=b9f8938ea7b4

Comment 4

[Bob Owen (:bobowen)]

https://tbpl.mozilla.org/?tree=Try&rev=1759dbca9add

Comment 5

[Bob Owen (:bobowen)]

Attachment: Define MOZ_STACKWALKING when NS_StackWalk is available and replace other instances of the same #if logic v1

This should make it easier for any new consumers of NS_StackWalk.

Comment 6

[Andrew McCreight [:mccr8]]

This should probably also get build peer review.

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8485837 [details] [diff] [review]
Define MOZ_STACKWALKING when NS_StackWalk is available and replace other instances of the same #if logic v1

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

This is good stuff, but the configure.in checks need a build peer's review.

Comment 8

[Michael Shal [:mshal]]

Comment on attachment 8485837 [details] [diff] [review]
Define MOZ_STACKWALKING when NS_StackWalk is available and replace other instances of the same #if logic v1

>diff --git a/xpcom/build/LateWriteChecks.cpp b/xpcom/build/LateWriteChecks.cpp
>--- a/xpcom/build/LateWriteChecks.cpp
>+++ b/xpcom/build/LateWriteChecks.cpp
>@@ -29,17 +29,17 @@
> #include <sys/stat.h>
> #include <windows.h>
> #else
> #define NS_SLASH "/"
> #endif
> 
> #include "LateWriteChecks.h"
> 
>-#if !defined(XP_WIN) || (!defined(MOZ_OPTIMIZE) || defined(MOZ_PROFILING) || defined(DEBUG))
>+#if defined(MOZ_STACKWALKING)

You could just #ifdef if you want.

Any idea if this #if is supposed to be the same as MOZ_STACKWALKING? (Or maybe !MOZ_STACKWALKING?) - http://mxr.mozilla.org/mozilla-central/source/accessible/base/Logging.cpp#70

It originally came from bug 733848 - based on the review comments there it sounds like folks weren't sure of the correct logic, maybe because we didn't have a consistent token for it :)

Comment 9

[Bob Owen (:bobowen)]

https://tbpl.mozilla.org/?tree=Try&rev=73fb9c843c56

Comment 10

[Bob Owen (:bobowen)]

Thanks both.
Just kicked off another try run with a few actual tests running.

(In reply to Michael Shal [:mshal] from comment #8)

> Any idea if this #if is supposed to be the same as MOZ_STACKWALKING? (Or
> maybe !MOZ_STACKWALKING?) -
> http://mxr.mozilla.org/mozilla-central/source/accessible/base/Logging.cpp#70
> 
> It originally came from bug 733848 - based on the review comments there it
> sounds like folks weren't sure of the correct logic, maybe because we didn't
> have a consistent token for it :)

Yeah, I thought about changing this to !MOZ_STACKWLAKING, but as this would turn the logging on unconditionally for non Windows builds, I decided against it.

I've just raised bug 1065296, so there's more of a chance that this and the other uses of NS_StackWalk will get looked at. :)

Comment 11

[Bob Owen (:bobowen)]

Try push building on all platforms:
https://tbpl.mozilla.org/?tree=Try&rev=1759dbca9add

Even thought this is basically a build config change, a try push with some tests to be safe:
https://tbpl.mozilla.org/?tree=Try&rev=73fb9c843c56

Thanks.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ff268a72378

FYI, for future reference, you can consolidate those two Try pushes into one by making use of the "Restrict tests to platform(s)" option on Trychooser :)

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0ff268a72378