Closed Bug 689301 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: XPCOM, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: cpearce, Assigned: adityab)

References

Details

(Keywords: regression, Whiteboard: [qa-])

Attachments

(2 files, 2 obsolete files)

No description provided.
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.
Summary: Compile error in nx → Compile error in nsTimerImpl.cpp with gcc 4.6.0 on Fedora core 15 x64
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!
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!
Blocks: 666414
Keywords: regression
Attached patch WIPSplinter Review
Chris, can you please see if this patch fixes your build?
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
I can confirm that it builds fine now.
Comment on attachment 562606 [details] [diff] [review]
WIP

Confirm that, this fixes the build for me.
Attachment #562606 - Flags: feedback+
Attached patch Add explicit destructor and note (obsolete) — Splinter Review
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
Duplicate of this bug: 689390
The patch fixes my build as well.
Comment on attachment 562624 [details] [diff] [review]
Add explicit destructor and note

This is the best of the three patches!
Attachment #562624 - Flags: review?(benjamin)
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.
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.
It doesn't need to be virtual. The new patch makes it non-virtual and private.
Attachment #562612 - Attachment is obsolete: true
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 :-)
Attachment #562834 - Flags: feedback+
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.
s/Benjamin/Joe/, yikes, not winning.
I pushed Aditya's patch, thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/7147f6403c2a
Assignee: ehsan → aditya
Target Milestone: --- → mozilla10
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 (?))
Attachment #562624 - Attachment is obsolete: true
Attachment #562624 - Flags: review?(benjamin)
> (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? :-)
https://hg.mozilla.org/mozilla-central/rev/7147f6403c2a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Should we land this on aurora too?
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.  :-)
Attachment #562834 - Flags: approval-mozilla-aurora?
Aurora - Yes please, if the bug exists there!  Otherwise I need to keep local patches in my queue all the time
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.
Attachment #562834 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.