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

RESOLVED FIXED in mozilla10

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: cpearce, Assigned: adityab)

Tracking

({regression})

unspecified
mozilla10
x86_64
Linux
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

6 years ago
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!
(Assignee)

Comment 3

6 years ago
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
Created attachment 562606 [details] [diff] [review]
WIP

Chris, can you please see if this patch fixes your build?
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
(Assignee)

Comment 5

6 years ago
Created attachment 562612 [details] [diff] [review]
Add an explicit destructor to nsCOMPtr_base::nsDerivedSafe to comply with C++0x .
(Assignee)

Comment 6

6 years ago
I can confirm that it builds fine now.
(Reporter)

Comment 7

6 years ago
Comment on attachment 562606 [details] [diff] [review]
WIP

Confirm that, this fixes the build for me.
Attachment #562606 - Flags: feedback+

Comment 8

6 years ago
Created attachment 562624 [details] [diff] [review]
Add explicit destructor and note

Comment 9

6 years ago
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

Updated

6 years ago
Duplicate of this bug: 689390

Comment 11

6 years ago
The patch fixes my build as well.
Duplicate of this bug: 689520
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.
(Assignee)

Comment 16

6 years ago
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.
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+
Attachment #562834 - Flags: review+

Comment 18

6 years ago
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

6 years ago
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
Last Resolved: 6 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 27

6 years ago
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+
http://hg.mozilla.org/releases/mozilla-aurora/rev/79eab065f0d0
status-firefox9: --- → fixed
Whiteboard: [qa-]

Updated

4 years ago
status-firefox9: fixed → ---
You need to log in before you can comment on or make changes to this bug.