Non-SFI mode: Build nacl_helper_nonsfi_unittests

Non-SFI mode: Build nacl_helper_nonsfi_unittests

This CL starts to build nacl_helper_nonsfi_unittests.
The binary is not yet enabled to run on bots. It will be done in a following CL.

TEST=Ran locally. Ran build bots. Ran build bots with editing configuration to include nacl_helper_nonsfi_unittests.
BUG=358465
Committed: https://crrev.com/72c418b09bbf90505f54df2b76285650acd97960
Cr-Commit-Position: refs/heads/master@{#330069}

[hidehiko, 2015-05-12 09:55:37]

hidehiko@chromium.org changed reviewers:
+ mdempsky@chromium.org, mseaborn@chromium.org

[hidehiko, 2015-05-12 09:55:38]

Could you take a look?

mdempsky@: sandbox/...
mseaborn@: anything other.

FYI: ran the test on bot with local change:
https://codereview.chromium.org/1133483002
Some bots are red due to buildbot config incompleteness, but the
nacl_helper_nonsfi_unittests runs successfully.

This CL just starts to build the unittest binary. It will run on bots by
following CL.

Thank you for your review in advance,
- hidehiko

[hidehiko, 2015-05-12 12:25:10]

hidehiko@chromium.org changed reviewers:
+ thakis@chromium.org

[hidehiko, 2015-05-12 12:25:10]

R+=thakis@.
Could you review build/all.gyp as an OWNER?

Thanks!
- hidehiko

[Nico, 2015-05-12 16:00:51]

all.gyp lgtm

do you need to change any .gn files?

[mdempsky, 2015-05-12 17:23:29]

lgtm for sandbox/

[Mark Seaborn, 2015-05-12 18:07:37]

Nit: "will be done" in commit message.  LGTM.

https://codereview.chromium.org/1137553003/diff/20001/components/nacl/loader/...
File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right):

https://codereview.chromium.org/1137553003/diff/20001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:46: #if
!defined(F_DUPFD_CLOEXEC)
I notice we're accumulating various Linux #defines like these in different
files.

e.g.
components/nacl/loader/nonsfi/nonsfi_sandbox.cc:#if !defined(MAP_STACK)

Would it be worth creating a catch-all file in sandbox/linux/system_headers/ to
put these in (on an as-needed basis)?

(Otherwise, in the absence of an "#ifdef OS_NACL_NONSFI" around these, I'd
suggest putting in a comment to say explicitly that these are for building with
the PNaCl toolchain.)

https://codereview.chromium.org/1137553003/diff/20001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:66: #if
defined(__i386__)
Nit: Since this wraps a large block I'd put an empty line after the "#if" (and
before the "#endif") for readability.

(At least in the absence of indentation.)

https://codereview.chromium.org/1137553003/diff/20001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:431: // fcntl in PNaCl
toolchian returns an error without calling actual system
"toolchain"

https://codereview.chromium.org/1137553003/diff/20001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:478: // here, unlike
host toolchain build (nacl_loader_unittests).
Nit: by "here" do you mean "by the test runner"?

What's the difference that leads AtExitManager to be instantiated by the test
runner in the host toolchain build but not the PNaCl toolchain build?  Just
curious.

https://codereview.chromium.org/1137553003/diff/20001/components/nacl_nonsfi.gyp
File components/nacl_nonsfi.gyp (right):

https://codereview.chromium.org/1137553003/diff/20001/components/nacl_nonsfi....
components/nacl_nonsfi.gyp:183: # and put it directly under
out/{Debug,Release}/.
Nit: maybe add "so that this is in the standard location, for running on the
buildbots"?  I assume that's the reason for doing this?

https://codereview.chromium.org/1137553003/diff/20001/sandbox/linux/tests/uni...
File sandbox/linux/tests/unit_tests.cc (right):

https://codereview.chromium.org/1137553003/diff/20001/sandbox/linux/tests/uni...
sandbox/linux/tests/unit_tests.cc:24: #define POLLRDHUP 0x2000
See other comment: It would be nice to put this in a catch-all file in
sandbox/linux/system_headers/.

https://codereview.chromium.org/1137553003/diff/20001/sandbox/linux/tests/uni...
sandbox/linux/tests/unit_tests.cc:87: // So, for simplicity, just drop the
"timeout" feature from unittest framework.
You might say that we'll rely on Buildbot's timeout feature instead (so the bots
won't hang forever if a test hangs).

https://codereview.chromium.org/1137553003/diff/20001/sandbox/sandbox_nacl_no...
File sandbox/sandbox_nacl_nonsfi.gyp (right):

https://codereview.chromium.org/1137553003/diff/20001/sandbox/sandbox_nacl_no...
sandbox/sandbox_nacl_nonsfi.gyp:60: 'target_name':
'sandbox_test_utils_nacl_nonsfi',
Nit: should this be 'sandbox_linux_test_utils_nacl_nonsfi' to match the target
name in sandbox/linux/sandbox_linux.gypi?

(I know that's a bit long!)

[hidehiko, 2015-05-13 05:41:41]

Patchset #3 (id:40001) has been deleted

[hidehiko, 2015-05-13 05:43:15]

Thank you for review! Submitting.

https://codereview.chromium.org/1137553003/diff/20001/components/nacl/loader/...
File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right):

https://codereview.chromium.org/1137553003/diff/20001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:46: #if
!defined(F_DUPFD_CLOEXEC)
On 2015/05/12 18:07:37, Mark Seaborn wrote:
> I notice we're accumulating various Linux #defines like these in different
> files.
> 
> e.g.
> components/nacl/loader/nonsfi/nonsfi_sandbox.cc:#if !defined(MAP_STACK)
> 
> Would it be worth creating a catch-all file in sandbox/linux/system_headers/
to
> put these in (on an as-needed basis)?
> 
> (Otherwise, in the absence of an "#ifdef OS_NACL_NONSFI" around these, I'd
> suggest putting in a comment to say explicitly that these are for building
with
> the PNaCl toolchain.)

Hmm, it's up to sandbox/ team. Instead, I added comments in this CL.

https://codereview.chromium.org/1137553003/diff/20001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:66: #if
defined(__i386__)
On 2015/05/12 18:07:37, Mark Seaborn wrote:
> Nit: Since this wraps a large block I'd put an empty line after the "#if" (and
> before the "#endif") for readability.
> 
> (At least in the absence of indentation.)

Done.

https://codereview.chromium.org/1137553003/diff/20001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:431: // fcntl in PNaCl
toolchian returns an error without calling actual system
On 2015/05/12 18:07:37, Mark Seaborn wrote:
> "toolchain"

Done.

https://codereview.chromium.org/1137553003/diff/20001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:478: // here, unlike
host toolchain build (nacl_loader_unittests).
On 2015/05/12 18:07:37, Mark Seaborn wrote:
> Nit: by "here" do you mean "by the test runner"?
> 
> What's the difference that leads AtExitManager to be instantiated by the test
> runner in the host toolchain build but not the PNaCl toolchain build?  Just
> curious.

Done. The test runner for nacl_loader_unittests is base::LaunchUnitTest, though
it is gtest_main for this.

https://codereview.chromium.org/1137553003/diff/20001/components/nacl_nonsfi.gyp
File components/nacl_nonsfi.gyp (right):

https://codereview.chromium.org/1137553003/diff/20001/components/nacl_nonsfi....
components/nacl_nonsfi.gyp:183: # and put it directly under
out/{Debug,Release}/.
On 2015/05/12 18:07:37, Mark Seaborn wrote:
> Nit: maybe add "so that this is in the standard location, for running on the
> buildbots"?  I assume that's the reason for doing this?

Done.

https://codereview.chromium.org/1137553003/diff/20001/sandbox/linux/tests/uni...
File sandbox/linux/tests/unit_tests.cc (right):

https://codereview.chromium.org/1137553003/diff/20001/sandbox/linux/tests/uni...
sandbox/linux/tests/unit_tests.cc:24: #define POLLRDHUP 0x2000
On 2015/05/12 18:07:37, Mark Seaborn wrote:
> See other comment: It would be nice to put this in a catch-all file in
> sandbox/linux/system_headers/.

Done.

https://codereview.chromium.org/1137553003/diff/20001/sandbox/linux/tests/uni...
sandbox/linux/tests/unit_tests.cc:87: // So, for simplicity, just drop the
"timeout" feature from unittest framework.
On 2015/05/12 18:07:37, Mark Seaborn wrote:
> You might say that we'll rely on Buildbot's timeout feature instead (so the
bots
> won't hang forever if a test hangs).

Done.

https://codereview.chromium.org/1137553003/diff/20001/sandbox/sandbox_nacl_no...
File sandbox/sandbox_nacl_nonsfi.gyp (right):

https://codereview.chromium.org/1137553003/diff/20001/sandbox/sandbox_nacl_no...
sandbox/sandbox_nacl_nonsfi.gyp:60: 'target_name':
'sandbox_test_utils_nacl_nonsfi',
On 2015/05/12 18:07:37, Mark Seaborn wrote:
> Nit: should this be 'sandbox_linux_test_utils_nacl_nonsfi' to match the target
> name in sandbox/linux/sandbox_linux.gypi?
> 
> (I know that's a bit long!)

Done.

[hidehiko, 2015-05-13 05:43:19]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2015-05-13 05:43:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
mseaborn@chromium.org, thakis@chromium.org, mdempsky@chromium.org
Link to the patchset: https://codereview.chromium.org/1137553003/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-05-13 05:43:49]

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

[commit-bot: I haz the power, 2015-05-13 05:50:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-13 05:50:20]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[hidehiko, 2015-05-13 05:53:47]

hidehiko@chromium.org changed reviewers:
+ jln@chromium.org

[hidehiko, 2015-05-13 05:53:48]

R+=jln@.
Could you review sandbox/sandbox_nacl_nonsfi.gyp as an OWNER?

Thanks,
- hidehiko

[jln (very slow on Chromium), 2015-05-14 18:34:55]

On 2015/05/13 05:53:48, hidehiko wrote:
> R+=jln@.
> Could you review sandbox/sandbox_nacl_nonsfi.gyp as an OWNER?

sandbox/sandbox_nacl_nonsfi.gyp lgtm, but...

Somehow I had missed the creation of sandbox/sandbox_nacl_nonsfi.gyp in
https://codereview.chromium.org/888903004.

- This, at the very least belongs to sandbox/linux/ rather than sandbox/. Could
you move it there (in another CL)?

- I'm concerned about the duplication of file names there. Things will break
very randomly as there is no guarantee that these files won't depend on files
not included here.
We should all think about ways to clean this up. I think it's probably ok short
term while non-SFI is moving fast, but we definitely need to clean this at some
point.

[hidehiko, 2015-05-15 05:27:05]

Thank you for review. Submitting.

On 2015/05/14 18:34:55, jln wrote:
> On 2015/05/13 05:53:48, hidehiko wrote:
> > R+=jln@.
> > Could you review sandbox/sandbox_nacl_nonsfi.gyp as an OWNER?
> 
> sandbox/sandbox_nacl_nonsfi.gyp lgtm, but...
> 
> Somehow I had missed the creation of sandbox/sandbox_nacl_nonsfi.gyp in
> https://codereview.chromium.org/888903004.
> 
> - This, at the very least belongs to sandbox/linux/ rather than sandbox/.
Could
> you move it there (in another CL)?

Sure. Let me send another CL.

> 
> - I'm concerned about the duplication of file names there. Things will break
> very randomly as there is no guarantee that these files won't depend on files
> not included here.
> We should all think about ways to clean this up. I think it's probably ok
short
> term while non-SFI is moving fast, but we definitely need to clean this at
some
> point.

[hidehiko, 2015-05-15 05:27:09]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2015-05-15 05:27:28]

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

[commit-bot: I haz the power, 2015-05-15 06:36:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-15 06:36:51]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hidehiko, 2015-05-15 07:09:25]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2015-05-15 07:09:53]

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

[commit-bot: I haz the power, 2015-05-15 07:13:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-15 07:13:38]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hidehiko, 2015-05-15 07:19:51]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2015-05-15 07:19:59]

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

[commit-bot: I haz the power, 2015-05-15 07:23:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-15 07:23:56]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hidehiko, 2015-05-15 09:26:24]

On 2015/05/15 07:23:56, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

Rebased. Resubmitting.

[hidehiko, 2015-05-15 09:26:30]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2015-05-15 09:26:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
mseaborn@chromium.org, thakis@chromium.org, jln@chromium.org,
mdempsky@chromium.org
Link to the patchset: https://codereview.chromium.org/1137553003/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2015-05-15 09:26:36]

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

[commit-bot: I haz the power, 2015-05-15 09:30:08]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-05-15 09:31:04]

Patchset 4 (id:??) landed as
https://crrev.com/72c418b09bbf90505f54df2b76285650acd97960
Cr-Commit-Position: refs/heads/master@{#330069}

[hans, 2015-05-15 16:59:50]

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

[hans, 2015-05-15 16:59:51]

The Clang ToT ASan buildbot started failing after this landed:
http://build.chromium.org/p/chromium.fyi/builders/ClangToTLinuxASan/builds/11...


FAILED: cd ../../sandbox; python ../native_client/build/build_nexe.py --root ..
--product-dir ../out/Release/xyz --config-name Release -t
../native_client/toolchain/ --arch x86-32-nonsfi --build newlib_nlib_pnacl
--name
../out/Release/gen/tc_nonsfi_helper/lib32/libsandbox_linux_test_utils_nacl_nonsfi.a
--objdir
../out/Release/obj/sandbox/sandbox_linux_test_utils_nacl_nonsfi.gen/nonsfi_helper-x86-32-nonsfi/sandbox_linux_test_utils_nacl_nonsfi
"--include-dirs=../out/Release/gen/tc_newlib/include
../native_client/src/public/linux_syscalls .. \"../out/Release/gen\"
../testing/gtest/include" "--compile_flags=--target=i686-unknown-nacl
--pnacl-bias=x86-32-nonsfi --pnacl-allow-translate --pnacl-allow-native -arch
x86-32-nonsfi -O2 -g -Wall -fdiagnostics-show-option \"\"  -Wno-unused-function
-Wno-char-subscripts -Wno-c++11-extensions -Wno-unnamed-type-template-args
-Wno-extra-semi -Wno-unused-private-field -Wno-char-subscripts
-Wno-unused-function \"-std=gnu++11\" " --gomadir ""
"--defines=\"__STDC_LIMIT_MACROS=1\" \"__STDC_FORMAT_MACROS=1\"
\"_GNU_SOURCE=1\" \"_DEFAULT_SOURCE=1\" \"_BSD_SOURCE=1\"
\"_POSIX_C_SOURCE=199506\" \"_XOPEN_SOURCE=600\"
\"DYNAMIC_ANNOTATIONS_ENABLED=1\" \"DYNAMIC_ANNOTATIONS_PREFIX=NACL_\"
\"NACL_BUILD_ARCH=x86\" V8_DEPRECATION_WARNINGS \"_FILE_OFFSET_BITS=64\"
NO_TCMALLOC CHROMIUM_BUILD \"CR_CLANG_REVISION=237444\" \"TOOLKIT_VIEWS=1\"
UI_COMPOSITOR_IMAGE_TRANSPORT \"USE_AURA=1\" \"USE_ASH=1\" \"USE_PANGO=1\"
\"USE_CAIRO=1\" \"USE_DEFAULT_RENDER_THEME=1\" \"USE_LIBJPEG_TURBO=1\"
\"USE_X11=1\" \"USE_CLIPBOARD_AURAX11=1\" ENABLE_ONE_CLICK_SIGNIN
ENABLE_PRE_SYNC_BACKUP \"ENABLE_REMOTING=1\" \"ENABLE_WEBRTC=1\"
\"ENABLE_MEDIA_ROUTER=1\" ENABLE_PEPPER_CDMS ENABLE_CONFIGURATION_POLICY
ENABLE_NOTIFICATIONS \"ENABLE_HIDPI=1\" USE_UDEV DONT_EMBED_BUILD_METADATA
ADDRESS_SANITIZER MEMORY_TOOL_REPLACES_ALLOCATOR MEMORY_SANITIZER_INITIAL_SIZE
\"ENABLE_TASK_MANAGER=1\" \"ENABLE_EXTENSIONS=1\" \"ENABLE_PLUGINS=1\"
\"ENABLE_SESSION_SERVICE=1\" \"ENABLE_THEMES=1\" \"ENABLE_AUTOFILL_DIALOG=1\"
\"ENABLE_BACKGROUND=1\" \"ENABLE_GOOGLE_NOW=1\" \"CLD_VERSION=2\"
\"ENABLE_PRINTING=1\" \"ENABLE_BASIC_PRINTING=1\" \"ENABLE_PRINT_PREVIEW=1\"
\"ENABLE_SPELLCHECK=1\" \"ENABLE_CAPTIVE_PORTAL_DETECTION=1\"
\"ENABLE_APP_LIST=1\" \"ENABLE_SETTINGS_APP=1\" \"ENABLE_SUPERVISED_USERS=1\"
\"ENABLE_MDNS=1\" \"ENABLE_SERVICE_DISCOVERY=1\" V8_USE_EXTERNAL_STARTUP_DATA
FULL_SAFE_BROWSING SAFE_BROWSING_CSD SAFE_BROWSING_DB_LOCAL
SAFE_BROWSING_SERVICE \"GTEST_HAS_POSIX_RE=0\" \"GTEST_LANG_CXX11=0\" UNIT_TEST
\"GTEST_HAS_RTTI=0\" \"USE_LIBPCI=1\" \"USE_OPENSSL=1\" \"USE_OPENSSL_CERTS=1\"
__STDC_CONSTANT_MACROS __STDC_FORMAT_MACROS LEAK_SANITIZER
\"WTF_USE_LEAK_SANITIZER=1\""
"--link_flags=-B../out/Release/gen/tc_nonsfi_helper/lib32  -lgtest_nacl"
"--source-list=../out/gypfiles/sandbox/nonsfi_helper-x86-32-nonsfi.sandbox_linux_test_utils_nacl_nonsfi.source_list.gypcmd"
linux/tests/unit_tests.cc:193:7: error: use of undeclared identifier
'__lsan_do_leak_check'
      __lsan_do_leak_check();
      ^
1 error generated.

Do you think it could be related?

[mdempsky, 2015-05-15 17:59:30]

On 2015/05/15 16:59:51, hans wrote:
> Do you think it could be related?

Probably. :(

[hans, 2015-05-15 18:39:39]

Repro steps:

$ GYP_DEFINES="asan=1 clang=1 component=static_library lsan=1 target_arch=x64"
gclient runhooks
$ ninja -C out/Release
gen/tc_nonsfi_helper/lib32/libsandbox_linux_test_utils_nacl_nonsfi.a

[hans, 2015-05-15 18:42:40]

On 2015/05/15 18:39:39, hans wrote:
> Repro steps:
> 
> $ GYP_DEFINES="asan=1 clang=1 component=static_library lsan=1 target_arch=x64"
> gclient runhooks
> $ ninja -C out/Release
> gen/tc_nonsfi_helper/lib32/libsandbox_linux_test_utils_nacl_nonsfi.a

Verified that reverting this fixes the build locally.

[hans, 2015-05-15 18:43:05]

A revert of this CL (patchset #4 id:80001) has been created in
https://codereview.chromium.org/1145593002/ by hans@chromium.org.

The reason for reverting is: This breaks the asan build. See comments on the
original CL..