Non-SFI mode: Remove old Non-SFI code.

Non-SFI mode: Remove old Non-SFI code.

The runtime for NaCl in Non-SFI mode is now nacl_helper_nonsfi,
and old nacl_helper' Non-SFI mode is deprecated.
This CL removes the old code.

TEST=Ran bots.
BUG=539866
Committed: https://crrev.com/241c6e0fcccb0b5986a8edc300d7a7d088c2639c
Cr-Commit-Position: refs/heads/master@{#354727}

[hidehiko, 2015-10-15 07:30:22]

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

[hidehiko, 2015-10-15 07:30:23]

PTAL.

[Mark Seaborn, 2015-10-15 18:10:11]

LGTM, thanks

https://codereview.chromium.org/1409633002/diff/1/components/nacl.gyp
File components/nacl.gyp (right):

https://codereview.chromium.org/1409633002/diff/1/components/nacl.gyp#newcode198
components/nacl.gyp:198: 'nacl_linux',
You might want to check whether these 2 dependencies are still needed.

https://codereview.chromium.org/1409633002/diff/1/components/nacl/BUILD.gn
File components/nacl/BUILD.gn (right):

https://codereview.chromium.org/1409633002/diff/1/components/nacl/BUILD.gn#ne...
components/nacl/BUILD.gn:220: "//ppapi/c",
Can ppapi/c and ppapi/shared_impl be removed too?

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/nacl...
File components/nacl/loader/nacl_helper_linux.cc (right):

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/nacl...
components/nacl/loader/nacl_helper_linux.cc:86: CHECK(uses_nonsfi_mode);
You could add:

#else
CHECK(!uses_nonsfi_mode);

or alternatively, remove the field 'uses_nonsfi_mode'.

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/nons...
File components/nacl/loader/nonsfi/nonsfi_listener.cc (right):

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/nons...
components/nacl/loader/nonsfi/nonsfi_listener.cc:24: #error "nonsfi_main.cc must
be built for nacl_helper_nonsfi."
"nonsfi_main.cc" -> "This file" (it's actually nonsfi_listener.cc)

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/nons...
File components/nacl/loader/nonsfi/nonsfi_main.cc (right):

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/nons...
components/nacl/loader/nonsfi/nonsfi_main.cc:18: 
Nit: don't add extra empty line

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/nons...
File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right):

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/nons...
components/nacl/loader/nonsfi/nonsfi_sandbox.cc:5: 
Nit: don't add empty line here

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/nons...
components/nacl/loader/nonsfi/nonsfi_sandbox.cc:187: // TODO(hamaji): newlib
does not use this. Make this SIGTRAP once
There are a few references to glibc and TODOs in this file.

Could you possibly address those (perhaps in a separate change)?

__NR_set_robust_list can definitely be taken out.

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/nons...
components/nacl/loader/nonsfi/nonsfi_sandbox.cc:312: case __NR_brk:
We should be able to remove brk.

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/nons...
File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right):

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/nons...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:569: // The libc
wrapper translates it to ENOMEM.
This comment applies to the code being removed, I think, so it can be removed
too.

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/sand...
File components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc (right):

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/sand...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:37: #include
"components/nacl/loader/nonsfi/nonsfi_sandbox.h"
Nit: I don't think you need to conditionalise this.

Generally, for simplicity, I would avoid conditionalising #includes if they work
unconditionalised.

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/sand...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:192:
CHECK(uses_nonsfi_mode) << "nacl_helper_nonsfi can run only Non-SFI plugin.";
Consistency nit: The #else branch doesn't have a message.  You could drop the
message.

[hidehiko, 2015-10-19 04:39:18]

Thank you for review. Submitting.

https://codereview.chromium.org/1409633002/diff/1/components/nacl.gyp
File components/nacl.gyp (right):

https://codereview.chromium.org/1409633002/diff/1/components/nacl.gyp#newcode198
components/nacl.gyp:198: 'nacl_linux',
On 2015/10/15 18:10:10, Mark Seaborn wrote:
> You might want to check whether these 2 dependencies are still needed.

Done.

https://codereview.chromium.org/1409633002/diff/1/components/nacl/BUILD.gn
File components/nacl/BUILD.gn (right):

https://codereview.chromium.org/1409633002/diff/1/components/nacl/BUILD.gn#ne...
components/nacl/BUILD.gn:220: "//ppapi/c",
On 2015/10/15 18:10:10, Mark Seaborn wrote:
> Can ppapi/c and ppapi/shared_impl be removed too?

Done.

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/nacl...
File components/nacl/loader/nacl_helper_linux.cc (right):

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/nacl...
components/nacl/loader/nacl_helper_linux.cc:86: CHECK(uses_nonsfi_mode);
On 2015/10/15 18:10:10, Mark Seaborn wrote:
> You could add:
> 
> #else
> CHECK(!uses_nonsfi_mode);
> 
> or alternatively, remove the field 'uses_nonsfi_mode'.

Done.

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/nons...
File components/nacl/loader/nonsfi/nonsfi_listener.cc (right):

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/nons...
components/nacl/loader/nonsfi/nonsfi_listener.cc:24: #error "nonsfi_main.cc must
be built for nacl_helper_nonsfi."
On 2015/10/15 18:10:10, Mark Seaborn wrote:
> "nonsfi_main.cc" -> "This file" (it's actually nonsfi_listener.cc)

Done.

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/nons...
File components/nacl/loader/nonsfi/nonsfi_main.cc (right):

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/nons...
components/nacl/loader/nonsfi/nonsfi_main.cc:18: 
On 2015/10/15 18:10:10, Mark Seaborn wrote:
> Nit: don't add extra empty line

Done.

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/nons...
File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right):

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/nons...
components/nacl/loader/nonsfi/nonsfi_sandbox.cc:5: 
On 2015/10/15 18:10:11, Mark Seaborn wrote:
> Nit: don't add empty line here

Done.

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/nons...
components/nacl/loader/nonsfi/nonsfi_sandbox.cc:187: // TODO(hamaji): newlib
does not use this. Make this SIGTRAP once
On 2015/10/15 18:10:11, Mark Seaborn wrote:
> There are a few references to glibc and TODOs in this file.
> 
> Could you possibly address those (perhaps in a separate change)?
> 
> __NR_set_robust_list can definitely be taken out.

I see. Let me give it a try on another CL.

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/nons...
components/nacl/loader/nonsfi/nonsfi_sandbox.cc:312: case __NR_brk:
On 2015/10/15 18:10:11, Mark Seaborn wrote:
> We should be able to remove brk.

Ditto.

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/nons...
File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right):

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/nons...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:569: // The libc
wrapper translates it to ENOMEM.
On 2015/10/15 18:10:11, Mark Seaborn wrote:
> This comment applies to the code being removed, I think, so it can be removed
> too.

Done.

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/sand...
File components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc (right):

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/sand...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:37: #include
"components/nacl/loader/nonsfi/nonsfi_sandbox.h"
On 2015/10/15 18:10:11, Mark Seaborn wrote:
> Nit: I don't think you need to conditionalise this.
> 
> Generally, for simplicity, I would avoid conditionalising #includes if they
work
> unconditionalised.

Done.

https://codereview.chromium.org/1409633002/diff/1/components/nacl/loader/sand...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:192:
CHECK(uses_nonsfi_mode) << "nacl_helper_nonsfi can run only Non-SFI plugin.";
On 2015/10/15 18:10:11, Mark Seaborn wrote:
> Consistency nit: The #else branch doesn't have a message.  You could drop the
> message.

Done.

[hidehiko, 2015-10-19 04:39:30]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2015-10-19 04:39:30]

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

[commit-bot: I haz the power, 2015-10-19 04:39:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1409633002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1409633002/40001

[commit-bot: I haz the power, 2015-10-19 05:30:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-19 05:30:55]

Patchset 3 (id:??) landed as
https://crrev.com/241c6e0fcccb0b5986a8edc300d7a7d088c2639c
Cr-Commit-Position: refs/heads/master@{#354727}