Last Comment Bug 689397 - Work around the broken-ness of gcc4.2 on Mac which causes the protection implemented in bug 666414 not take effect
: Work around the broken-ness of gcc4.2 on Mac which causes the protection impl...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla10
Assigned To: :Ehsan Akhgari (out sick)
:
Mentors:
Depends on:
Blocks: 666414 690235
  Show dependency treegraph
 
Reported: 2011-09-26 17:33 PDT by :Ehsan Akhgari (out sick)
Modified: 2011-10-12 03:14 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (7.07 KB, patch)
2011-09-26 17:43 PDT, :Ehsan Akhgari (out sick)
no flags Details | Diff | Review
First approach (10.99 KB, patch)
2011-09-29 08:10 PDT, :Ehsan Akhgari (out sick)
no flags Details | Diff | Review
Second approach (5.24 KB, patch)
2011-09-29 08:14 PDT, :Ehsan Akhgari (out sick)
no flags Details | Diff | Review
Patch (v1) (5.27 KB, patch)
2011-09-30 13:44 PDT, :Ehsan Akhgari (out sick)
benjamin: review+
Details | Diff | Review

Description :Ehsan Akhgari (out sick) 2011-09-26 17:33:34 PDT
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 :Ehsan Akhgari (out sick) 2011-09-26 17:43:20 PDT
Created attachment 562608 [details] [diff] [review]
Patch (v1)
Comment 2 Mozilla RelEng Bot 2011-09-26 23:31:12 PDT
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
Comment 3 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-27 13:28:38 PDT
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 :Ehsan Akhgari (out sick) 2011-09-27 17:47:30 PDT
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) 2011-09-27 20:42:16 PDT
(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 2011-09-28 00:03:16 PDT
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
Comment 7 Mozilla RelEng Bot 2011-09-29 00:02:39 PDT
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
Comment 8 :Ehsan Akhgari (out sick) 2011-09-29 08:10:31 PDT
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.
Comment 9 :Ehsan Akhgari (out sick) 2011-09-29 08:14:59 PDT
Created attachment 563415 [details] [diff] [review]
Second approach

The second approach is to override AddRef and Release everywhere.
Comment 10 Mozilla RelEng Bot 2011-09-29 13:02:33 PDT
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
Comment 11 :Ehsan Akhgari (out sick) 2011-09-29 13:12:11 PDT
(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 [:bsmedberg] 2011-09-30 12:53:45 PDT
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 :Ehsan Akhgari (out sick) 2011-09-30 13:44:19 PDT
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.
Comment 14 Mozilla RelEng Bot 2011-09-30 18:51:35 PDT
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
Comment 15 Mozilla RelEng Bot 2011-10-05 22:11:22 PDT
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 16 :Ehsan Akhgari (out sick) 2011-10-07 08:58:17 PDT
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) 2011-10-10 08:46:50 PDT
> 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 :Ehsan Akhgari (out sick) 2011-10-11 10:04:00 PDT
New try job: http://tbpl.mozilla.org/?tree=Try&rev=6a3266473cae
Comment 19 :Ehsan Akhgari (out sick) 2011-10-11 13:59:21 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1692379c7637
Comment 20 Mozilla RelEng Bot 2011-10-11 14:41:01 PDT
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
Comment 21 Marco Bonardo [::mak] 2011-10-12 03:14:00 PDT
https://hg.mozilla.org/mozilla-central/rev/1692379c7637

Note You need to log in before you can comment on or make changes to this bug.