|nsCOMPtr|: is it time to remove the 'don't forward declare T hack'?

RESOLVED FIXED in Future

Status

()

Core
XPCOM
P1
enhancement
RESOLVED FIXED
16 years ago
14 years ago

People

(Reporter: hacker formerly known as seawood@netscape.com, Assigned: John Keiser (jkeiser))

Tracking

Trunk
Future
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [FIX])

Attachments

(3 attachments, 1 obsolete attachment)

I was trying to clean up circular jar requirement from bug 107289 and I ran
across this tidbit in nsCOMPtr.h:

    enum { _force_even_compliant_compilers_to_fail_ = sizeof(T) };
      /*
        The declaration above exists specifically to make |nsCOMPtr<T>| _not_
compile with only
        a forward declaration of |T|.  This should prevent Windows and Mac
engineers from
        breaking Solaris and other compilers that naturally have this behavior.
 Thank
        <law@netscape.com> for inventing this specific trick.

        Of course, if you're using |nsCOMPtr| outside the scope of wanting to
compile on
        Solaris and old GCC, you probably want to remove the enum so you can
exploit forward
        declarations.
      */

What exactly was the "tainting" problem and which compilers did it affect?
I believe the only compiler affected was gcc 2.7.2.3, which we dropped a few
weeks ago.  We could try removing this enum declaration and then putting an
nsCOMPtr with a forward-declaration into the tree and seeing if any tinderboxes
go red...

Comment 2

16 years ago
back to you chris.
Assignee: dougt → seawood
Priority: -- → P2
Target Milestone: --- → mozilla0.9.7
Target Milestone: mozilla0.9.7 → mozilla0.9.9
Target Milestone: mozilla0.9.9 → mozilla1.2
Nevermind.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → WONTFIX
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Why nevermind?  We should fix this.  I'll take it if you want (although I won't
get to it immediately).
Taking.
Assignee: seawood → dbaron
Status: REOPENED → NEW
Priority: P2 → P1
Target Milestone: mozilla1.2alpha → Future
*** Bug 173154 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 7

15 years ago
Created attachment 102112 [details] [diff] [review]
Patch

This patch includes both nsCOMPtr<> and a short testcase for it in dlldeps.cpp
(a fictional class)
Comment on attachment 102112 [details] [diff] [review]
Patch

dlldeps.cpp is not built cross-platform.  Find a real test that's actually part
of the build, and that you've tested fails without the nsCOMPtr.h changes.
(Assignee)

Comment 9

15 years ago
Created attachment 102113 [details] [diff] [review]
Patch #2

This one changes a file that is compiled on all platforms and which I have
tested does *not* work before and *does* work after the change.
Attachment #102112 - Attachment is obsolete: true
Comment on attachment 102113 [details] [diff] [review]
Patch #2

r/sr=dbaron
Attachment #102113 - Flags: superreview+
Comment on attachment 102113 [details] [diff] [review]
Patch #2

r=sicking assuming you've talked with scc about this and he's for it
Attachment #102113 - Flags: review+
(Assignee)

Comment 12

15 years ago
Fix checked in.  Let's see what she does to ports, arrr.
Assignee: dbaron → jkeiser
(Assignee)

Updated

15 years ago
Status: NEW → ASSIGNED
Whiteboard: [FIX]
(Assignee)

Comment 13

15 years ago
*sigh* OS/2 failed.  Will back out once BeOS picks up the change so we can see
what happens with that platform..

mkaply, what can we do about this?  IBM stopped supporting the OS/2 compiler a
year and a half ago (
http://www1.ibmlink.ibm.com/cgi-bin/master?xh=Fi7dVYO8vOv$7X1USenGnN9332&request=announcements&parms=H%5f901%2d013&xhi=usa%2emain&xfr=N
).  Are there any other options?

bbaetz also points out that this means we will need the hack in nsCOMArray.
(Assignee)

Comment 14

15 years ago
Fix backed out.  This is a serious issue.  As we start to use more and more
templates in our code, backwards platforms are going to hinder progress.
alecf: will we need this hack in nsCOMArray? I'm guessing yes, but I guess it
depends on exactly what part of this construct os2 was breaking on.
Oh, and one other thing I mentioned to jkeiser - when we do enable this again,
we should have an autoconf test which errors out if this won't compile, just to
make the requirements explicit/easier to find/etc.

Comment 17

15 years ago
we might, actually. though in nsCOMArray's case, there is no member variable
that depends on T, the only member variable is a nsVoidArray.

Comment 18

15 years ago
Can someone give me a simple non Mozilla testcase for this?

Was this something that became allowed in later C++ standards?

And yes our compiler does suck. But there is not much I can do about it.

If what we are seeing is a genuine bug, I can still try to get it fixed.

We are looking into other compilers, but are stuck with this one for now.
(Assignee)

Comment 19

15 years ago
Created attachment 102201 [details]
Testcase

I *believe* this is a minimized testcase that will fail.  The essential deal
is, class A { TemplatizedClass<B> b; }; declaration should work until such time
as the constructor / destructor actually have to be called.  I guess the OS/2
compiler is compiling these things early.  I'm a little surprised so many
compilers do this, in fact :)

mkaply, can you give this one a try?

And yes, OS/2 is the only build that failed to compile ...

Comment 20

15 years ago
Here is the statement from our compiler folks:

According to the development this works as designed. The development have
returned this defect claim with explanation that IBM C++ compilers 3.6 used a
template instantiation method where all member functions of the class were
always instantiated. Unlike the newer, ISO-14882 compliant compilers, the 3.6
compiler doesn't use the rule of only instantiating the referenced member functions.

So, if we go this way, the OS/2 port of Mozilla will break until we can switch
to a new compiler - we have no timeframe for this.
Well, since the ibm compiler appears to have been EOL'd for OS2, would it be
reasonable to assume that any solution would involve using an entirely different
compiler? (timeless mentioned openwatcom)

Comment 22

15 years ago
Yes, right now we are looking at OpenWatcom. We have no timeframe though.
I got timeless to test, and his version of openwatcom (Watcom C16 Optimizing
Compiler  Version 11.0c) doesn't handle this test case. It also doesn't seem to
know about the true/false boolean types. Its possible that anextra option of
some sort was needed, though - it was a really quick test. OTOH, a quick browse
through the docs at http://www.openwatcom.org/support/reference_content.html
shows functions called |isEmpty| with an int return type, which isn't very hopeful.

gcc doesn't work on os2, either, although there are patches for pgcc for 2.95.3.
pgcc was gcc + 'pentium specific optimisations'. That was released in Jan 2000,
and seems to be unmaintained. theres also a 2.8.1 binary downloadable.

Sigh.
(Assignee)

Comment 24

15 years ago
gcc 2.95 compiles Mozilla, what's wrong with that?  Unless the patch sucks,
maybe that will work better than VisualAge :)  timeless, you have a copy of that
around?

Comment 25

15 years ago
We're looking at bring the OS/2 GCC build back. It's just a matter of time.
(Assignee)

Comment 26

15 years ago
That would be great :)

Comment 27

15 years ago
collecting under the |nsCOMPtr| tracking bug # 178174
Blocks: 178174
Summary: cannot forward declare nsCOMPtr classes → |nsCOMPtr|: is it time to remove the 'don't forward declare T hack'?

Updated

15 years ago
Severity: normal → enhancement
(Assignee)

Comment 28

15 years ago
Setting dependent on bug 107291, "drop VisualAge on OS/2"
Depends on: 179118

Comment 29

15 years ago
This hack is also present in nsAutoPtr.h
(Assignee)

Comment 30

14 years ago
Created attachment 124785 [details] [diff] [review]
nsAutoPtr.h Patch

This patch does the same thing for nsRefPtr and friends.  sicking has given r=
already.  There are no other copies of the hack that I can find in lxr.
Does the AIX compiler have the same bug as the VisualAge compiler?  (The AIX
tinderbox recently disappeared from SeaMonkey-Ports, so be careful...)

Comment 32

14 years ago
We were having networking problems today which is why the Tinderbox went down. 
I am restarting it now.

I will try the testcase in this defect against the AIX VisualAge C++ v6 
compiler.
(Assignee)

Comment 33

14 years ago
We'll have to find out ... I checked in the nsCOMPtr patch last night, so when
the tinderbox cycles we shall see.
(Assignee)

Comment 34

14 years ago
Comment on attachment 124785 [details] [diff] [review]
nsAutoPtr.h Patch

I will not check this in until we verify that AIX has cleared, but the VA
people told us before that they had fixed this bug in later versions of the
compiler, so v6 is probably OK.  Nonetheless, I'd like to get r=/sr= now so I
*can* check this in when that is verified.
Attachment #124785 - Flags: superreview?(dbaron)
Attachment #124785 - Flags: review?(bugmail)
Attachment #124785 - Flags: superreview?(dbaron) → superreview+
Comment on attachment 124785 [details] [diff] [review]
nsAutoPtr.h Patch

you should probably forwarddeclare some user of nsAutoPtr too. There's a bunch
in transformiix which is XP
Attachment #124785 - Flags: review?(bugmail) → review+

Comment 36

14 years ago
Yesterday's xpcom patch built fine on AIX - we didn't have any errors while
building today's daily build. I have appled the patch to nsAutoPtr.h to a build
tree and rebuilt several files which use nsAutoPtr.h (including
extensions/transformiix/source/xslt/*.cpp). I have not rebuilt the entire tree
with the nsAutoPtr.h patch, but everything seems to work fine.
(Assignee)

Updated

14 years ago
Blocks: 208188
(Assignee)

Comment 37

14 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago14 years ago
Resolution: --- → FIXED
(Assignee)

Comment 38

14 years ago
OK, so there is a problem which we're trying to figure out if we can get around:
on gcc2.9x and below, a class with an automatic destructor and a
forward-declared nsCOMPtr will fail to compile because the destructor is
compiled when declared, not when used.  This showed up in about 15% of the XSLT
changes (bug 208188).

Comment 39

14 years ago
there's a bug about ABI warnings which relates to these automatic destructors,
perhaps the simplest fix for all of the problems is to simply write the empty
automatic destructors.
(Assignee)

Comment 40

14 years ago
That would solve it (I presume you mean declaring all destructors in these
classes and then defining them in the .cpp), but if you *don't* follow that rule
you will break gcc2.9x Linux and waste your own time.  So ... is a C++
portability rule enough?  We have other things you can do that break platforms,
but this one *is* preventable.

sicking has a great idea to make COMPtrs not depend on the type when destructing
(reinterpret_cast to nsISupports before calling AddRef / Release).  That would
allow us to keep the hack out of nsCOMPtr.  Unfortunately for nsAutoPtr and
nsRefPtr there is no such recourse; we'd have to rely on a style rule or a
sizeof hack there.

Another thing that could be done is focus the hack so that it only happens when
the RefPtr / AutoPtr is *destructed* and there is no type information ... but
that probably wouldn't work because as we have seen the automatic destructor is
not compiled until and unless it is needed on the other platforms.  I'll give it
a whirl anyway, on the off chance.
The ABI warnings aare because (basically), if you don't declare a destructor,
the compiler has to place a default one somewhere in the vtable. The spec says
to put it at the end; instead it puts it at the beginning. At some point gc will
move to follow the spec, and that will change the ABI.

See teh ABI bug for more.
You need to log in before you can comment on or make changes to this bug.