1118554 - make android's stdcxx work

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: android-libstdcxx.patch

There are issues with using our in tree stlport with the latest NDKs, dynamically linking to the stl provided by the NDK is a nice alternative, but first we need to make it work.

Comment 1

[Mike Hommey [:glandium]]

> There are issues with using our in tree stlport with the latest NDKs

What are they?

Comment 2

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #1)
> > There are issues with using our in tree stlport with the latest NDKs
> 
> What are they?

Note that probably means they did some changes to stlport that we may want to take.

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8544916 [details] [diff] [review]
android-libstdcxx.patch

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

::: media/webrtc/trunk/webrtc/system_wrappers/source/spreadsortlib/spreadsort.hpp
@@ +20,5 @@
>  #include <cstring>
>  #include <vector>
>  #include "webrtc/system_wrappers/source/spreadsortlib/constants.hpp"
>  
> +#define getchar(a, b) getchar()

How can this even not break the code in this file, where getchar is a local variable pointing to a callback function?

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: android-libstdcxx.patch

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Mike Hommey [:glandium] from comment #1)
> > There are issues with using our in tree stlport with the latest NDKs
> 
> What are they?

bug 1091377

Comment 6

[Mike Hommey [:glandium]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #0)
> Created attachment 8544916 [details] [diff] [review]
> android-libstdcxx.patch
> 
> There are issues with using our in tree stlport with the latest NDKs,
> dynamically linking to the stl provided by the NDK is a nice alternative,
> but first we need to make it work.

Note, I wonder if we shouldn't remove --enable-android-libstdcxx.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Mike Hommey [:glandium] from comment #6)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #0)
> > Created attachment 8544916 [details] [diff] [review]
> > android-libstdcxx.patch
> > 
> > There are issues with using our in tree stlport with the latest NDKs,
> > dynamically linking to the stl provided by the NDK is a nice alternative,
> > but first we need to make it work.
> 
> Note, I wonder if we shouldn't remove --enable-android-libstdcxx.

why?

Comment 8

[Mike Hommey [:glandium]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #7)
> (In reply to Mike Hommey [:glandium] from comment #6)
> > (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #0)
> > > Created attachment 8544916 [details] [diff] [review]
> > > android-libstdcxx.patch
> > > 
> > > There are issues with using our in tree stlport with the latest NDKs,
> > > dynamically linking to the stl provided by the NDK is a nice alternative,
> > > but first we need to make it work.
> > 
> > Note, I wonder if we shouldn't remove --enable-android-libstdcxx.
> 
> why?

Because it's never going to be used for release builds, and obviously, noone uses it since it's broken, and you only figured it's broken because you were trying to work around a build failure with stlport (instead of fixing that)

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Mike Hommey [:glandium] from comment #8)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #7)
> > (In reply to Mike Hommey [:glandium] from comment #6)
> > > (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #0)
> > > > Created attachment 8544916 [details] [diff] [review]
> > > > android-libstdcxx.patch
> > > > 
> > > > There are issues with using our in tree stlport with the latest NDKs,
> > > > dynamically linking to the stl provided by the NDK is a nice alternative,
> > > > but first we need to make it work.
> > > 
> > > Note, I wonder if we shouldn't remove --enable-android-libstdcxx.
> > 
> > why?
> 
> Because it's never going to be used for release builds, and obviously, noone
> uses it since it's broken, and you only figured it's broken because you were
> trying to work around a build failure with stlport (instead of fixing that)

why not use it on release?

Comment 10

[Mike Hommey [:glandium]]

Because of its massive size, increasing the apk size.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/93a8e6ea1324

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8544991 [details] [diff] [review]
android-libstdcxx.patch

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

r+ on the spreadsort... r- on the pkixtestutil.cpp unless there's a darn good reason.  And since it's already landed without resolving the r?, consider backing that part out.

::: security/pkix/test/lib/pkixtestutil.cpp
@@ +28,5 @@
>  #include <cstdio>
>  #include <limits>
>  #include <new>
>  #include <sstream>
> +#include <stdlib.h>

is this supposed to be here in this patch?

Comment 13

[Mike Hommey [:glandium]]

(In reply to Randell Jesup [:jesup] from comment #12)
> Comment on attachment 8544991 [details] [diff] [review]
> android-libstdcxx.patch
> 
> Review of attachment 8544991 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ on the spreadsort...

BTW, don't you store patches applied to webrtc separately?

> r- on the pkixtestutil.cpp unless there's a darn
> good reason.  And since it's already landed

without a landing comment, at that... (besides the merge from kwierso in comment 11, but that was landed on inbound before that)

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Randell Jesup [:jesup] from comment #12)
> Comment on attachment 8544991 [details] [diff] [review]
> android-libstdcxx.patch
> 
> Review of attachment 8544991 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ on the spreadsort... r- on the pkixtestutil.cpp unless there's a darn
> good reason.  And since it's already landed without resolving the r?,
> consider backing that part out.
> 
> ::: security/pkix/test/lib/pkixtestutil.cpp
> @@ +28,5 @@
> >  #include <cstdio>
> >  #include <limits>
> >  #include <new>
> >  #include <sstream>
> > +#include <stdlib.h>
> 
> is this supposed to be here in this patch?

Yes, without it abort() isn't defined

10:35.91 In file included from /Users/blassey/src/mozilla-inbound/security/pkix/test/gtest/pkixder_pki_types_tests.cpp:30:0:
10:35.91 /Users/blassey/src/mozilla-inbound/security/pkix/test/gtest/../../lib/pkixder.h: In function 'mozilla::pkix::Result mozilla::pkix::der::OptionalVersion(mozilla::pkix::Reader&, mozilla::pkix::der::Version&)':
10:35.91 Warning: -Wmaybe-uninitialized in /Users/blassey/src/mozilla-inbound/security/pkix/lib/pkixder.h: 'integerValue' may be used uninitialized in this function
10:35.91 /Users/blassey/src/mozilla-inbound/security/pkix/test/gtest/../../lib/pkixder.h:504:3: warning: 'integerValue' may be used uninitialized in this function [-Wmaybe-uninitialized]
10:35.91    switch (integerValue) {
10:35.91    ^
10:36.78 TestMP4Demuxer.o
10:36.95 /Users/blassey/src/mozilla-inbound/security/pkix/test/lib/pkixtestutil.cpp: In function 'bool mozilla::pkix::test::InputEqualsByteString(mozilla::pkix::Input, const ByteString&)':
10:36.95 /Users/blassey/src/mozilla-inbound/security/pkix/test/lib/pkixtestutil.cpp:81:11: error: 'abort' was not declared in this scope
10:36.95      abort();
10:36.95            ^
10:36.95 /Users/blassey/src/mozilla-inbound/security/pkix/test/lib/pkixtestutil.cpp: In function 'mozilla::pkix::test::ByteString mozilla::pkix::test::TLV(uint8_t, const ByteString&)':
10:36.95 /Users/blassey/src/mozilla-inbound/security/pkix/test/lib/pkixtestutil.cpp:140:11: error: 'abort' was not declared in this scope
10:36.95      abort();
10:36.95            ^
10:36.95 /Users/blassey/src/mozilla-inbound/security/pkix/test/lib/pkixtestutil.cpp: In function 'mozilla::pkix::test::ByteString mozilla::pkix::test::Integer(long int)':
10:36.95 /Users/blassey/src/mozilla-inbound/security/pkix/test/lib/pkixtestutil.cpp:215:11: error: 'abort' was not declared in this scope
10:36.95      abort();
10:36.95            ^
10:36.96 /Users/blassey/src/mozilla-inbound/security/pkix/test/lib/pkixtestutil.cpp: In function 'void mozilla::pkix::test::MaybeLogOutput(const ByteString&, const char*)':
10:36.96 /Users/blassey/src/mozilla-inbound/security/pkix/test/lib/pkixtestutil.cpp:452:59: error: 'getenv' was not declared in this scope
10:36.96    const char* logPath = getenv("MOZILLA_PKIX_TEST_LOG_DIR");
10:36.96                                                            ^
10:37.05 In the directory  /Users/blassey/src/mozilla-inbound/objdir-droid-ndk_r10d/security/pkix/test/lib
10:37.05 The following command failed to execute properly:
10:37.05 /tools/android-ndk-r10d/toolchains/arm-linux-androideabi-4.9/prebuilt/darwin-x86_64/bin/arm-linux-androideabi-g++ -o pkixtestutil.o -c -I../../../../dist/system_wrappers -include /Users/blassey/src/mozilla-inbound/config/gcc_hidden.h -DSTATIC_EXPORTABLE_JS_API -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DAB_CD=en-US -DNO_NSPR_10_SUPPORT -I/Users/blassey/src/mozilla-inbound/security/pkix/test/lib -I. -I/Users/blassey/src/mozilla-inbound/security/pkix/test/lib/../../include -I/Users/blassey/src/mozilla-inbound/security/pkix/test/lib/../../lib -I../../../../dist/include -I/Users/blassey/src/mozilla-inbound/objdir-droid-ndk_r10d/dist/include/nspr -I/Users/blassey/src/mozilla-inbound/objdir-droid-ndk_r10d/dist/include/nss -fPIC -idirafter /tools/android-ndk-r10d/platforms/android-9/arch-arm/usr/include -DMOZILLA_CLIENT -include ../../../../mozilla-config.h -MD -MP -MF .deps/pkixtestutil.o.pp -idirafter /tools/android-ndk-r10d/platforms/android-9/arch-arm/usr/include -Wall -Wempty-body -Woverloaded-virtual -Wsign-compare -Wwrite-strings -Werror=endif-labels -Werror=int-to-pointer-cast -Werror=missing-braces -Werror=parentheses -Werror=pointer-arith -Werror=return-type -Werror=sequence-point -Werror=switch -Werror=trigraphs -Werror=type-limits -Werror=unused-label -Wno-invalid-offsetof -Wno-error=uninitialized -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -mandroid -fno-short-enums -fno-exceptions -Wno-psabi -march=armv7-a -mthumb -mfpu=vfp -mfloat-abi=softfp -mno-unaligned-access -I/tools/android-ndk-r10d/sources/cxx-stl/gnu-libstdc++/4.9/include -I/tools/android-ndk-r10d/sources/cxx-stl/gnu-libstdc++/4.9/libs/armeabi-v7a/include -I/tools/android-ndk-r10d/sources/cxx-stl/gnu-libstdc++/4.9/include/backward -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -std=gnu++0x -pipe -DNDEBUG -DTRIMMED -g -freorder-blocks -Os -fno-reorder-functions -fomit-frame-pointer -Werror /Users/blassey/src/mozilla-inbound/security/pkix/test/lib/pkixtestutil.cpp

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Mike Hommey [:glandium] from comment #13)
> (In reply to Randell Jesup [:jesup] from comment #12)
> without a landing comment, at that... (besides the merge from kwierso in
> comment 11, but that was landed on inbound before that)

huh?

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #15)
> (In reply to Mike Hommey [:glandium] from comment #13)
> > (In reply to Randell Jesup [:jesup] from comment #12)
> > without a landing comment, at that... (besides the merge from kwierso in
> > comment 11, but that was landed on inbound before that)
> 
> huh?

I think he means it was landed on inbound without a comment in the bug (link to pushlog).  That was why I thought kweirso had landed it directly on inbound (which would be weird, but I only had a few minutes at the time).

Also it was landed with an outstanding r? to me (from glandium); I presume because it was to a webrtc file.  I'm not a security peer, and the security file in the patch wasn't mentioned anywhere here, which is why that was so confusing.

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Randell Jesup [:jesup] from comment #16)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #15)
> > (In reply to Mike Hommey [:glandium] from comment #13)
> > > (In reply to Randell Jesup [:jesup] from comment #12)
> > > without a landing comment, at that... (besides the merge from kwierso in
> > > comment 11, but that was landed on inbound before that)
> > 
> > huh?
> 
> I think he means it was landed on inbound without a comment in the bug (link
> to pushlog).  
I don't usually comment when I land on inbound. We stopped that practice a when we stopped putting [inbound] in the whiteboard.
> That was why I thought kweirso had landed it directly on
> inbound (which would be weird, but I only had a few minutes at the time).
> 
> Also it was landed with an outstanding r? to me (from glandium); I presume
> because it was to a webrtc file. 
I didn't see that glandium added a reviewer when he r+'d. The original review request was to gcp before glandium did a drive by and seemed to take over.


> I'm not a security peer, and the security
> file in the patch wasn't mentioned anywhere here, which is why that was so
> confusing.

Adding a missing include doesn't seem like it requires specialized security review, but again, that's why the original reviewer was gcp.

Comment 18

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8544991 [details] [diff] [review]
android-libstdcxx.patch

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

::: security/pkix/test/lib/pkixtestutil.cpp
@@ +28,5 @@
>  #include <cstdio>
>  #include <limits>
>  #include <new>
>  #include <sstream>
> +#include <stdlib.h>

For consistency <cstdlib> would've been preferable BTW.

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: cstdlib.patch

Comment 20

[Mike Hommey [:glandium]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #17)
> I don't usually comment when I land on inbound. We stopped that practice a
> when we stopped putting [inbound] in the whiteboard.

Huh? Where does that come from?

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ca36fde7d91f