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

RESOLVED FIXED in mozilla10

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

Trunk
mozilla10
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
Blocks: 666414
Created attachment 562608 [details] [diff] [review]
Patch (v1)
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #562608 - Flags: review?(benjamin)

Comment 2

6 years ago
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/eakhgari@mozilla.com-f1fb5e90c12f
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?
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?
(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

6 years ago
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/eakhgari@mozilla.com-afd108456d31
Attachment #562608 - Flags: review?(benjamin)
Blocks: 690235

Comment 7

6 years ago
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/eakhgari@mozilla.com-f834dfa8bbde
Created attachment 563410 [details] [diff] [review]
First approach

This is the first approach: doing a configure check and use using if we can, and fall back to an override otherwise.
Attachment #562608 - Attachment is obsolete: true
Attachment #563410 - Flags: review?(benjamin)
Created attachment 563415 [details] [diff] [review]
Second approach

The second approach is to override AddRef and Release everywhere.
Attachment #563415 - Flags: review?(benjamin)

Comment 10

6 years ago
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
(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.
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.
Attachment #563410 - Flags: review?(benjamin)
Attachment #563415 - Flags: review?(benjamin)
Created attachment 563832 [details] [diff] [review]
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.
Attachment #563410 - Attachment is obsolete: true
Attachment #563415 - Attachment is obsolete: true
Attachment #563832 - Flags: review?(benjamin)

Comment 14

6 years ago
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/eakhgari@mozilla.com-79854fd04a17
Attachment #563832 - Flags: review?(benjamin) → review+

Comment 15

6 years ago
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/eakhgari@mozilla.com-2ecc049218c9
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?
> 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.
New try job: http://tbpl.mozilla.org/?tree=Try&rev=6a3266473cae
https://hg.mozilla.org/integration/mozilla-inbound/rev/1692379c7637
Target Milestone: --- → mozilla10

Comment 20

6 years ago
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/eakhgari@mozilla.com-6a3266473cae
https://hg.mozilla.org/mozilla-central/rev/1692379c7637
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.