Last Comment Bug 681202 - support x86 android
: support x86 android
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86 Android
: -- enhancement (vote)
: mozilla10
Assigned To: Makoto Kato [:m_kato]
:
:
Mentors:
Depends on: 681203 681558
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-23 00:57 PDT by Makoto Kato [:m_kato]
Modified: 2011-10-07 08:46 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (12.45 KB, patch)
2011-08-24 00:20 PDT, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review
fix v1 (14.25 KB, patch)
2011-09-19 05:57 PDT, Makoto Kato [:m_kato]
ted: review-
Details | Diff | Splinter Review
fix v2 (15.30 KB, patch)
2011-09-26 06:51 PDT, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review
fix v3 (16.83 KB, patch)
2011-09-28 07:07 PDT, Makoto Kato [:m_kato]
mh+mozilla: review+
Details | Diff | Splinter Review

Description Makoto Kato [:m_kato] 2011-08-23 00:57:45 PDT
NDKr6 has x86 compiler, so I would like to add x86 android target for experiment
Comment 1 James Willcox (:snorp) (jwillcox@mozilla.com) 2011-08-23 10:00:40 PDT
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] 2011-08-23 18:06:12 PDT
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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] 2011-08-24 00:20:29 PDT
Created attachment 555338 [details] [diff] [review]
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] 2011-09-19 05:57:56 PDT
Created attachment 560883 [details] [diff] [review]
fix v1
Comment 5 Makoto Kato [:m_kato] 2011-09-19 06:00:57 PDT
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 Ted Mielczarek [:ted.mielczarek] 2011-09-23 10:10:31 PDT
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] 2011-09-26 06:51:20 PDT
Created attachment 562419 [details] [diff] [review]
fix v2
Comment 8 Mike Hommey [:glandium] 2011-09-26 07:12:22 PDT
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 Ted Mielczarek [:ted.mielczarek] 2011-09-26 08:21:10 PDT
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] 2011-09-28 07:07:51 PDT
Created attachment 563062 [details] [diff] [review]
fix v3
Comment 11 Mike Hommey [:glandium] 2011-09-28 07:23:18 PDT
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 13 Michael Wu [:mwu] 2011-09-29 01:39:00 PDT
https://hg.mozilla.org/mozilla-central/rev/c5d8bfe7a014
Comment 14 James Willcox (:snorp) (jwillcox@mozilla.com) 2011-10-07 07:34:45 PDT
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) (jwillcox@mozilla.com) 2011-10-07 08:46:44 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.