[NaCl SDK] Compile for naclio for Bionic

[NaCl SDK] Compile for naclio for Bionic

Adds ARM components to build (sel_ldr, irt_core, nacl_bootstrap_helper).

Adds bionic kernel wrap.
Fixes compile issues for NaCl IO.
Fixes nacl_io_test cases for bionic.

BUG=345338
R=binji@chromium.org, sbc@chromium.org, benji@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261537

[noelallen1, 2014-03-26 23:50:37]

ptal

[Sam Clegg, 2014-03-27 00:10:02]

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/bu...
File native_client_sdk/src/build_tools/build_projects.py (right):

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/bu...
native_client_sdk/src/build_tools/build_projects.py:32: 
Extra newline here for some reason?

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/bu...
native_client_sdk/src/build_tools/build_projects.py:189:
make_cmd.append('ENABLE_BIONIC=1')
Is it OK to add this unconditionally?

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/bu...
File native_client_sdk/src/build_tools/build_sdk.py (right):

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/bu...
native_client_sdk/src/build_tools/build_sdk.py:1029: 
I don't think you need to full folder here, just a few .so files that sel_ldr
needs... but we not shipping this so I guess its fine for now.

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/h_errno.cc (right):

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/h_errno.cc:19: extern "C" int
*__h_errno_location() {
Make this into a .c file and drop the extern "C"s?

Or at least use the macros for adding extern "C".

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap.h (right):

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap.h:67: #if
!defined(__BIONIC__)
Why?

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_newlib.cc (right):

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_newlib.cc:9: #if
defined(__native_client__) && !defined(__GLIBC__) && !defined(__BIONIC__)
You can change this to _NEWLIB_VERSION I think.  I just found out about this
define the other day.

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_real.h (right):

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_real.h:14: /* NOTE: We do
not use off_t because it may be 32 or 64 bit */
Can you use a typedef here to signal the intent (perhaps off64_t?).

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/syscalls/htonl.c (right):

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/syscalls/htonl.c:7: #if
defined(PROVIDES_SOCKET_API) && !defined(__GLIBC__) && !defined(__BIONIC__)
Use _NEWLIB_VERSION here and in other syscalls .c files.

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/syscalls/ioctl.c (right):

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/syscalls/ioctl.c:9: */
Comment is not true.  Maybe just drop it.. Also doesn't the stdarg include below
include __BIONIC__?  Maybe you can drop the sys/types.h include too?

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/syscalls/poll.c (right):

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/syscalls/poll.c:11: #if
!defined(__BIONIC__)
If you move #if below the include you don't need to add the extra includes.  
This is inline with the other syscall .c files that are newlib or glibc only.

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/syscalls/signal.c (right):

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/syscalls/signal.c:10: 
Comment is not true.

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/to...
File native_client_sdk/src/tools/nacl_gcc.mk (right):

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/to...
native_client_sdk/src/tools/nacl_gcc.mk:332: $(call LOG,LINK,$$@,$(ARM_LINK)
-static -o $$@ $$(filter %.o,$$^) $(NACL_LDFLAGS) $(ARM_LDFLAGS) $(foreach
path,$(6),-L$(path)/$(TOOLCHAIN)_arm/$(CONFIG)) $(foreach lib,$(3),-l$(lib))
$(5))
Make this bionic only?  Or is it fine to add this for the existing arm compiler?

[Sam Clegg, 2014-03-27 00:10:43]

other than that lgtm.

[noelallen1, 2014-03-27 18:13:48]

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/bu...
File native_client_sdk/src/build_tools/build_projects.py (right):

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/bu...
native_client_sdk/src/build_tools/build_projects.py:32: 
On 2014/03/27 00:10:02, Sam Clegg wrote:
> Extra newline here for some reason?

Done.

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/bu...
native_client_sdk/src/build_tools/build_projects.py:189:
make_cmd.append('ENABLE_BIONIC=1')
On 2014/03/27 00:10:02, Sam Clegg wrote:
> Is it OK to add this unconditionally?

Yes, every project we provide overrides the VALID_TOOLCHAINS

ifneq ($(ENABLE_BIONIC),)
ALL_TOOLCHAINS ?= pnacl newlib glibc bionic
else
ALL_TOOLCHAINS ?= pnacl newlib glibc
endif

...

So for some example:
VALID_TOOLCHAINS := newlib glibc pnacl linux

#
# The target for all versions
#
USABLE_TOOLCHAINS=$(filter $(OSNAME) $(ALL_TOOLCHAINS), $(VALID_TOOLCHAINS))

If bionic is not enabled, then the project generator will not add bionic to the
list of possible targets.

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/bu...
File native_client_sdk/src/build_tools/build_sdk.py (right):

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/bu...
native_client_sdk/src/build_tools/build_sdk.py:1029: 
On 2014/03/27 00:10:02, Sam Clegg wrote:
> I don't think you need to full folder here, just a few .so files that sel_ldr
> needs... but we not shipping this so I guess its fine for now.

True, I will re-examine if/when we ship it.

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/h_errno.cc (right):

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/h_errno.cc:19: extern "C" int
*__h_errno_location() {
On 2014/03/27 00:10:02, Sam Clegg wrote:
> Make this into a .c file and drop the extern "C"s?
> 
> Or at least use the macros for adding extern "C".

Done.

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap.h (right):

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap.h:67: #if
!defined(__BIONIC__)
On 2014/03/27 00:10:02, Sam Clegg wrote:
> Why?

Done.

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_newlib.cc (right):

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_newlib.cc:9: #if
defined(__native_client__) && !defined(__GLIBC__) && !defined(__BIONIC__)
On 2014/03/27 00:10:02, Sam Clegg wrote:
> You can change this to _NEWLIB_VERSION I think.  I just found out about this
> define the other day.

I think a global replace makes more sense.

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_real.h (right):

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_real.h:14: /* NOTE: We do
not use off_t because it may be 32 or 64 bit */
On 2014/03/27 00:10:02, Sam Clegg wrote:
> Can you use a typedef here to signal the intent (perhaps off64_t?).

Initially I had off64_t but got push from core.  I suspect they want to just use
types in stdint.h

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/syscalls/ioctl.c (right):

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/syscalls/ioctl.c:9: */
On 2014/03/27 00:10:02, Sam Clegg wrote:
> Comment is not true.  Maybe just drop it.. Also doesn't the stdarg include
below
> include __BIONIC__?  Maybe you can drop the sys/types.h include too?

Done.

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/syscalls/poll.c (right):

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/syscalls/poll.c:11: #if
!defined(__BIONIC__)
On 2014/03/27 00:10:02, Sam Clegg wrote:
> If you move #if below the include you don't need to add the extra includes.  
> This is inline with the other syscall .c files that are newlib or glibc only.

Done.

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/syscalls/signal.c (right):

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/syscalls/signal.c:10: 
On 2014/03/27 00:10:02, Sam Clegg wrote:
> Comment is not true.

Done.

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/to...
File native_client_sdk/src/tools/nacl_gcc.mk (right):

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/to...
native_client_sdk/src/tools/nacl_gcc.mk:332: $(call LOG,LINK,$$@,$(ARM_LINK)
-static -o $$@ $$(filter %.o,$$^) $(NACL_LDFLAGS) $(ARM_LDFLAGS) $(foreach
path,$(6),-L$(path)/$(TOOLCHAIN)_arm/$(CONFIG)) $(foreach lib,$(3),-l$(lib))
$(5))
On 2014/03/27 00:10:02, Sam Clegg wrote:
> Make this bionic only?  Or is it fine to add this for the existing arm
compiler?

Since the arm compiler only links statically now it's a no-op.

[Sam Clegg, 2014-03-27 18:18:31]

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_real.h (right):

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_real.h:14: /* NOTE: We do
not use off_t because it may be 32 or 64 bit */
On 2014/03/27 18:13:48, noelallen1 wrote:
> On 2014/03/27 00:10:02, Sam Clegg wrote:
> > Can you use a typedef here to signal the intent (perhaps off64_t?).
> 
> Initially I had off64_t but got push from core.  I suspect they want to just
use
> types in stdint.h

Can we just use nacl_io_off_t then?   just a nacl_io-local change to make things
more clear.

[binji, 2014-04-02 18:35:53]

lgtm

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/bu...
File native_client_sdk/src/build_tools/build_projects.py (right):

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/bu...
native_client_sdk/src/build_tools/build_projects.py:189: # -t bionic have been
provided ont he command line, then VALID_TOOLCHAINS
on the

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/h_errno.c (right):

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/h_errno.c:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/h_errno.c:12: static pthread_key_t
__h_errno_key;
identifiers that begin with __ are reserved. Chrome code seems to use s_ as a
prefix for static variables.

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_bionic.cc (right):

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_bionic.cc:8: // compiled,
even on a non-glibc build.
non-bionic

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_bionic.cc:20: #include
<irt_syscalls.h>
alphabetize? If this has to come after the previous headers, add a newline
between so it doesn't get caught by the presubmit check.

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_bionic.cc:126: OP(dup2);  \
nit: remove extra space

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_bionic.cc:154: int
WRAP(chdir) (const char* pathname) {
nit: remove extra space

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_bionic.cc:202: return
ki_getcwd(buf, size);
why doesn't this return errno? Is this wrapped function different in bionic?

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_bionic.cc:439: int err =
REAL(lstat)(path, (struct stat*) &st);
This C-style cast makes me nervous... why are you forcing nacl_abi_stat to
struct stat here?

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/socket/socket_node.cc (right):

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/socket/socket_node.cc:279: if
(static_cast<size_t>(len) < sizeof(int))
Might be nicer to do this once at the top of the function, and mention why. What
is the size of socklen_t on bionic?

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/syscalls/access.c (right):

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/syscalls/access.c:9: // GLIBC-only entry
point.
comment should mention bionic (or be removed)

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/to...
File native_client_sdk/src/tools/nacl_gcc.mk (right):

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/to...
native_client_sdk/src/tools/nacl_gcc.mk:46: ARM_CFLAGS ?= -gdwarf-3
only in debug configuration?

[noelallen1, 2014-04-03 17:58:51]

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/bu...
File native_client_sdk/src/build_tools/build_projects.py (right):

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/bu...
native_client_sdk/src/build_tools/build_projects.py:189: # -t bionic have been
provided ont he command line, then VALID_TOOLCHAINS
On 2014/04/02 18:35:53, binji wrote:
> on the

Done.

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/h_errno.c (right):

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/h_errno.c:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2014/04/02 18:35:53, binji wrote:
> 2014

Done.

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/h_errno.c:12: static pthread_key_t
__h_errno_key;
On 2014/04/02 18:35:53, binji wrote:
> identifiers that begin with __ are reserved. Chrome code seems to use s_ as a
> prefix for static variables.

Done.

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_bionic.cc (right):

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_bionic.cc:8: // compiled,
even on a non-glibc build.
On 2014/04/02 18:35:53, binji wrote:
> non-bionic

Done.

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_bionic.cc:20: #include
<irt_syscalls.h>
On 2014/04/02 18:35:53, binji wrote:
> alphabetize? If this has to come after the previous headers, add a newline
> between so it doesn't get caught by the presubmit check.

Done.

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_bionic.cc:126: OP(dup2);  \
On 2014/04/02 18:35:53, binji wrote:
> nit: remove extra space

Done.

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_bionic.cc:154: int
WRAP(chdir) (const char* pathname) {
On 2014/04/02 18:35:53, binji wrote:
> nit: remove extra space

Done.

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_bionic.cc:202: return
ki_getcwd(buf, size);
On 2014/04/02 18:35:53, binji wrote:
> why doesn't this return errno? Is this wrapped function different in bionic?

Done.

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_bionic.cc:439: int err =
REAL(lstat)(path, (struct stat*) &st);
On 2014/04/02 18:35:53, binji wrote:
> This C-style cast makes me nervous... why are you forcing nacl_abi_stat to
> struct stat here?

Because the IRT.H unfortunately uses struct stat where it should use nacl_stat.

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/socket/socket_node.cc (right):

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/socket/socket_node.cc:279: if
(static_cast<size_t>(len) < sizeof(int))
On 2014/04/02 18:35:53, binji wrote:
> Might be nicer to do this once at the top of the function, and mention why.
What
> is the size of socklen_t on bionic?
It's a different type, not size.

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/syscalls/access.c (right):

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/syscalls/access.c:9: // GLIBC-only entry
point.
On 2014/04/02 18:35:53, binji wrote:
> comment should mention bionic (or be removed)

Done.

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/to...
File native_client_sdk/src/tools/nacl_gcc.mk (right):

https://codereview.chromium.org/176923017/diff/80001/native_client_sdk/src/to...
native_client_sdk/src/tools/nacl_gcc.mk:46: ARM_CFLAGS ?= -gdwarf-3
On 2014/04/02 18:35:53, binji wrote:
> only in debug configuration?

Done.

[Sam Clegg, 2014-04-03 18:57:33]

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_real.h (right):

https://codereview.chromium.org/176923017/diff/40001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_real.h:14: /* NOTE: We do
not use off_t because it may be 32 or 64 bit */
On 2014/03/27 18:18:32, Sam Clegg wrote:
> On 2014/03/27 18:13:48, noelallen1 wrote:
> > On 2014/03/27 00:10:02, Sam Clegg wrote:
> > > Can you use a typedef here to signal the intent (perhaps off64_t?).
> > 
> > Initially I had off64_t but got push from core.  I suspect they want to just
> use
> > types in stdint.h
> 
> Can we just use nacl_io_off_t then?   just a nacl_io-local change to make
things
> more clear.

Ping?

Can we typedef this? Even if you just use int64_t in your typedef I think its
better than using int64_t directly in the signatures below.

[noelallen1, 2014-04-03 21:45:17]

Committed patchset #7 manually as r261537 (presubmit successful).