make android's stdcxx work

RESOLVED FIXED in mozilla37

Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: blassey, Assigned: blassey)

Tracking

Trunk
mozilla37
x86
macOS

Firefox Tracking Flags

(fennec+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Posted patch android-libstdcxx.patch (obsolete) — Splinter Review
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.
Attachment #8544916 - Flags: review?(gpascutto)
> There are issues with using our in tree stlport with the latest NDKs

What are they?
(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 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?
Attachment #8544916 - Flags: review?(gpascutto) → feedback-
Assignee: nobody → blassey.bugs
Attachment #8544916 - Attachment is obsolete: true
Attachment #8544991 - Flags: review?(mh+mozilla)
(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
Attachment #8544991 - Flags: review?(rjesup)
Attachment #8544991 - Flags: review?(mh+mozilla)
Attachment #8544991 - Flags: review+
(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.
(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?
(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)
(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?
Because of its massive size, increasing the apk size.
https://hg.mozilla.org/mozilla-central/rev/93a8e6ea1324
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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?
Attachment #8544991 - Flags: review?(rjesup) → review+
(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)
(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
(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?
(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.
(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 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.
Posted patch cstdlib.patchSplinter Review
Attachment #8545939 - Flags: review?(gpascutto)
Attachment #8545939 - Flags: review?(gpascutto) → review+
tracking-fennec: ? → +
(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?
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.