837297 - Latest merge causing build errors with gcc 4.6

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Scott Johnson (:jwir3)]

I'm receiving the following errors on building fennec with gcc-4.6, ndk-r8c, and sdk-16:

    /usr/local/src/android-ndk-r8c/toolchains/arm-linux-androideabi-4.6/prebuilt/linux-x86/bin/../lib/gcc/arm-linux-androideabi/4.6/../../../../arm-linux-androideabi/bin/ld: /home/sjohnson/Source/mozilla-android/obj-android/mfbt/tests/TestWeakPtr.o: in function mozilla::RefCounted<mozilla::SupportsWeakPtr<C>::WeakReference>::Release():../../dist/include/mozilla/RefPtr.h:70: error: undefined reference to 'mozilla::RefCounted<mozilla::SupportsWeakPtr<C>::WeakReference>::dead'
    /usr/local/src/android-ndk-r8c/toolchains/arm-linux-androideabi-4.6/prebuilt/linux-x86/bin/../lib/gcc/arm-linux-androideabi/4.6/../../../../arm-linux-androideabi/bin/ld: /home/sjohnson/Source/mozilla-android/obj-android/mfbt/tests/TestWeakPtr.o: in function mozilla::RefCounted<mozilla::SupportsWeakPtr<C>::WeakReference>::~RefCounted():../../dist/include/mozilla/RefPtr.h:53: error: undefined reference to 'mozilla::RefCounted<mozilla::SupportsWeakPtr<C>::WeakReference>::dead'
    /usr/local/src/android-ndk-r8c/toolchains/arm-linux-androideabi-4.6/prebuilt/linux-x86/bin/../lib/gcc/arm-linux-androideabi/4.6/../../../../arm-linux-androideabi/bin/ld: /home/sjohnson/Source/mozilla-android/obj-android/mfbt/tests/TestWeakPtr.o: in function mozilla::RefCounted<mozilla::SupportsWeakPtr<A>::WeakReference>::Release():../../dist/include/mozilla/RefPtr.h:70: error: undefined reference to 'mozilla::RefCounted<mozilla::SupportsWeakPtr<A>::WeakReference>::dead'
    /usr/local/src/android-ndk-r8c/toolchains/arm-linux-androideabi-4.6/prebuilt/linux-x86/bin/../lib/gcc/arm-linux-androideabi/4.6/../../../../arm-linux-androideabi/bin/ld: /home/sjohnson/Source/mozilla-android/obj-android/mfbt/tests/TestWeakPtr.o: in function mozilla::RefCounted<mozilla::SupportsWeakPtr<A>::WeakReference>::~RefCounted():../../dist/include/mozilla/RefPtr.h:53: error: undefined reference to 'mozilla::RefCounted<mozilla::SupportsWeakPtr<A>::WeakReference>::dead'
    collect2: ld returned 1 exit status 

After using hg bisect to try and nail down the problem, I found the following changeset to be bad:

changeset:   120478:a4f8cb70cc5d
tag:         tip
parent:      120467:50cf5bbcb180
parent:      120477:44772e261a55
user:        Tim Taubert <[e-mail]>
date:        Fri Feb 01 10:17:01 2013 -0500
summary:     merge m-c to fx-team

Comment 1

[Kartikaya Gupta (email:[e-mail])]

I think nrc was seeing this as well, based on backscroll in #mobile. Not sure why your build would fail while the tbpl builders still build fine. What does your mozconfig look like?

Comment 2

[Nick Cameron [:nrc]]

Yes, I did, and still do. Only on debug builds, as long as I enable opt it builds fine. Error is:

APKOpen.cpp
/home/ncameron/android-ndk-r8c/toolchains/arm-linux-androideabi-4.6/prebuilt/linux-x86/bin/../lib/gcc/arm-linux-androideabi/4.6/../../../../arm-linux-androideabi/bin/ld: /home/ncameron/gfx-android/mozglue/build/../android/APKOpen.o: in function mozilla::RefCounted<Zip>::Release():../../dist/include/mozilla/RefPtr.h:70: error: undefined reference to 'mozilla::RefCounted<Zip>::dead'
/home/ncameron/android-ndk-r8c/toolchains/arm-linux-androideabi-4.6/prebuilt/linux-x86/bin/../lib/gcc/arm-linux-androideabi/4.6/../../../../arm-linux-androideabi/bin/ld: /home/ncameron/gfx-android/mozglue/build/../linker/Zip.o: in function mozilla::RefCounted<Zip>::~RefCounted():../../dist/include/mozilla/RefPtr.h:53: error: undefined reference to 'mozilla::RefCounted<Zip>::dead'
collect2: ld returned 1 exit status
make[6]: *** [libmozglue.so] Error 1
make[5]: *** [libs] Error 2
make[4]: *** [libs_tier_base] Error 2
make[3]: *** [tier_base] Error 2
make[2]: *** [default] Error 2
make[1]: *** [realbuild] Error 2
make: *** [build] Error 2

MozConfig is:

mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/../obj-android
mk_add_options MOZ_MAKE_FLAGS="-j12"
ac_add_options --enable-debug
ac_add_options --disable-tests
ac_add_options --disable-optimize
ac_add_options --with-android-ndk="$HOME/android-ndk-r8c"
ac_add_options --with-android-toolchain="$HOME/android-ndk-r8c/toolchains/arm-linux-androideabi-4.6/prebuilt/linux-x86"
ac_add_options --with-android-sdk="$HOME/android-sdk-linux/platforms/android-17"
ac_add_options --with-android-version=5
ac_add_options --enable-application=mobile/android
ac_add_options --target=arm-linux-androideabi
ac_add_options --with-ccache=/usr/bin/ccache

Comment 3

[Scott Johnson (:jwir3)]

It looks like, after doing a more thorough bisection, that the following is the first bad revision:

changeset:   120455:799de1cf584b
user:        Mike Hommey <[e-mail]>
date:        Tue Jan 29 09:35:16 2013 +0100
summary:     Bug 834769 - Change the "destroyed" state value for RefCounted. r=Waldo

Comment 4

[Nick Cameron [:nrc]]

Attachment: rename RefCounted::dead

Comment 5

[Nick Cameron [:nrc]]

Attachment: change dead field to a define

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 709533 [details] [diff] [review]
change dead field to a define

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

::: mfbt/RefPtr.h
@@ -54,5 @@
>  
>    public:
>      // Compatibility with nsRefPtr.
>      void AddRef() {
> -      MOZ_ASSERT(refCnt >= 0);

This test is there for good reasons, so changing the ref count type and removing it is not a great thing to do.

Comment 7

[Nick Cameron [:nrc]]

Attachment: patch v2

Without the signed/unsigned change.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 709639 [details] [diff] [review]
patch v2

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

LGTM, but I'm not a peer for MFBT.

Comment 9

[Nick Cameron [:nrc]]

try push: https://tbpl.mozilla.org/?tree=Try&rev=1df586f0b8f9

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 709639 [details] [diff] [review]
patch v2

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

Sigh, I guess C++ is screwy-ish in not allowing this.  Too bad we don't have constexpr yet; I think that'd get us around this trivially.

::: mfbt/RefPtr.h
@@ +41,5 @@
>   * use-after-destroy (refcount==0xffffdead).
>   */
> +#ifdef DEBUG
> +#define DEAD int(0xffffdead)
> +#endif

#defines leak, and leaking is going to come back to bite us.  In this case we could #undef when we're done, but it's still messy.  We'd also want to rename to MOZ_DEAD or something, too, to avoid conflicts.

I think the best workaround, rather than introducing a #define even temporarily, is to have a static const int that's *not* inside a class.  Then you can define it and use it without having to have a class-external definition and all the potential pain that might entail:

namespace detail {
static const int DEAD = 0xffffDEAD;
}

And then use detail::DEAD as necessary.

Comment 11

[Nick Cameron [:nrc]]

Attachment: patch v3

Comment 12

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15dd558ca101

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/15dd558ca101