Closed Bug 689397 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: XPCOM, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

Attachments

(1 file, 3 obsolete files)

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
Attached patch Patch (v1) (obsolete) — Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #562608 - Flags: review?(benjamin)
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
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)
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
Attached patch First approach (obsolete) — Splinter Review
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)
Attached patch Second approach (obsolete) — Splinter Review
The second approach is to override AddRef and Release everywhere.
Attachment #563415 - Flags: review?(benjamin)
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)
Attached patch Patch (v1)Splinter Review
(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)
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+
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.
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
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.