The default bug view has changed. See this FAQ.

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!

Updated

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