Subzero. ARM32. Implements sandboxing.

Subzero. ARM32. Implements sandboxing.

(See https://codereview.chromium.org/1491473002/ for the steps taken
while implementing sandboxing.)

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4076
R=stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=52b515717f437804b62c7d24f36414baaf15daa7

[John, 2015-12-04 17:14:05]

jpp@chromium.org changed reviewers:
+ kschimpf@google.com, sehr@chromium.org, stichnot@chromium.org

[John, 2015-12-04 17:14:05]

https://codereview.chromium.org/1499983002/diff/20001/pydir/crosstest_generat...
File pydir/crosstest_generator.py (right):

https://codereview.chromium.org/1499983002/diff/20001/pydir/crosstest_generat...
pydir/crosstest_generator.py:70: 'arm32': ['--filetype=asm'] }
This is broken for --sandbox

[Karl, 2015-12-04 20:41:17]

https://codereview.chromium.org/1499983002/diff/40001/pydir/run-pnacl-sz.py
File pydir/run-pnacl-sz.py (right):

https://codereview.chromium.org/1499983002/diff/40001/pydir/run-pnacl-sz.py#n...
pydir/run-pnacl-sz.py:95: help='Sanboxes the generated code.')
sanboxes?

https://codereview.chromium.org/1499983002/diff/40001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1499983002/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:5182: _ret(getPhysicalRegister(RegARM32::Reg_lr),
Reg);
Indentation wrong?

https://codereview.chromium.org/1499983002/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:5961: }
Add blank line?

https://codereview.chromium.org/1499983002/diff/40001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1499983002/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.h:944: void sub_sp(Operand *SubAmount);
How are AddAmount and SubAmount related?

https://codereview.chromium.org/1499983002/diff/40001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/sandboxing.ll (right):

https://codereview.chromium.org/1499983002/diff/40001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/sandboxing.ll:1: ; Tests basics and corner cases of
x86-32 sandboxing, using -Om1 in
x86-32? or is it arm32?

[Jim Stichnoth, 2015-12-04 22:51:54]

https://codereview.chromium.org/1499983002/diff/40001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1499983002/diff/40001/Makefile.standalone#new...
Makefile.standalone:408: -i arm32,sandbox,neon \
Instead of adding near-copies of the previous 3 lines, I think you can just
remove ",native" from the previous 3 lines.

It was split out for x86-32 because we wanted to control the amount of time
spent on running cross tests, and for reasons I can't quite recall cross tests
take substantially longer to build+run when sandboxing is involved.

https://codereview.chromium.org/1499983002/diff/40001/pydir/crosstest_generat...
File pydir/crosstest_generator.py (right):

https://codereview.chromium.org/1499983002/diff/40001/pydir/crosstest_generat...
pydir/crosstest_generator.py:70: 'arm32': ['--filetype=asm'] }
I know this is only temporary, but in the meantime I think there should be a
brief comment why it's not iasm (like the existing comment why it's not obj).

https://codereview.chromium.org/1499983002/diff/40001/pydir/run-pnacl-sz.py
File pydir/run-pnacl-sz.py (right):

https://codereview.chromium.org/1499983002/diff/40001/pydir/run-pnacl-sz.py#n...
pydir/run-pnacl-sz.py:20: flags = { 'x8632': ['-triple=%s' % ('i686' if not
sandboxed else 'i686-nacl')],
'i686-nacl' if sandboxed else 'i686'

this should also fix the 80-col problem

https://codereview.chromium.org/1499983002/diff/40001/pydir/run-pnacl-sz.py#n...
pydir/run-pnacl-sz.py:22: 'armv7a' if not sandboxed else 'armv7a-nacl'),
'armv7a-nacl' if sandboxed else 'armv7a'

https://codereview.chromium.org/1499983002/diff/40001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1499983002/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.h:825: /// AutoSandboxer defines methods for ensuring
that "dangerous" operations are
What do you think about calling this Sandboxer instead of AutoSandboxer?  The
latter makes me think of NaCl auto-sandboxing, where inline asm in C code is
automatically transformed to be sandbox-valid.

https://codereview.chromium.org/1499983002/diff/40001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/sandboxing.ll (right):

https://codereview.chromium.org/1499983002/diff/40001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/sandboxing.ll:4: ; minimal use of registers and stack
slots in the lowering sequence.
Maybe reflow to 80-col while you're at it?

https://codereview.chromium.org/1499983002/diff/40001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/sandboxing.ll:155: 
Remove trailing newline to silence git.

[John, 2015-12-05 16:20:11]

https://codereview.chromium.org/1499983002/diff/40001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1499983002/diff/40001/Makefile.standalone#new...
Makefile.standalone:408: -i arm32,sandbox,neon \
On 2015/12/04 22:51:54, stichnot wrote:
> Instead of adding near-copies of the previous 3 lines, I think you can just
> remove ",native" from the previous 3 lines.
> 
> It was split out for x86-32 because we wanted to control the amount of time
> spent on running cross tests, and for reasons I can't quite recall cross tests
> take substantially longer to build+run when sandboxing is involved.

Done.

https://codereview.chromium.org/1499983002/diff/40001/pydir/crosstest_generat...
File pydir/crosstest_generator.py (right):

https://codereview.chromium.org/1499983002/diff/40001/pydir/crosstest_generat...
pydir/crosstest_generator.py:70: 'arm32': ['--filetype=asm'] }
On 2015/12/04 22:51:54, stichnot wrote:
> I know this is only temporary, but in the meantime I think there should be a
> brief comment why it's not iasm (like the existing comment why it's not obj).

Done.

https://codereview.chromium.org/1499983002/diff/40001/pydir/run-pnacl-sz.py
File pydir/run-pnacl-sz.py (right):

https://codereview.chromium.org/1499983002/diff/40001/pydir/run-pnacl-sz.py#n...
pydir/run-pnacl-sz.py:20: flags = { 'x8632': ['-triple=%s' % ('i686' if not
sandboxed else 'i686-nacl')],
On 2015/12/04 22:51:54, stichnot wrote:
> 'i686-nacl' if sandboxed else 'i686'
> 
> this should also fix the 80-col problem

Done.

https://codereview.chromium.org/1499983002/diff/40001/pydir/run-pnacl-sz.py#n...
pydir/run-pnacl-sz.py:22: 'armv7a' if not sandboxed else 'armv7a-nacl'),
On 2015/12/04 22:51:54, stichnot wrote:
> 'armv7a-nacl' if sandboxed else 'armv7a'

Done.

https://codereview.chromium.org/1499983002/diff/40001/pydir/run-pnacl-sz.py#n...
pydir/run-pnacl-sz.py:95: help='Sanboxes the generated code.')
On 2015/12/04 20:41:17, Karl wrote:
> sanboxes?

Done.

https://codereview.chromium.org/1499983002/diff/40001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1499983002/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:5182: _ret(getPhysicalRegister(RegARM32::Reg_lr),
Reg);
On 2015/12/04 20:41:17, Karl wrote:
> Indentation wrong?

where, the _ret? seems fine to me (there's an underscore that is a leading
underscore that is hard to see in the diffs.)

https://codereview.chromium.org/1499983002/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:5961: }
On 2015/12/04 20:41:17, Karl wrote:
> Add blank line?

Done.

https://codereview.chromium.org/1499983002/diff/40001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1499983002/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.h:825: /// AutoSandboxer defines methods for ensuring
that "dangerous" operations are
On 2015/12/04 22:51:54, stichnot wrote:
> What do you think about calling this Sandboxer instead of AutoSandboxer?  The
> latter makes me think of NaCl auto-sandboxing, where inline asm in C code is
> automatically transformed to be sandbox-valid.

Done.

https://codereview.chromium.org/1499983002/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.h:944: void sub_sp(Operand *SubAmount);
On 2015/12/04 20:41:17, Karl wrote:
> How are AddAmount and SubAmount related?

Done.

https://codereview.chromium.org/1499983002/diff/40001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/sandboxing.ll (right):

https://codereview.chromium.org/1499983002/diff/40001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/sandboxing.ll:1: ; Tests basics and corner cases of
x86-32 sandboxing, using -Om1 in
On 2015/12/04 20:41:17, Karl wrote:
> x86-32? or is it arm32?

Done.

https://codereview.chromium.org/1499983002/diff/40001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/sandboxing.ll:4: ; minimal use of registers and stack
slots in the lowering sequence.
On 2015/12/04 22:51:54, stichnot wrote:
> Maybe reflow to 80-col while you're at it?

Done.

https://codereview.chromium.org/1499983002/diff/40001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/sandboxing.ll:155: 
On 2015/12/04 22:51:54, stichnot wrote:
> Remove trailing newline to silence git.

Done.

[Jim Stichnoth, 2015-12-05 16:33:53]

lgtm

[John, 2015-12-05 22:16:28]

Description was changed from

==========
Subzero. ARM32. Implements sandboxing.

(See https://codereview.chromium.org/1491473002/ for the steps taken
while implementing sandboxing.)

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4076
==========

to

==========
Subzero. ARM32. Implements sandboxing.

(See https://codereview.chromium.org/1491473002/ for the steps taken
while implementing sandboxing.)

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4076
R=stichnot@chromium.org

Committed:
https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a...
==========

[John, 2015-12-05 22:16:29]

Committed patchset #4 (id:60001) manually as
52b515717f437804b62c7d24f36414baaf15daa7 (tree was closed).