681202 - support x86 android

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Makoto Kato [:m_kato]]

NDKr6 has x86 compiler, so I would like to add x86 android target for experiment

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

I had a crack at this last week.  I think the x86 target in r6 is just busted, ran into some linking issues.

Comment 2

[Makoto Kato [:m_kato]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #1)
> I had a crack at this last week.  I think the x86 target in r6 is just
> busted, ran into some linking issues.

It seems not to work --allow-shlib-undefined on NDK x86 toolchain.

Comment 3

[Makoto Kato [:m_kato]]

Attachment: WIP

build-able fix with NDK R6

- I should file a bug for libvpx because rand isn't defined in NDK.  (rand is inlined function on NDK).
- some configuration is incorrect even if arm android (why android tries to link asound?).
- nspr part is bug 681558.

Comment 4

[Makoto Kato [:m_kato]]

Attachment: fix v1

Comment 5

[Makoto Kato [:m_kato]]

libvpx needs fix on upstream (This fix isn't included in this patches).  More detail is https://groups.google.com/a/webmproject.org/group/webm-discuss/browse_thread/thread/145f12e8441c6456/37c61a7f65288e98.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 560883 [details] [diff] [review]
fix v1

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

This is r- for the symbol visibility issue, but otherwise it looks ok.

::: configure.in
@@ +3039,3 @@
>          if test "$ac_cv_have_visibility_builtin_bug" = "no" -a \
> +                "$ac_cv_have_visibility_class_bug" = "no" -a \
> +                "$OS_TARGET" != "Android"; then

This doesn't seem right. What symbols is it missing? Are we just missing some entries in config/system-headers?

::: other-licenses/android/Makefile.in
@@ +53,5 @@
>    -DLINKER_AREA_SIZE=0x01000000 \
>    -DANDROID_PACKAGE_NAME='"$(ANDROID_PACKAGE_NAME)"' \
>    $(NULL)
>  
> +ifeq ($(OS_TEST),arm)

Please use CPU_ARCH here instead.

@@ +58,5 @@
> +DEFINES += \
> +  -DANDROID_ARM_LINKER \
> +  $(NULL)
> +else
> +ifeq (86,$(findstring 86,$(OS_TEST)))

You can use ifdef INTEL_ARCHITECTURE here nowadays.

::: toolkit/xre/nsSigHandlers.cpp
@@ +64,1 @@
>  #ifndef __arm__ // no arm impl

Was this __arm__ define added just for android? If so, I think you could remove it.

Comment 7

[Makoto Kato [:m_kato]]

Attachment: fix v2

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 562419 [details] [diff] [review]
fix v2

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

::: memory/jemalloc/jemalloc.h
@@ +80,4 @@
>  void	jemalloc_stats(jemalloc_stats_t *stats);
> +#if defined(MOZ_MEMORY_ANDROID)
> +#pragma GCC visibility pop
> +#endif

This sounds wrong. We may simply want to add jemalloc.h to system-headers.

::: other-licenses/android/Makefile.in
@@ +56,5 @@
>  
> +ifeq ($(CPU_ARCH),arm)
> +DEFINES += \
> +  -DANDROID_ARM_LINKER \
> +  $(NULL)

DEFINES += -DANDROID_ARM_LINKER
No need for multi-line things for a single addition.

::: other-licenses/android/dlfcn.h
@@ +56,5 @@
>  extern int          dlclose(void*  handle);
>  extern const char*  dlerror(void);
>  extern void*        dlsym(void*  handle, const char*  symbol);
>  extern int          dladdr(void* addr, Dl_info *info);
> +#pragma GCC visibility pop

Arguably, all these should be NS_EXPORT, but maybe the header for that doesn't exist in dist/include when we build that directory... maybe we should -I$(topsrcdir)/xpcom/base and #include nscore.h

Likewise in APKOpen.h

::: other-licenses/android/nsGeckoUtils.cpp
@@ +37,5 @@
>  
>  #include <jni.h>
>  #include <stdlib.h>
>  
> +#pragma GCC visibility push(default)

That should definitely not be needed here.

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 562419 [details] [diff] [review]
fix v2

How about I just defer this entire review to glandium?

Comment 10

[Makoto Kato [:m_kato]]

Attachment: fix v3

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 563062 [details] [diff] [review]
fix v3

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

r=me, with some minor nits.

::: configure.in
@@ +4400,5 @@
>  
>  case "${target}" in
>      *-android*|*-linuxandroid*)
> +        case "${target_cpu}" in
> +        arm*)

if test "$CPU_ARCH" = "arm" ; then
  USE_ARM_KUSER=1
fi

::: memory/jemalloc/jemalloc.h
@@ +53,5 @@
>  /* Android doesn't have posix_memalign */
>  #ifdef MOZ_MEMORY_ANDROID
>  int	posix_memalign(void **memptr, size_t alignment, size_t size);
> +void    _malloc_prefork(void);
> +void    _malloc_postfork(void);

Please add a comment for these, since hg blame will likely be confused by the definition move.
Something like /* Android < 2.3 doesn't have pthread_atfork, so we need to call these when forking the child process. See bug 680190 */

Comment 12

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5d8bfe7a014

Comment 13

[Michael Wu [:mwu]]

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

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

I still can't make x86 build.  I got the familiar 'missing atexit()' error with latest m-c, and after commenting that out it fails to find libpthread during linking. Do you have a working mozconfig you could post?

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

Also, my local checkout of m-c doesn't appear to have the patch here. Did it get backed out? Regardless, applying the patch results in the problems I mention above.