689301 - Compile error in nsTimerImpl.cpp with gcc 4.6.0 on Fedora core 15 x64

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

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 <[e-mail]>
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

[(no longer active)]

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]]

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

[(no longer active)]

Attachment: WIP

Chris, can you please see if this patch fixes your build?

Comment 5

[Aditya Bhatt [:adityab]]

Attachment: Add an explicit destructor to nsCOMPtr_base::nsDerivedSafe to comply with C++0x .

Comment 6

[Aditya Bhatt [:adityab]]

I can confirm that it builds fine now.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 562606 [details] [diff] [review]
WIP

Confirm that, this fixes the build for me.

Comment 8

[Chris Forbes]

Attachment: Add explicit destructor and note

Comment 9

[Mozilla RelEng Bot]

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/[e-mail]-13d450008bdf

Comment 11

[Brad Jackson]

The patch fixes my build as well.

Comment 13

[(no longer active)]

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)]

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)]

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]]

Attachment: 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)]

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]

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]

s/Benjamin/Joe/, yikes, not winning.

Comment 20

[(no longer active)]

I pushed Aditya's patch, thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/7147f6403c2a

Comment 21

[Daniel Holbert [:dholbert]]

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)]

> (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]]

https://hg.mozilla.org/mozilla-central/rev/7147f6403c2a

Comment 24

[Boris Zbarsky [:bzbarsky]]

Should we land this on aurora too?

Comment 25

[(no longer active)]

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] (needinfo me)]

Aurora - Yes please, if the bug exists there!  Otherwise I need to keep local patches in my queue all the time

Comment 27

[LegNeato]

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.

Comment 28

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/79eab065f0d0