689397 - Work around the broken-ness of gcc4.2 on Mac which causes the protection implemented in bug 666414 not take effect

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[(no longer active)]

gcc4.2 accepts this program:

class A { public: int foo() { return 0; } };
template <class T> class B : public T { private: using T::foo; };
int main() {
  B<A> b; return b.foo();
}

but correctly rejects this program:

class A { public: int foo() { return 0; } };
class B : public A { private: using A::foo; };
int main() {
  B b; return b.foo();
}

This wonderful bug causes gcc to not catch what bug 666414 tried to protect against.

I have a patch which revives a version of HAVE_CPP_ACCESS_CHANGING_USING in order to work around this bug.  I'm currently building to make sure that I have all of the Mac specific usage sites of bug 666414 covered.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[Mozilla RelEng Bot]

Try run for f1fb5e90c12f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f1fb5e90c12f
Results (out of 18 total builds):
    success: 7
    warnings: 1
    failure: 10
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f1fb5e90c12f

Comment 3

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Is it worth it? Maybe it is better to just say


--------------------------------------------
// We don't use using in here because some compilers (like gcc 4.2) fail to enforce that
// they are private.
//             using T::AddRef;
//             using T::Release;

            nsrefcnt AddRef();
            nsrefcnt Release();
----------------------------------------------

If using the configure check, can you put it in a .m4 file so that we don't get even more duplicated code in configure and js/src/configure?

Comment 4

[(no longer active)]

For some reason the configure check is not run at all on Windows: <https://tbpl.mozilla.org/php/getParsedLog.php?id=6581571&tree=Try&full=1>

Can someone think of why?

Comment 5

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(In reply to Ehsan Akhgari [:ehsan] from comment #4)
> For some reason the configure check is not run at all on Windows:
> <https://tbpl.mozilla.org/php/getParsedLog.php?id=6581571&tree=Try&full=1>
> 
> Can someone think of why?

SKIP_COMPILER_CHECKS

Comment 6

[Mozilla RelEng Bot]

Try run for afd108456d31 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=afd108456d31
Results (out of 18 total builds):
    success: 13
    warnings: 2
    failure: 3
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-afd108456d31

Comment 7

[Mozilla RelEng Bot]

Try run for f834dfa8bbde is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f834dfa8bbde
Results (out of 18 total builds):
    success: 18
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f834dfa8bbde

Comment 8

[(no longer active)]

Attachment: First approach

This is the first approach: doing a configure check and use using if we can, and fall back to an override otherwise.

Comment 9

[(no longer active)]

Attachment: Second approach

The second approach is to override AddRef and Release everywhere.

Comment 10

[Mozilla RelEng Bot]

Try run for 53a1b41d569f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=53a1b41d569f
Results (out of 18 total builds):
    success: 14
    warnings: 1
    failure: 3
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-53a1b41d569f

Comment 11

[(no longer active)]

(In reply to Mozilla RelEng Bot from comment #10)
> Try run for 53a1b41d569f is complete.
> Detailed breakdown of the results available here:
>     https://tbpl.mozilla.org/?tree=Try&rev=53a1b41d569f
> Results (out of 18 total builds):
>     success: 14
>     warnings: 1
>     failure: 3
> Builds available at
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.
> com-53a1b41d569f

The Windows build failures here can be fixed by using NS_IMETHOD_(nsrefcnt) instead of nsrefcnt in the patch.

Comment 12

[Benjamin Smedberg]

Approach B seems fine to me and avoids configure weirdness, but I'm not sure why we need to fix this at all: real cases of this being broken will show up in the cross-platform builds.

Comment 13

[(no longer active)]

Attachment: Patch (v1)

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #12)
> Approach B seems fine to me and avoids configure weirdness, but I'm not sure
> why we need to fix this at all: real cases of this being broken will show up
> in the cross-platform builds.

But Mac specific code will still get away with such problems.  See bug 690235 for an example.

Comment 14

[Mozilla RelEng Bot]

Try run for 79854fd04a17 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=79854fd04a17
Results (out of 18 total builds):
    exception: 3
    success: 4
    warnings: 2
    failure: 9
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-79854fd04a17

Comment 15

[Mozilla RelEng Bot]

Try run for 2ecc049218c9 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2ecc049218c9
Results (out of 18 total builds):
    exception: 3
    success: 4
    warnings: 2
    failure: 9
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-2ecc049218c9

Comment 16

[(no longer active)]

Comment on attachment 563832 [details] [diff] [review]
Patch (v1)

>diff --git a/xpcom/glue/nsCOMPtr.h b/xpcom/glue/nsCOMPtr.h
>--- a/xpcom/glue/nsCOMPtr.h
>+++ b/xpcom/glue/nsCOMPtr.h
>@@ -438,19 +438,26 @@ nsCOMPtr_base
>             No client should ever see or have to type the name of this class.  It is the
>             artifact that makes it a compile-time error to call |AddRef| and |Release|
>             on a |nsCOMPtr|.  DO NOT USE THIS TYPE DIRECTLY IN YOUR CODE.
> 
>             See |nsCOMPtr::operator->| and |nsRefPtr::operator->|.
>           */
>         {
>           private:
>-            using T::AddRef;
>-            using T::Release;
>-            
>+            NS_IMETHOD_(nsrefcnt) AddRef();
>+            NS_IMETHOD_(nsrefcnt) Release();
>+            //using T::AddRef;
>+            //using T::Release;
>+            /*
>+             We could use |using| above, except that gcc 4.2 on Mac has a bug
>+             which causes |using| be unable to make the function private in
>+             templated derived classes (see bug 689397).
>+            */
>+
>             ~nsDerivedSafe(); // NOT TO BE IMPLEMENTED
>             /* 
>               This dtor is added to make this class compatible with GCC 4.6.
>               If the destructor for T is private, nsDerivedSafe's unimplemented destructor 
>               will be implicitly-declared by the compiler as deleted.
>               Therefore this explicit dtor exists to avoid that deletion. See bug 689301.
>             */
> 

Seems like this hunk causes xpcshell to crash during make package!  This happens even with clang.  Rafael, an you think of why?

Comment 17

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> Seems like this hunk causes xpcshell to crash during make package!  This
> happens even with clang.  Rafael, an you think of why?

Do these macros expand into virtual methods? That would create an undefined reference from the vtable.

Comment 18

[(no longer active)]

New try job: http://tbpl.mozilla.org/?tree=Try&rev=6a3266473cae

Comment 19

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1692379c7637

Comment 20

[Mozilla RelEng Bot]

Try run for 6a3266473cae is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6a3266473cae
Results (out of 18 total builds):
    success: 16
    warnings: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6a3266473cae

Comment 21

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/1692379c7637