66159 - comment out nsTraceRefcnt in optimized builds

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect, P1)

Description

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

As Brendan said in bug 61243, we should make class nsTraceRefcnt completely
#ifdef NS_BUILD_REFCNT_LOGGING (which is defined by default in debug builds
but not optimized builds).  I propose also that we pull WalkTheStack out of
nsTraceRefcnt and make it it's own global function, depending on a different
#ifdef (since I don't think it's #ifdef-ed at all now, and there is at least
one other user).  Perhaps it should be renamed to NS_WalkTheStack and within
|#if defined(DEBUG) || defined(NS_BUILD_REFCNT_LOGGING)|?

Comment 1

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

And I meant to assign this to myself...

Comment 2

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

Attachment: mostly completed patch (hand-edited diff that won't apply)

Comment 3

[Brendan Eich [:brendan]]

Cool -- how do you want to land this?

Once it's in, I'll try to unify nsTraceMalloc's stack and symbol stuff with it.

/be

Comment 4

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

Attachment: patch for this bug and bug 14989 and bug 63665 (ignore makefile changes removing nslogging stuff)

Comment 5

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

Attachment: The real changes in the large chunk that was moved from nsTraceRefcnt.cpp to nsStackWalker.cpp in the above patch

Comment 6

[Chris Waterson]

Looks ok. Why not make LoadLibrarySymbols and DemangleSymbol accessable from C,
too? (e.g., I wish that the Boehm GC would dump demangled symbols...)

r=waterson

Comment 7

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

LoadLibrarySymbols is called from the function that loads the DLL, i.e., the
code in xcDll.cpp.  DemangleSymbol might be nice from the boehm GC, except,
at least on Linux, it leaks, which leads to exponential growth of the leak
report if you do real-time symbol demangling. :-(

I thought about making them accessible from C, too, but I didn't see how they
would be useful (see above).  Do you think I should anyway?  (If so, can you
think of good names for them?)

Comment 8

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

Is anybody interested in testing this on Windows/Mac?  I'd normally be
comfortable landing something like this on a quiet Sunday morning, but I'm
not going to be able to find Mac help then...  Or do you think it's safe
enough?

Comment 9

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

Since I really need to get the stuff in bug 66159 tested on Windows and Mac,
moving this to mozilla 0.9.1

Comment 10

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

Pushing out again.  I'll try to land this early in 0.9.2.  Really.  (I should
have time this time around.)

Comment 11

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

Attachment: patch, brought up to date

Comment 12

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

What I discovered (a month or two ago) was wrong with this is that it didn't
account for the windows library loading functions properly.  Probably more of
that would need to be in nsStackWalker, or something.

Comment 13

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

This should probably be done before 1.0 for binary compatibility, but I'm not
going to be able to do it.

Comment 14

[Doug Turner (:dougt)]

I do not think that there is a problem with binary compatiblity if one builds
with optimized and does not set FORCE_BUILD_REFCNT_LOGGING.  Is this a correct
observation?

Comment 15

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

That's correct, but we might in the future want to be able to change the
signatures of methods in nsTraceRefcnt, and right now, we can't (for any clients
that link to xpcom -- although I really think they shouldn't be).

Comment 16

[Doug Turner (:dougt)]

Sure we can change the symbol.  I agree with you... clients that link against
symbols that are not frozen shouldn't.

Comment 17

[Benjamin Smedberg]

we now provide these symbols in release builds as we should, I think (although the stacks are not always so useful)