Last Comment Bug 689301 - Compile error in nsTimerImpl.cpp with gcc 4.6.0 on Fedora core 15 x64
: Compile error in nsTimerImpl.cpp with gcc 4.6.0 on Fedora core 15 x64
Status: RESOLVED FIXED
[qa-]
: regression
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla10
Assigned To: Aditya Bhatt [:adityab]
:
Mentors:
: 689390 689520 (view as bug list)
Depends on:
Blocks: 666414
  Show dependency treegraph
 
Reported: 2011-09-26 14:28 PDT by Chris Pearce (:cpearce)
Modified: 2013-10-13 02:15 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (844 bytes, patch)
2011-09-26 17:41 PDT, :Ehsan Akhgari
cpearce: feedback+
Details | Diff | Splinter Review
Add an explicit destructor to nsCOMPtr_base::nsDerivedSafe to comply with C++0x . (483 bytes, patch)
2011-09-26 17:54 PDT, Aditya Bhatt [:adityab]
no flags Details | Diff | Splinter Review
Add explicit destructor and note (805 bytes, patch)
2011-09-26 18:27 PDT, Chris Forbes
no flags Details | Diff | Splinter Review
Add an explicit private non-virtual destructor and explanatory comment that refers to this thread. (778 bytes, patch)
2011-09-27 12:27 PDT, Aditya Bhatt [:adityab]
respindola: review+
respindola: feedback+
christian: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Chris Pearce (:cpearce) 2011-09-26 14:28:13 PDT

    
Comment 1 Chris Pearce (:cpearce) 2011-09-26 14:32:21 PDT
I get the following compile error when building /xpcom/threads/ on my fedora core 15 x64 machine with gcc 4.6.0:

nsTimerImpl.cpp
In file included from /home/cpearce/src/m-c/xpcom/threads/nsTimerImpl.cpp:41:0:
/home/cpearce/src/m-c/xpcom/threads/nsTimerImpl.h: In member function ‘bool nsTimerImpl::IsRepeating() const’:
/home/cpearce/src/m-c/xpcom/threads/nsTimerImpl.h:118:59: warning: comparison between ‘enum nsITimer::<anonymous>’ and ‘enum nsITimer::<anonymous>’ [-Wenum-compare]
/home/cpearce/src/m-c/xpcom/threads/nsTimerImpl.h:119:66: warning: comparison between ‘enum nsITimer::<anonymous>’ and ‘enum nsITimer::<anonymous>’ [-Wenum-compare]
/home/cpearce/src/m-c/xpcom/threads/nsTimerImpl.h:120:68: warning: comparison between ‘enum nsITimer::<anonymous>’ and ‘enum nsITimer::<anonymous>’ [-Wenum-compare]
In file included from /home/cpearce/src/m-c/xpcom/threads/nsTimerImpl.h:50:0,
                 from /home/cpearce/src/m-c/xpcom/threads/nsTimerImpl.cpp:41:
../../dist/include/nsCOMPtr.h: At global scope:
../../dist/include/nsCOMPtr.h: In instantiation of ‘nsCOMPtr_base::nsDerivedSafe<nsTimerEvent>’:
/home/cpearce/src/m-c/xpcom/threads/nsTimerImpl.cpp:540:10:   instantiated from here
../../dist/include/nsCOMPtr.h:436:7: error: deleted function ‘virtual nsCOMPtr_base::nsDerivedSafe<nsTimerEvent>::~nsDerivedSafe()’
/home/cpearce/src/m-c/xpcom/threads/nsTimerImpl.cpp:491:3: error: overriding non-deleted function ‘virtual nsTimerEvent::~nsTimerEvent()’
../../dist/include/nsCOMPtr.h:436:7: error: ‘virtual nsCOMPtr_base::nsDerivedSafe<nsTimerEvent>::~nsDerivedSafe()’ is implicitly deleted because the default definition would be ill-formed:
/home/cpearce/src/m-c/xpcom/threads/nsTimerImpl.cpp:491:3: error: ‘virtual nsTimerEvent::~nsTimerEvent()’ is private
../../dist/include/nsCOMPtr.h:436:7: error: within this context

In the directory  /home/cpearce/src/m-c/objdir/xpcom/threads
The following command failed to execute properly:
/usr/bin/ccache c++ -o nsTimerImpl.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include ../../../config/gcc_hidden.h -DMOZILLA_INTERNAL_API -DOSTYPE="Linux2.6.38.6-26.rc1.fc15" -DOSARCH=Linux -D_IMPL_NS_COM -I../../../xpcom/threads/../components -I../../../xpcom/threads -I. -I../../dist/include -I../../dist/include/nsprpub -I/home/cpearce/src/m-c/objdir/dist/include/nspr -I/home/cpearce/src/m-c/objdir/dist/include/nss -fPIC -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-variadic-macros -Werror=return-type -pedantic -Wno-long-long -fno-strict-aliasing -std=gnu++0x -pthread -ffunction-sections -fdata-sections -pipe -DDEBUG -D_DEBUG -DTRACING -g -fno-omit-frame-pointer -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MF .deps/nsTimerImpl.pp /home/cpearce/src/m-c/xpcom/threads/nsTimerImpl.cpp
make[2]: *** [nsTimerImpl.o] Error 1
make[1]: *** [libs] Error 2
make: *** [default] Error 2

Other info:

[cpearce@caesar m-c]$ hg tip
changeset:   77586:ebccffe6d7e6
tag:         tip
user:        Ehsan Akhgari <ehsan@mozilla.com>
date:        Mon Sep 26 16:41:38 2011 -0400
summary:     Bug 666414 followup - Fix the Android build bustage

[cpearce@caesar m-c]$ gcc -v
Using built-in specs.
COLLECT_GCC=/usr/bin/gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/4.6.0/lto-wrapper
Target: x86_64-redhat-linux
Configured with: ../configure --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-bootstrap --enable-shared --enable-threads=posix --enable-checking=release --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --enable-languages=c,c++,objc,obj-c++,java,fortran,ada,go,lto --enable-plugin --enable-java-awt=gtk --disable-dssi --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-1.5.0.0/jre --enable-libgcj-multifile --enable-java-maintainer-mode --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --disable-libjava-multilib --with-ppl --with-cloog --with-tune=generic --with-arch_32=i686 --build=x86_64-redhat-linux
Thread model: posix
gcc version 4.6.0 20110603 (Red Hat 4.6.0-10) (GCC) 


Possibly a problem with a recent checkins by Ehsan (maybe 489f9e746213 ?) with gcc 4.6.0.
Comment 2 :Ehsan Akhgari 2011-09-26 14:42:45 PDT
So, I _think_ the reason why this happens is nsTimerEvent having a private dtor, and nsCOMPtr_base::nsDerivedSafe having no dtors at all.

Questions for the compiler gurus:

1. What in world is a deleted function?  This is the first time that I'm seeing this phrase.  :-)
2. Do you guys know if our code pattern here is not valid C++, or if we're just seeing a gcc bug of some sort?

Thanks!
Comment 3 Aditya Bhatt [:adityab] 2011-09-26 16:54:02 PDT
Ehsan,
1. A deleted function is one that a declaration of the form
int func(int) = delete;

AFAIK this is a new feature in GCC 4.6 and is used when you don't want anyone to use that method - if used anywhere, the compilation should fail.

2. See this link:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2009/n2924.pdf
In section "12.4 Destructors", it lists why the implicitly-declared destructor for nsCOMPtr_base::nsDerivedSafe should be deleted:
"An implicitly-declared destructor for a class X is deleted if:
...
- any direct or virtual base class has a deleted destructor or a destructor that is inaccessible from the implicitly-declared destructor."

The destructor for nsTimerEvent is private, therefore nsDerivedSafe's destructor is implicitly-declared as deleted.

Also:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2326.html#delete
states that "A deleted virtual function may not override a non-deleted virtual function and vice-versa.".
Hence the error 

../../dist/include/nsCOMPtr.h:436:7: error: deleted function ‘virtual nsCOMPtr_base::nsDerivedSafe<nsTimerEvent>::~nsDerivedSafe()’
/home/cpearce/src/m-c/xpcom/threads/nsTimerImpl.cpp:491:3: error: overriding non-deleted function ‘virtual nsTimerEvent::~nsTimerEvent()’

.

Maybe simply adding an explicit destructor (without implementing) to nsDerivedSafe should do the trick?


Thanks!
Comment 4 :Ehsan Akhgari 2011-09-26 17:41:38 PDT
Created attachment 562606 [details] [diff] [review]
WIP

Chris, can you please see if this patch fixes your build?
Comment 5 Aditya Bhatt [:adityab] 2011-09-26 17:54:51 PDT
Created attachment 562612 [details] [diff] [review]
Add an explicit destructor to nsCOMPtr_base::nsDerivedSafe to comply with C++0x .
Comment 6 Aditya Bhatt [:adityab] 2011-09-26 18:20:55 PDT
I can confirm that it builds fine now.
Comment 7 Chris Pearce (:cpearce) 2011-09-26 18:23:52 PDT
Comment on attachment 562606 [details] [diff] [review]
WIP

Confirm that, this fixes the build for me.
Comment 8 Chris Forbes 2011-09-26 18:27:22 PDT
Created attachment 562624 [details] [diff] [review]
Add explicit destructor and note
Comment 9 Mozilla RelEng Bot 2011-09-26 22:40:53 PDT
Try run for 13d450008bdf is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=13d450008bdf
Results (out of 18 total builds):
    success: 18
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-13d450008bdf
Comment 10 Takanori MATSUURA 2011-09-27 02:36:16 PDT
*** Bug 689390 has been marked as a duplicate of this bug. ***
Comment 11 Brad Jackson 2011-09-27 04:49:37 PDT
The patch fixes my build as well.
Comment 12 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-27 04:51:04 PDT
*** Bug 689520 has been marked as a duplicate of this bug. ***
Comment 13 :Ehsan Akhgari 2011-09-27 08:19:58 PDT
Comment on attachment 562624 [details] [diff] [review]
Add explicit destructor and note

This is the best of the three patches!
Comment 14 Joe Drew (not getting mail) 2011-09-27 11:23:07 PDT
Comment on attachment 562624 [details] [diff] [review]
Add explicit destructor and note

Review of attachment 562624 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/glue/nsCOMPtr.h
@@ +446,5 @@
>              using T::AddRef;
>              using T::Release;
> +            virtual ~nsDerivedSafe();   // NOT TO BE IMPLEMENTED
> +              /*
> +                This dtor exists to make this valid c++11. If the dtor is implicitly deleted

Drive-by: s/If the/The/, I think.
Comment 15 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-27 11:49:44 PDT
Should it be virtual? I am afraid of getting undefined references from the vtable. If possible I would try to declare a private non virtual destructor.
Comment 16 Aditya Bhatt [:adityab] 2011-09-27 12:27:53 PDT
Created attachment 562834 [details] [diff] [review]
Add an explicit private non-virtual destructor and explanatory comment that refers to this thread.

It doesn't need to be virtual. The new patch makes it non-virtual and private.
Comment 17 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-27 13:05:34 PDT
Comment on attachment 562834 [details] [diff] [review]
Add an explicit private non-virtual destructor and explanatory comment that refers to this thread.

Review of attachment 562834 [details] [diff] [review]:
-----------------------------------------------------------------

Not sure I can r+ this, but I would if I could :-)
Comment 18 Chris Forbes 2011-09-27 13:41:58 PDT
Absolutely, it doesn't need to be virtual, if T's destructor isn't already.

Benjamin: Yeah, I botched the explanation, sorry. s/If the/The/ and a couple of other clarity issues. Aditya's latest patch is better.
Comment 19 Chris Forbes 2011-09-27 13:47:05 PDT
s/Benjamin/Joe/, yikes, not winning.
Comment 20 :Ehsan Akhgari 2011-09-27 14:12:12 PDT
I pushed Aditya's patch, thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/7147f6403c2a
Comment 21 Daniel Holbert [:dholbert] 2011-09-27 16:06:19 PDT
Comment on attachment 562624 [details] [diff] [review]
Add explicit destructor and note

(canceling review-request & obsoleting the earlier patch-version that didn't get landed.  FWIW, this earlier patch actually triggered a startup crash for me, but the one that landed works.  (the only difference seems to be the 'virtual' keyword -- that must trigger the crash somehow (?))
Comment 22 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-27 16:07:29 PDT
> (canceling review-request & obsoleting the earlier patch-version that didn't
> get landed.  FWIW, this earlier patch actually triggered a startup crash for
> me, but the one that landed works.  (the only difference seems to be the
> 'virtual' keyword -- that must trigger the crash somehow (?))

undefined reference? :-)
Comment 23 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-09-28 02:11:20 PDT
https://hg.mozilla.org/mozilla-central/rev/7147f6403c2a
Comment 24 Boris Zbarsky [:bz] 2011-09-28 08:53:00 PDT
Should we land this on aurora too?
Comment 25 :Ehsan Akhgari 2011-09-28 09:20:28 PDT
Comment on attachment 562834 [details] [diff] [review]
Add an explicit private non-virtual destructor and explanatory comment that refers to this thread.

(In reply to Boris Zbarsky (:bz) from comment #24)
> Should we land this on aurora too?

I don't have a strong preference.  Technically, gcc4.6 is not a supported compiler, but I understand that quite a few people are using it because that's what Linux distros are shipping these days.  And the patch is as low risk as one can be, so let's nom it.  :-)
Comment 26 Randell Jesup [:jesup] 2011-09-28 13:18:38 PDT
Aurora - Yes please, if the bug exists there!  Otherwise I need to keep local patches in my queue all the time
Comment 27 christian 2011-09-29 14:37:03 PDT
Comment on attachment 562834 [details] [diff] [review]
Add an explicit private non-virtual destructor and explanatory comment that refers to this thread.

Looks harmless, approved.

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