[NaCl SDK] Enable linux host build for nacl_io and nacl_io_tests.

[NaCl SDK] Enable linux host build for nacl_io and nacl_io_tests.

The process of doing this already found one serious bug
in lseek and will enable us to find many more via asan and
other sanitisers which run on the linux host envionment.

R=binji@chromium.org, binji

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

[Sam Clegg, 2013-11-15 03:11:54]

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/kernel_handle.cc (right):

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_handle.cc:80: if (base + offset <
0)
This was always failing because base was is unsigned.  Not sure why clang didn't
warn us about this before.

[binji, 2013-11-15 17:26:23]

Is the plan to use ASAN w/ GCC? Or to add a host_llvm.mk and use clang on Linux?

Anyway, lgtm

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/event_listener.cc (right):

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:21: #ifdef WIN32
defined(WIN32)

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:38: static uint64_t
usec_since_epoch() {
thanks for moving this, it didn't make sense to put it in kernel_wrap_*

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/kernel_handle.cc (right):

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_handle.cc:80: if (base + offset <
0)
On 2013/11/15 03:11:55, Sam Clegg wrote:
> This was always failing because base was is unsigned.  Not sure why clang
didn't
> warn us about this before.

scary, nice catch.

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc (right):

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc:1: #if defined
WIN32 || defined __linux__
copyright

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc:1: #if defined
WIN32 || defined __linux__
in the other files we use defined(...)

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc:8: // "real"
functions, i.e. the unwrapped original functions. On Windows we don't
change comment?

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc:10: 
The linux host implementation is a bit different than the windows host was.
We're not saying that it is usable (because we don't wrap -- not even with
linker tricks), but we are saying that we use it for testing. Can we document
this somewhere?

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/library.dsc (right):

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/library.dsc:33: "kernel_wrap_dummy.cc",
alphabetize

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/mount_node_mem.cc (right):

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/mount_node_mem.cc:45: if (attr.offs +
count > static_cast<size_t>(size)) {
Why is this necessary? size is already a size_t.

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/ossignal.h (right):

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/ossignal.h:8: #ifndef WIN23
use #if !defined(WIN32), like the other files

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/ppapi_simple/library.dsc (right):

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/ppapi_simple/library.dsc:2: 'TOOLS': ['newlib',
'glibc', 'pnacl', 'linux'],
not sure this is a good idea -- the first call to mount will call the real mount
function, right?

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/tests/n...
File native_client_sdk/src/tests/nacl_io_test/kernel_wrap_test.cc (right):

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/tests/n...
native_client_sdk/src/tests/nacl_io_test/kernel_wrap_test.cc:5: #ifndef
__linux__
!defined(__linux__)

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/tools/c...
File native_client_sdk/src/tools/common.mk (right):

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/tools/c...
native_client_sdk/src/tools/common.mk:300: HOST_EXT =
Ah, sneaky. SEL_LDR build is just a normal build... it's convenient, but I'm not
sure I like it; don't you think it is weird to build

make TOOLCHAIN=linux SEL_LDR=1

and get a native binary that can't be run via sel_ldr? Maybe if we change
SEL_LDR to COMMAND_LINE or something?

[Sam Clegg, 2013-11-15 18:13:33]

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/event_listener.cc (right):

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/event_listener.cc:21: #ifdef WIN32
On 2013/11/15 17:26:23, binji wrote:
> defined(WIN32)

Done.

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc (right):

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc:1: #if defined
WIN32 || defined __linux__
On 2013/11/15 17:26:23, binji wrote:
> copyright

Done.

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc:1: #if defined
WIN32 || defined __linux__
On 2013/11/15 17:26:23, binji wrote:
> in the other files we use defined(...)

Done.

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc:8: // "real"
functions, i.e. the unwrapped original functions. On Windows we don't
On 2013/11/15 17:26:23, binji wrote:
> change comment?

Done.

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/mount_node_mem.cc (right):

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/mount_node_mem.cc:45: if (attr.offs +
count > static_cast<size_t>(size)) {
On 2013/11/15 17:26:23, binji wrote:
> Why is this necessary? size is already a size_t.

Done.

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/nacl_io/ossignal.h (right):

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/nacl_io/ossignal.h:8: #ifndef WIN23
On 2013/11/15 17:26:23, binji wrote:
> use #if !defined(WIN32), like the other files

Done.

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/ppapi_simple/library.dsc (right):

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/ppapi_simple/library.dsc:2: 'TOOLS': ['newlib',
'glibc', 'pnacl', 'linux'],
On 2013/11/15 17:26:23, binji wrote:
> not sure this is a good idea -- the first call to mount will call the real
mount
> function, right?

No because we intercept mount directly in syscalls/mount.c.

It only the function like fopen/fclose/etc where we intercept at the low
level that we will get failures.

For sure nacl_io will not work as expected under linux but enabling the
linux build enables us to do all this great testing.

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/tests/n...
File native_client_sdk/src/tests/nacl_io_test/kernel_wrap_test.cc (right):

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/tests/n...
native_client_sdk/src/tests/nacl_io_test/kernel_wrap_test.cc:5: #ifndef
__linux__
On 2013/11/15 17:26:23, binji wrote:
> !defined(__linux__)

Done.

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/tools/c...
File native_client_sdk/src/tools/common.mk (right):

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/tools/c...
native_client_sdk/src/tools/common.mk:300: HOST_EXT =
On 2013/11/15 17:26:23, binji wrote:
> Ah, sneaky. SEL_LDR build is just a normal build... it's convenient, but I'm
not
> sure I like it; don't you think it is weird to build
> 
> make TOOLCHAIN=linux SEL_LDR=1
> 
> and get a native binary that can't be run via sel_ldr? Maybe if we change
> SEL_LDR to COMMAND_LINE or something?


I agree.  How about STANDALONE?

[Sam Clegg, 2013-11-15 18:14:55]

On 2013/11/15 17:26:23, binji wrote:
> Is the plan to use ASAN w/ GCC? Or to add a host_llvm.mk and use clang on
Linux?

Step 1 is just to get all the tests running and passing on desktop linux,
which this change does.

asan, valgrind, etc, can come next.


> 
> Anyway, lgtm
> 
>
https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
> File native_client_sdk/src/libraries/nacl_io/event_listener.cc (right):
> 
>
https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
> native_client_sdk/src/libraries/nacl_io/event_listener.cc:21: #ifdef WIN32
> defined(WIN32)
> 
>
https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
> native_client_sdk/src/libraries/nacl_io/event_listener.cc:38: static uint64_t
> usec_since_epoch() {
> thanks for moving this, it didn't make sense to put it in kernel_wrap_*
> 
>
https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
> File native_client_sdk/src/libraries/nacl_io/kernel_handle.cc (right):
> 
>
https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
> native_client_sdk/src/libraries/nacl_io/kernel_handle.cc:80: if (base + offset
<
> 0)
> On 2013/11/15 03:11:55, Sam Clegg wrote:
> > This was always failing because base was is unsigned.  Not sure why clang
> didn't
> > warn us about this before.
> 
> scary, nice catch.
> 
>
https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
> File native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc (right):
> 
>
https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
> native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc:1: #if defined
> WIN32 || defined __linux__
> copyright
> 
>
https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
> native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc:1: #if defined
> WIN32 || defined __linux__
> in the other files we use defined(...)
> 
>
https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
> native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc:8: // "real"
> functions, i.e. the unwrapped original functions. On Windows we don't
> change comment?
> 
>
https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
> native_client_sdk/src/libraries/nacl_io/kernel_wrap_dummy.cc:10: 
> The linux host implementation is a bit different than the windows host was.
> We're not saying that it is usable (because we don't wrap -- not even with
> linker tricks), but we are saying that we use it for testing. Can we document
> this somewhere?
> 
>
https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
> File native_client_sdk/src/libraries/nacl_io/library.dsc (right):
> 
>
https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
> native_client_sdk/src/libraries/nacl_io/library.dsc:33:
"kernel_wrap_dummy.cc",
> alphabetize
> 
>
https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
> File native_client_sdk/src/libraries/nacl_io/mount_node_mem.cc (right):
> 
>
https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
> native_client_sdk/src/libraries/nacl_io/mount_node_mem.cc:45: if (attr.offs +
> count > static_cast<size_t>(size)) {
> Why is this necessary? size is already a size_t.
> 
>
https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
> File native_client_sdk/src/libraries/nacl_io/ossignal.h (right):
> 
>
https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
> native_client_sdk/src/libraries/nacl_io/ossignal.h:8: #ifndef WIN23
> use #if !defined(WIN32), like the other files
> 
>
https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
> File native_client_sdk/src/libraries/ppapi_simple/library.dsc (right):
> 
>
https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
> native_client_sdk/src/libraries/ppapi_simple/library.dsc:2: 'TOOLS':
['newlib',
> 'glibc', 'pnacl', 'linux'],
> not sure this is a good idea -- the first call to mount will call the real
mount
> function, right?
> 
>
https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/tests/n...
> File native_client_sdk/src/tests/nacl_io_test/kernel_wrap_test.cc (right):
> 
>
https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/tests/n...
> native_client_sdk/src/tests/nacl_io_test/kernel_wrap_test.cc:5: #ifndef
> __linux__
> !defined(__linux__)
> 
>
https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/tools/c...
> File native_client_sdk/src/tools/common.mk (right):
> 
>
https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/tools/c...
> native_client_sdk/src/tools/common.mk:300: HOST_EXT =
> Ah, sneaky. SEL_LDR build is just a normal build... it's convenient, but I'm
not
> sure I like it; don't you think it is weird to build
> 
> make TOOLCHAIN=linux SEL_LDR=1
> 
> and get a native binary that can't be run via sel_ldr? Maybe if we change
> SEL_LDR to COMMAND_LINE or something?

[binji, 2013-11-15 18:21:17]

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
File native_client_sdk/src/libraries/ppapi_simple/library.dsc (right):

https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
native_client_sdk/src/libraries/ppapi_simple/library.dsc:2: 'TOOLS': ['newlib',
'glibc', 'pnacl', 'linux'],
On 2013/11/15 18:13:34, Sam Clegg wrote:
> On 2013/11/15 17:26:23, binji wrote:
> > not sure this is a good idea -- the first call to mount will call the real
> mount
> > function, right?
> 
> No because we intercept mount directly in syscalls/mount.c.
> 
> It only the function like fopen/fclose/etc where we intercept at the low
> level that we will get failures.
> 
> For sure nacl_io will not work as expected under linux but enabling the
> linux build enables us to do all this great testing.

Agreed, I'm just not sure we want to turn it out for ppapi_simple. Or is it
required because the tests use it?

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

https://codereview.chromium.org/73083005/diff/140001/native_client_sdk/src/to...
native_client_sdk/src/tools/nacl_gcc.mk:465: $(error Cannot run in sel_ldr
unless $$NACL_ARCH is set)
standalone

[Sam Clegg, 2013-11-15 18:26:21]

On 2013/11/15 18:21:17, binji wrote:
>
https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
> File native_client_sdk/src/libraries/ppapi_simple/library.dsc (right):
> 
>
https://codereview.chromium.org/73083005/diff/1/native_client_sdk/src/librari...
> native_client_sdk/src/libraries/ppapi_simple/library.dsc:2: 'TOOLS':
['newlib',
> 'glibc', 'pnacl', 'linux'],
> On 2013/11/15 18:13:34, Sam Clegg wrote:
> > On 2013/11/15 17:26:23, binji wrote:
> > > not sure this is a good idea -- the first call to mount will call the real
> > mount
> > > function, right?
> > 
> > No because we intercept mount directly in syscalls/mount.c.
> > 
> > It only the function like fopen/fclose/etc where we intercept at the low
> > level that we will get failures.
> > 
> > For sure nacl_io will not work as expected under linux but enabling the
> > linux build enables us to do all this great testing.
> 
> Agreed, I'm just not sure we want to turn it out for ppapi_simple. Or is it
> required because the tests use it?

It is required for the tests right now since they include it in their
deps.

ppapi_simple actually works fine under linux.  If anything its nacl_io that
should
be excluded since its is the thing that is non-function, except for testing.

Perhaps we just need to document that nacl_io is not usable under linux
host except for testing purposes?

> 
>
https://codereview.chromium.org/73083005/diff/140001/native_client_sdk/src/to...
> File native_client_sdk/src/tools/nacl_gcc.mk (right):
> 
>
https://codereview.chromium.org/73083005/diff/140001/native_client_sdk/src/to...
> native_client_sdk/src/tools/nacl_gcc.mk:465: $(error Cannot run in sel_ldr
> unless $$NACL_ARCH is set)
> standalone

[Sam Clegg, 2013-11-15 18:26:28]

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

https://codereview.chromium.org/73083005/diff/140001/native_client_sdk/src/to...
native_client_sdk/src/tools/nacl_gcc.mk:465: $(error Cannot run in sel_ldr
unless $$NACL_ARCH is set)
On 2013/11/15 18:21:18, binji wrote:
> standalone

Done.

[Sam Clegg, 2013-11-18 17:35:14]

Committed patchset #5 manually as r235753 (presubmit successful).