support x86 android

RESOLVED FIXED in mozilla10

Status

()

Core
Build Config
--
enhancement
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

Trunk
mozilla10
x86
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
NDKr6 has x86 compiler, so I would like to add x86 android target for experiment
Depends on: 681203
I had a crack at this last week.  I think the x86 target in r6 is just busted, ran into some linking issues.
(Assignee)

Comment 2

6 years ago
(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.
(Assignee)

Updated

6 years ago
Depends on: 681558
(Assignee)

Comment 3

6 years ago
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.
Assignee: nobody → m_kato
(Assignee)

Comment 4

6 years ago
Created attachment 560883 [details] [diff] [review]
fix v1
Attachment #555338 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #560883 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 5

6 years ago
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 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.
Attachment #560883 - Flags: review?(ted.mielczarek) → review-
(Assignee)

Comment 7

6 years ago
Created attachment 562419 [details] [diff] [review]
fix v2
Attachment #560883 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #562419 - Flags: review?(ted.mielczarek)
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 on attachment 562419 [details] [diff] [review]
fix v2

How about I just defer this entire review to glandium?
Attachment #562419 - Flags: review?(ted.mielczarek) → review?(mh+mozilla)
(Assignee)

Comment 10

6 years ago
Created attachment 563062 [details] [diff] [review]
fix v3
Attachment #562419 - Attachment is obsolete: true
Attachment #562419 - Flags: review?(mh+mozilla)
Attachment #563062 - Flags: review?(mh+mozilla)
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 */
Attachment #563062 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 12

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5d8bfe7a014
Whiteboard: [inbound]

Comment 13

6 years ago
https://hg.mozilla.org/mozilla-central/rev/c5d8bfe7a014
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla10
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?
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.
You need to log in before you can comment on or make changes to this bug.