205023 - nsTraceRefcnt::LogAddCOMPtr and LogReleaseCOMPtr missing in debug buld

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Colin Blake]

Similar to bug 205013, I have more missing symbols from my debug build. Again,
this is not new, I'm just trying to fix them now.

nsTraceRefcnt::LogAddCOMPtr and nsTraceRefcnt::LogReleaseCOMPtr are referenced
in the link of regxpcom and mozilla-bin, but neither are present. Where are they
meant to be?

Comment 1

[Colin Blake]

nsTraceRefcnt::LogAddCOMPtr and nsTraceRefcnt::LogReleaseCOMPtr do not exist in
libxpcomglue, and that's where the link of regxpcom and mozilla-bin are looking
for them.

Who is referencing these in a debug build? Both are referenced by:
 void nsCOMPtr<nsIServiceManager >::assign_from_helper(const nsCOMPtr_helper &,
const nsID &)
 void **nsCOMPtr<nsIServiceManager >::begin_assignment()

Brendan, as a designated XPCOM expert, should these be in libxpcomglue, or is
something else amiss?

(ignore my earlier comment, I think this is a new bug for mozilla-bin,
introduced when GRE_BUILD was added).

Comment 2

[Brendan Eich [:brendan]]

This looks like something for dougt.

Why isn't anyone seeing this outside of OpenVMS?  Dbaron said it sounded like
what you get when you mix debug and non-debug libraries in a build.

/be

Comment 3

[Doug Turner (:dougt)]

Hi Colin,
See bug:  http://bugzilla.mozilla.org/show_bug.cgi?id=151072

Comment 4

[Colin Blake]

> Why isn't anyone seeing this outside of OpenVMS?

It could be something that's going wrong on the OpenVMS build, that's for sure.
But I've looked carefully at a link map (shows exactly what getting pulled in by
the linker) and everything is coming from the debug tree. No opt in sight. I
guess its possible that something is the debug tree is getting built without
some necessary debug flag. Is the only important one -DDEBUG?

> Dbaron said it sounded like what you get when you mix debug and non-debug
> libraries in a build.

I'll keep looking, but like I said above...

Comment 5

[Colin Blake]

> See bug:  http://bugzilla.mozilla.org/show_bug.cgi?id=151072

What am I looking at exactly, there? That bug was fixed nine months ago.

Comment 6

[Colin Blake]

Dbaron was right. Give that man a cigar.

At some point when I had been trying to fix bug 205013 (another xpcom debug
build bug) I must have accidently built some of the glue modules without the
-DDEBUG stuff, and that's what was causing this problem. And because it was a
compile mistake and not a link mistake, that's why I couldn't see anything amiss
in the map.

Sorry for the false alarms.

Comment 7

[Colin Blake]

I do still have a link problem. I wasn't losing it before. Here's the problem.

I kick off a full debug build. xpcom/tools/registry fails to link regxpcom
because nsTraceRefcnt::LogAddCOMPtr and nsTraceRefcnt::LogReleaseCOMPtr are
missing. These are referenced from some template code (can't tell what
referenced those templates because all generated template code gets bundled into
the same location).

Anyway, I found that if I go back to xpcom/glue and rebuild then, I can then
link regxpcom without a problem and the rest of the build continues of its merry
way.

Until mozilla-bin. That fails with the same missing symbols. The building
xpcom/glue trick doesn't work here, so some other directory is generating the
"bad" template code.

This is where we are in Mozilla when we hit the "missing code":

#ifdef NSCAP_FEATURE_DEBUG_PTR_TYPES
    private:
      void    assign_with_AddRef( nsISupports* );
      void    assign_from_helper( const nsCOMPtr_helper&, const nsIID& );
      void**  begin_assignment();

      void
      assign_assuming_AddRef( T* newPtr )
        {
          T* oldPtr = mRawPtr;
          mRawPtr = newPtr;
-->       NSCAP_LOG_ASSIGNMENT(this, newPtr);
          NSCAP_LOG_RELEASE(this, oldPtr);
          if ( oldPtr )
            NSCAP_RELEASE(this, oldPtr);
        }

And here's the calls which got us there:

DBG> sho call
*CXX$assgnfrmhlpr30nsCMPt168efgo 
                assign_assuming_AddRef 
*CXX$assgnfrmhlpr30nsCMPt168efgo 
                assign_from_helper 
*NSXPCOMGLUE    nsCOMPtr(const nsQueryInterface &) 
*NSXPCOMGLUE    Assert_NoQueryNeeded 
*NSXPCOMGLUE    ~nsGetterAddRefs 
*NSXPCOMGLUE    GRE_Startup     
*NSAPPRUNNER    main            
*NSAPPRUNNER    __MAIN          
*SHARE$PTHREAD$RTL              
*SHARE$PTHREAD$RTL              
DBG> 

Can someone tell me what's going on!!!

Comment 8

[Colin Blake]

Because XPCOM_GLUE is defined in xpcom/tools/registry's Makefile, this causes
NS_BUILD_REFCNT_LOGGING to get UNdefined:
http://lxr.mozilla.org/seamonkey/source/xpcom/base/nsTraceRefcnt.h#73

This means the LogAddRef etc are NOT referenced any more, and so (I think this
is what's happening) the code for LogAddRef and friends is NOT created in the
template code BUT when regxpcom is linked we suck in some other code which DOES
expect LogAddRef and friends to exist in the template code.

I suspect I could work around this by performing my debug builds with
--disable-logrefcnt, but I'd rather fix the root problem if possible.

Comment 9

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

I'd think the right solution would be what I suggested in bug 193585 comment 12
-- which really applies to nsTraceRefcnt and nsDebug.

Comment 10

[Colin Blake]

Marking "depends on 193585" (not 100% its a dupe at this point).

Comment 11

[Doug Turner (:dougt)]

Attachment: patch v.1

similar to what we just did for nsDebug.  This patch is missing some changes to
areas outside of mozilla/xpcom.  Those changes are basically,
s/nsTraceRefcnt/nsTraceRefcntImpl.

commments welcom.

Comment 12

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

I don't see why this should block 1.5alpha.  We've shipped with this before.  Or
am I missing something?

Comment 13

[Doug Turner (:dougt)]

Comment on attachment 127378 [details] [diff] [review]
patch v.1

dbaron, can you review today?

Comment 14

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

Comment on attachment 127378 [details] [diff] [review]
patch v.1


It would be good to at least do a repository copy of nsTraceRefcnt.{h,cpp} to
nsTraceRefcntImpl{h,cpp} before landing (and maybe to the new nsTraceRefcnt.h
location as well).

>Index: build/dlldeps.cpp

I don't understand why you changed exactly what you did, but then again, I
don't really understand this file at all...

>Index: build/nsXPComInit.cpp
>+nsresult NS_COM NS_GetTraceRefcnt(nsITraceRefcnt** result)
>+{
>+#ifdef NS_BUILD_REFCNT_LOGGING
>+    nsresult rv = NS_OK;
>+    if (!gTraceRefcnt)
>+    {
>+        rv = nsTraceRefcntImpl::Create(nsnull, 
>+                                       NS_GET_IID(nsITraceRefcnt), 
>+                                       (void**)&gTraceRefcnt);
>+    }
>+    NS_IF_ADDREF(*result = gTraceRefcnt);
>+    return rv;
>+#else
>+    return NS_ERROR_NOT_INITIALIZED;

Maybe NS_ERROR_NOT_AVAILABLE would make more sense?

>+#endif
>+}

>Index: glue/nsTraceRefcnt.cpp
>+#define ENSURE_TRACEOBJECT \

Did you mean to define this to be something?  (Something needs to call
SetupTraceRefcntObject for the non-glue case, if that code ever gets used
(which I think it does on some platforms), but you don't want to do it multiple
times.)

I'll give r=dbaron once I see how you address this last issue.

Comment 15

[Doug Turner (:dougt)]

dbaron, not sure what happened with the patch, but ENSURE_TRACEOBJECT is defined as:

  (gTraceRefcntObject ? PR_TRUE : (PRBool)(SetupTraceRefcntObject() != nsnull))


This is similar to what we do for the nsMemory case.  I am going to rediff...
stay tuned.

Comment 16

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

> (gTraceRefcntObject ? PR_TRUE : (PRBool)(SetupTraceRefcntObject() != nsnull))

That would be written much more cleanly as:

(gTraceRefcntObject || SetupTraceRefcntObject() != nsnull)

With that and my other comments addressed, r=dbaron.

Comment 17

[Asa Dotzler [:asa]]

It looks like this landed on 08/05. If so, can we resolve this bug?

Comment 18

[Doug Turner (:dougt)]

yup.