Consider creating a define for MOZ_STACKWALKING in configure.in

RESOLVED FIXED in mozilla35

Status

()

Core
XPCOM
--
minor
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bobowen, Assigned: bobowen)

Tracking

unspecified
mozilla35
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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
(Assignee)

Comment 1

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

Do you think it is worth doing?
Flags: needinfo?(nfroyd)
I am a fan of doing things Once And Only Once, yes. :)
Flags: needinfo?(nfroyd)
(Assignee)

Comment 3

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=b9f8938ea7b4
(Assignee)

Comment 4

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=1759dbca9add
(Assignee)

Comment 5

3 years ago
Created attachment 8485837 [details] [diff] [review]
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.
Attachment #8485837 - Flags: review?(nfroyd)
(Assignee)

Updated

3 years ago
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
This should probably also get build peer review.
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.
Attachment #8485837 - Flags: review?(nfroyd)
Attachment #8485837 - Flags: review?(mshal)
Attachment #8485837 - Flags: review+
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 :)
Attachment #8485837 - Flags: review?(mshal) → review+
(Assignee)

Comment 9

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=73fb9c843c56
(Assignee)

Updated

3 years ago
Depends on: 1065296
(Assignee)

Comment 10

3 years ago
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. :)
(Assignee)

Comment 11

3 years ago
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.
Keywords: checkin-needed
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 :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0ff268a72378
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.