Closed Bug 1103816 Opened 5 years ago Closed 5 years ago

[gonk-l] update android_stub.h for api level 21

Categories

(Firefox OS Graveyard :: GonkIntegration, defect, major)

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S2 (19dec)

People

(Reporter: seinlin, Assigned: mwu)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Summary: [gonk-l] nss get compile error → [gonk-l] update android_stub.h for api level 21
Blocks: gonk-L
Assignee: nobody → mwu
Comment on attachment 8537575 [details] [diff] [review]
Add support for gonk-L to android_stub.h

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

::: config/external/nss/Makefile.in
@@ +212,5 @@
>  	OS_PTHREAD= \
>  	$(NULL)
>  
> +ifeq ($(filter 21,$(ANDROID_VERSION)),)
> +EXTRA_FLAGS := "-DRTLD_NOLOAD=0"

RTLD_NOLOAD is actually a proper enum now, so defining RTLD_NOLOAD as zero means the compiler ends up seeing "0 = 4" when it opens dlfcn.h. http://androidxref.com/5.0.0_r2/xref/bionic/libc/include/dlfcn.h#52

Alternately we might be able to include dlfcn in the android stub and #define RTLD_NOLOAD 4 to avoid version checks.

::: security/manager/android_stub.h
@@ +14,5 @@
>  
>  #include <sys/cdefs.h>
>  #include <linux/kernel.h>
>  
> +extern int getdtablesize(void);

Android is deprecating this and removing it from their headers. It's still available for 32bit arm, but not in 64bit arm. We should probably switch NSS to sysconf() if we can, but that involves an NSS patch. Looks easy to do though - I'll follow up with a bug for that.
Attachment #8537575 - Flags: review?(mh+mozilla)
On second thought, I don't think we can switch to sysconf so easily. Bionic's sysconf(_SC_OPEN_MAX) is hard coded to 256, but the actual default limit is 1024. glibc on the other hand, implements sysconf(_SC_OPEN_MAX) using getdtablesize, so there's no problems there.
Comment on attachment 8537575 [details] [diff] [review]
Add support for gonk-L to android_stub.h

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

::: config/external/nss/Makefile.in
@@ +211,5 @@
>  	OS_RELEASE='2.6' \
>  	OS_PTHREAD= \
>  	$(NULL)
>  
> +ifeq ($(filter 21,$(ANDROID_VERSION)),)

This test is not future-proof. You should probably move the define in android_stub.h, since #if can do numeric comparisons there.

@@ +212,5 @@
>  	OS_PTHREAD= \
>  	$(NULL)
>  
> +ifeq ($(filter 21,$(ANDROID_VERSION)),)
> +EXTRA_FLAGS := "-DRTLD_NOLOAD=0"

There should be a comment to that effect in the code rather than in the bug.

::: security/manager/android_stub.h
@@ +14,5 @@
>  
>  #include <sys/cdefs.h>
>  #include <linux/kernel.h>
>  
> +extern int getdtablesize(void);

You should be able to use getrlimit(RLIMIT_NOFILE, ...)
Attachment #8537575 - Flags: review?(mh+mozilla) → feedback-
Attachment #8537575 - Attachment is obsolete: true
Comment on attachment 8538562 [details] [diff] [review]
Add support for gonk-L to android_stub.h, v2

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

Review comments addressed. I didn't put extra comments about RTLD_NOLOAD since moving the version check to the header makes it obvious.
Attachment #8538562 - Flags: review?(mh+mozilla)
Comment on attachment 8538562 [details] [diff] [review]
Add support for gonk-L to android_stub.h, v2

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

::: config/external/nss/Makefile.in
@@ +211,5 @@
>  	OS_RELEASE='2.6' \
>  	OS_PTHREAD= \
>  	$(NULL)
>  
> +ifneq ($(ANDROID_VERSION),)

ifdef ANDROID_VERSION has the same meaning and is less obscure. OTOH, you could even do DEFAULT_GMAKE_FLAGS += $(addprefix -DANDROID_VERSION=,$(ANDROID_VERSION)), with no condition.
Attachment #8538562 - Flags: review?(mh+mozilla) → review+
Attachment #8538562 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/997e003830a8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S2 (19dec)
(In reply to Mike Hommey [:glandium] from comment #10)
...
> ifdef ANDROID_VERSION has the same meaning and is less obscure. OTOH, you
> could even do DEFAULT_GMAKE_FLAGS += $(addprefix
> -DANDROID_VERSION=,$(ANDROID_VERSION)), with no condition.

security/nss/lib/softoken/fipstokn.c fails to compile using NDK 10d here.

It appears as though ANDROID_VERSION is not defined when including:
security/manager/android_stub.h

If the above is edited to explicitly contain '-DANDROID_VERSION=21' then it does build.

fwiw 'ac_add_options --with-android-version=21' is included in the mozconfig file, yet config.status does not defined ANDROID_VERSION, rather ANDROID_TARGET_SDK is defined to be 21.
Flags: needinfo?(mh+mozilla)
(In reply to Douglas Crosher [:dougc] from comment #14)
> fwiw 'ac_add_options --with-android-version=21' is included in the mozconfig
> file, yet config.status does not defined ANDROID_VERSION, rather
> ANDROID_TARGET_SDK is defined to be 21.

ANDROID_VERSION is only defined in gonk builds.
Flags: needinfo?(mh+mozilla)
Duplicate of this bug: 1092026
(In reply to Michael Wu [:mwu] from comment #6)
> On second thought, I don't think we can switch to sysconf so easily.
> Bionic's sysconf(_SC_OPEN_MAX) is hard coded to 256, but the actual default
> limit is 1024. glibc on the other hand, implements sysconf(_SC_OPEN_MAX)
> using getdtablesize, so there's no problems there.

Just as a note, looks like ndk11 has removed getdtablesize from the 32-but headers too. NSS is failing - did you ever log an NSS bug for this?
You need to log in before you can comment on or make changes to this bug.