Added support for NaCl IO to use the IRT Extension API.

Added support for NaCl IO to use the IRT Extension API.

In order to better integrate the IRT Extension API into NaCl IO, the
first implementation mirrors the other kernel_wrap_* files. All the
unit tests should therefore still be valid. Eventually it may be
easier to just have a separate version of kernel_intercept which
utilizes the IRT extension API directly.

Currently the IRT Extension interface is only supported within newlib,
so I have enabled it for newlib only.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=3918
TEST= trybots

Committed: https://crrev.com/3183230ac54a9b443f6e67a64121a0c3ee734e6b
Cr-Commit-Position: refs/heads/master@{#299938}

[David Yen, 2014-09-30 20:36:41]

dyen@chromium.org changed reviewers:
+ sbc@chromium.org

[David Yen, 2014-09-30 20:38:10]

I've added support in NaCl IO to use the IRT Extension API.

I'm waiting for a DEPS roll before this will actually compile on the main
branch. When that happens I'll rebase it to the head and run the trybots.

[Sam Clegg, 2014-09-30 20:49:03]

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

https://codereview.chromium.org/608913003/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap.h:22: #if
defined(__native_client__) && !defined(__GLIBC__) && !defined(__BIONIC__)
You can check for _NEWLIB_VERSION I think

https://codereview.chromium.org/608913003/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap.h:23: # define
__NACL_IO_IRT_EXT__ 1
Done use __ prefix and suffix.   IIUC such macros are reserved for the compiler.

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

https://codereview.chromium.org/608913003/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_irt_ext.cc:2: // Use of this
source code is governed by a BSD-style license that can be
You're not going to like this but... do you think you do this in C
rather than C++.  We are currently in the process of trying to remove
all the C++ from nacl_io for various reasons (e.g. it gets linked into
a lot of C programs).  Sorry :(

https://codereview.chromium.org/608913003/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_irt_ext.cc:25: (void) ret;  
         \
Use __attribute__ unused?

[David Yen, 2014-09-30 21:37:36]

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

https://codereview.chromium.org/608913003/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap.h:22: #if
defined(__native_client__) && !defined(__GLIBC__) && !defined(__BIONIC__)
On 2014/09/30 20:49:03, Sam Clegg wrote:
> You can check for _NEWLIB_VERSION I think

Done.

https://codereview.chromium.org/608913003/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap.h:23: # define
__NACL_IO_IRT_EXT__ 1
On 2014/09/30 20:49:03, Sam Clegg wrote:
> Done use __ prefix and suffix.   IIUC such macros are reserved for the
compiler.

Done.

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

https://codereview.chromium.org/608913003/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_irt_ext.cc:2: // Use of this
source code is governed by a BSD-style license that can be
On 2014/09/30 20:49:03, Sam Clegg wrote:
> You're not going to like this but... do you think you do this in C
> rather than C++.  We are currently in the process of trying to remove
> all the C++ from nacl_io for various reasons (e.g. it gets linked into
> a lot of C programs).  Sorry :(  

Okay, I will do this as separate patches so as to not mess up interpatch diffs.

https://codereview.chromium.org/608913003/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_irt_ext.cc:25: (void) ret;  
         \
On 2014/09/30 20:49:03, Sam Clegg wrote:
> Use __attribute__ unused?

Done.

[David Yen, 2014-10-01 20:01:44]

I've rewrote kernel_wrap_irt_ext to be in C, PTAL.

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

https://codereview.chromium.org/608913003/diff/80001/native_client_sdk/src/li...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_irt_ext.cc:2: // Use of this
source code is governed by a BSD-style license that can be
On 2014/09/30 21:37:36, David Yen wrote:
> On 2014/09/30 20:49:03, Sam Clegg wrote:
> > You're not going to like this but... do you think you do this in C
> > rather than C++.  We are currently in the process of trying to remove
> > all the C++ from nacl_io for various reasons (e.g. it gets linked into
> > a lot of C programs).  Sorry :(  
> 
> Okay, I will do this as separate patches so as to not mess up interpatch
diffs.

Done.

[binji, 2014-10-02 17:54:33]

binji@chromium.org changed reviewers:
+ binji@chromium.org

[binji, 2014-10-02 17:54:34]

https://codereview.chromium.org/608913003/diff/180001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc (right):

https://codereview.chromium.org/608913003/diff/180001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc:9: #if
defined(WIN32) || (!defined(NACL_IO_IRT_EXT) && \
I don't think you want this one. This file will only be included for host
builds, where the definition of NACL_IO_IRT_EXT will be irrelevant.

Oh, I see. You are defining NACL_IO_IRT_EXT dependently in kernel_wrap.h... I
think it might be nicer to have that logic in the source file, rather than
distributing between here and the kernel_wrap header.

https://codereview.chromium.org/608913003/diff/180001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_irt_ext.c (right):

https://codereview.chromium.org/608913003/diff/180001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_irt_ext.c:2: * Copyright (c)
2014 The Native Client Authors. All rights reserved.
Chromium authors. Copy this from one of the other C files

https://codereview.chromium.org/608913003/diff/180001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_irt_ext.c:85: false,
this looks strange. Maybe reorder the fields above so this gets initialized to
false automatically?

https://codereview.chromium.org/608913003/diff/180001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_irt_ext.c:172: return
s_irt_memory.interface.mmap(addr, len, prot, flags, fd, off);
nit: this is a bit clearer in kernel_wrap_newlib, IMO. Can you use a macro or
something here to make it clear that this is calling into the original
interface?

https://codereview.chromium.org/608913003/diff/180001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_irt_ext.c:490:
EXT_SUPPLY_INTERFACE_ASSERT(s_irt_dev_fdio,
should this not assert if the interface doesn't exist?

[David Yen, 2014-10-02 18:23:49]

https://codereview.chromium.org/608913003/diff/180001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc (right):

https://codereview.chromium.org/608913003/diff/180001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc:9: #if
defined(WIN32) || (!defined(NACL_IO_IRT_EXT) && \
On 2014/10/02 17:54:34, binji wrote:
> I don't think you want this one. This file will only be included for host
> builds, where the definition of NACL_IO_IRT_EXT will be irrelevant.
> 
> Oh, I see. You are defining NACL_IO_IRT_EXT dependently in kernel_wrap.h... I
> think it might be nicer to have that logic in the source file, rather than
> distributing between here and the kernel_wrap header.

I'll revert this file, the !defined(__native_client__) check should be
sufficient to turn it off when NACL_IO_IRT_EXT is on.

https://codereview.chromium.org/608913003/diff/180001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_irt_ext.c (right):

https://codereview.chromium.org/608913003/diff/180001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_irt_ext.c:2: * Copyright (c)
2014 The Native Client Authors. All rights reserved.
On 2014/10/02 17:54:34, binji wrote:
> Chromium authors. Copy this from one of the other C files

Done.

https://codereview.chromium.org/608913003/diff/180001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_irt_ext.c:85: false,
On 2014/10/02 17:54:34, binji wrote:
> this looks strange. Maybe reorder the fields above so this gets initialized to
> false automatically?

Done.

https://codereview.chromium.org/608913003/diff/180001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_irt_ext.c:172: return
s_irt_memory.interface.mmap(addr, len, prot, flags, fd, off);
On 2014/10/02 17:54:34, binji wrote:
> nit: this is a bit clearer in kernel_wrap_newlib, IMO. Can you use a macro or
> something here to make it clear that this is calling into the original
> interface?

The newlib version just uses pointers so this one would have to include the name
of the structure, which would look something like "REAL(s_irt_memory, mmap)" vs
"s_irt_memory.interface.mmap". I am not sure that it is any clearer other than
the word "real" is not part of the variable name. Should I just rename
s_irt_memory to s_real_irt_memory or something?

All things considered, I would rather the usage of a pointer within a structure
be clear instead of being hidden behind a macro.

https://codereview.chromium.org/608913003/diff/180001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_irt_ext.c:490:
EXT_SUPPLY_INTERFACE_ASSERT(s_irt_dev_fdio,
On 2014/10/02 17:54:34, binji wrote:
> should this not assert if the interface doesn't exist?

The details of this case is a bit tricky, we will probably have a better grasp
of what to do once more C Libraries support implement the API. My intuition is
that the C Standard Library should still accept it if the IRT didn't supply it,
since the C library is built independently of the IRT.

The thing I'm not sure about is how different libraries go back to an interface
that shouldn't exist. In the newlib case before any call it checks if the
pointers are NULL, so at least for newlib this is doing the right thing.

[David Yen, 2014-10-02 18:26:15]

https://codereview.chromium.org/608913003/diff/180001/native_client_sdk/src/l...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc (right):

https://codereview.chromium.org/608913003/diff/180001/native_client_sdk/src/l...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc:9: #if
defined(WIN32) || (!defined(NACL_IO_IRT_EXT) && \
On 2014/10/02 18:23:49, David Yen wrote:
> On 2014/10/02 17:54:34, binji wrote:
> > I don't think you want this one. This file will only be included for host
> > builds, where the definition of NACL_IO_IRT_EXT will be irrelevant.
> > 
> > Oh, I see. You are defining NACL_IO_IRT_EXT dependently in kernel_wrap.h...
I
> > think it might be nicer to have that logic in the source file, rather than
> > distributing between here and the kernel_wrap header.
> 
> I'll revert this file, the !defined(__native_client__) check should be
> sufficient to turn it off when NACL_IO_IRT_EXT is on.

Note that NACL_IO_IRT_EXT is meant to be able to replace every kernel_wrap_**
file, so it should be shared across all the source files.

[Sam Clegg, 2014-10-08 19:58:20]

this lgtm now.  Ben?

[binji, 2014-10-08 21:08:14]

sorry! lgtm

[David Yen, 2014-10-16 18:09:22]

The CQ bit was checked by dyen@chromium.org

[commit-bot: I haz the power, 2014-10-16 18:11:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/608913003/540001

[commit-bot: I haz the power, 2014-10-16 18:32:20]

Committed patchset #20 (id:540001)

[commit-bot: I haz the power, 2014-10-16 18:35:09]

Patchset 20 (id:??) landed as
https://crrev.com/3183230ac54a9b443f6e67a64121a0c3ee734e6b
Cr-Commit-Position: refs/heads/master@{#299938}