Change BKPT and UDF encodings on ARM.

Change BKPT and UDF encodings on ARM.

This only includes ARM validator changes and runtime changes. I'll have
to do LLVM and GCC changes separately.

BUG=https://code.google.com/p/nativeclient/issues/detail?id=2963 and 446
R=mcgrathr@chromium.org,kschimpf@chromium.org,mseaborn@chromium.org

Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=10074

[Mark Seaborn, 2012-10-18 01:21:39]

Filing an issue to track this would be useful.

There are other places to convert:

$ git grep -l 7777
...
tests/debug_stub/debugger_test.c
tests/faulted_thread_queue/faultqueue_test_guest.c
tests/syscall_return_sandboxing/sandboxed_arm.S

https://codereview.chromium.org/11194045/diff/1/src/trusted/debug_stub/abi.cc
File src/trusted/debug_stub/abi.cc (right):

https://codereview.chromium.org/11194045/diff/1/src/trusted/debug_stub/abi.cc...
src/trusted/debug_stub/abi.cc:133: static uint32_t breakpoint_code_arm[] = {
0xE125BE7F /* BKPT 0x5BEF */ };
Can this reuse NACL_HALT_OPCODE?

https://codereview.chromium.org/11194045/diff/1/src/trusted/service_runtime/a...
File src/trusted/service_runtime/arch/arm/nacl_text_pad_test.S (right):

https://codereview.chromium.org/11194045/diff/1/src/trusted/service_runtime/a...
src/trusted/service_runtime/arch/arm/nacl_text_pad_test.S:9: #define halt
0xE7FEDEFF
Ditto.  Can this file reuse NACL_HALT_OPCODE?

https://codereview.chromium.org/11194045/diff/1/tests/dynamic_code_loading/dy...
File tests/dynamic_code_loading/dynamic_load_test.c (right):

https://codereview.chromium.org/11194045/diff/1/tests/dynamic_code_loading/dy...
tests/dynamic_code_loading/dynamic_load_test.c:84: fill_int32(data, size,
0xE7FEDEFF); /* UDF #0xEDEF */
Ditto

[Please use jfb - chromium.org, 2012-10-18 01:26:20]

There's issue 2963.


On Wed, Oct 17, 2012 at 6:21 PM, <mseaborn@chromium.org> wrote:

> Filing an issue to track this would be useful.
>
> There are other places to convert:
>
> $ git grep -l 7777
> ...
> tests/debug_stub/debugger_**test.c
> tests/faulted_thread_queue/**faultqueue_test_guest.c
> tests/syscall_return_**sandboxing/sandboxed_arm.S
>
>
>
> https://codereview.chromium.**org/11194045/diff/1/src/**
>
trusted/debug_stub/abi.cc<https://codereview.chromium.org/11194045/diff/1/src/trusted/debug_stub/abi.cc>
> File src/trusted/debug_stub/abi.cc (right):
>
> https://codereview.chromium.**org/11194045/diff/1/src/**
>
trusted/debug_stub/abi.cc#**newcode133<https://codereview.chromium.org/11194045/diff/1/src/trusted/debug_stub/abi.cc#newcode133>
> src/trusted/debug_stub/abi.cc:**133: static uint32_t breakpoint_code_arm[]
> = { 0xE125BE7F /* BKPT 0x5BEF */ };
> Can this reuse NACL_HALT_OPCODE?
>
> https://codereview.chromium.**org/11194045/diff/1/src/**
>
trusted/service_runtime/arch/**arm/nacl_text_pad_test.S<https://codereview.chromium.org/11194045/diff/1/src/trusted/service_runtime/arch/arm/nacl_text_pad_test.S>
> File src/trusted/service_runtime/**arch/arm/nacl_text_pad_test.S (right):
>
> https://codereview.chromium.**org/11194045/diff/1/src/**
>
trusted/service_runtime/arch/**arm/nacl_text_pad_test.S#**newcode9<https://codereview.chromium.org/11194045/diff/1/src/trusted/service_runtime/arch/arm/nacl_text_pad_test.S#newcode9>
> src/trusted/service_runtime/**arch/arm/nacl_text_pad_test.S:**9: #define
> halt 0xE7FEDEFF
> Ditto.  Can this file reuse NACL_HALT_OPCODE?
>
> https://codereview.chromium.**org/11194045/diff/1/tests/**
>
dynamic_code_loading/dynamic_**load_test.c<https://codereview.chromium.org/11194045/diff/1/tests/dynamic_code_loading/dynamic_load_test.c>
> File tests/dynamic_code_loading/**dynamic_load_test.c (right):
>
> https://codereview.chromium.**org/11194045/diff/1/tests/**
>
dynamic_code_loading/dynamic_**load_test.c#newcode84<https://codereview.chromium.org/11194045/diff/1/tests/dynamic_code_loading/dynamic_load_test.c#newcode84>
> tests/dynamic_code_loading/**dynamic_load_test.c:84: fill_int32(data,
> size, 0xE7FEDEFF); /* UDF #0xEDEF */
> Ditto
>
>
https://codereview.chromium.**org/11194045/<https://codereview.chromium.org/1...
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[Mark Seaborn, 2012-10-18 01:32:13]

On 17 October 2012 18:26, JF Bastien <jfb@google.com> wrote:

> There's issue 2963.
>

OK.  So please set BUG= to
https://code.google.com/p/nativeclient/issues/detail?id=2963.

Or maybe to https://code.google.com/p/nativeclient/issues/detail?id=446,
whichever you think is more appropriate.

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[JF, 2012-10-18 02:34:37]

On 2012/10/18 01:32:13, Mark Seaborn wrote:
> On 17 October 2012 18:26, JF Bastien <mailto:jfb@google.com> wrote:
> 
> > There's issue 2963.
> >
> 
> OK.  So please set BUG= to
> https://code.google.com/p/nativeclient/issues/detail?id=2963.
> 
> Or maybe to https://code.google.com/p/nativeclient/issues/detail?id=446,
> whichever you think is more appropriate.
> 
> Cheers,
> Mark
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Native-Client-Reviews" group.
> To post to this group, send email to
mailto:native-client-reviews@googlegroups.com.
> To unsubscribe from this group, send email to
> mailto:native-client-reviews+unsubscribe@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/native-client-reviews?hl=en.
> 

Done.

[JF, 2012-10-18 02:37:22]

On 2012/10/18 01:21:39, Mark Seaborn wrote:
> Filing an issue to track this would be useful.
> 
> There are other places to convert:
> 
> $ git grep -l 7777
> ...
> tests/debug_stub/debugger_test.c
> tests/faulted_thread_queue/faultqueue_test_guest.c
> tests/syscall_return_sandboxing/sandboxed_arm.S
> 
> https://codereview.chromium.org/11194045/diff/1/src/trusted/debug_stub/abi.cc
> File src/trusted/debug_stub/abi.cc (right):
> 
>
https://codereview.chromium.org/11194045/diff/1/src/trusted/debug_stub/abi.cc...
> src/trusted/debug_stub/abi.cc:133: static uint32_t breakpoint_code_arm[] = {
> 0xE125BE7F /* BKPT 0x5BEF */ };
> Can this reuse NACL_HALT_OPCODE?
> 
>
https://codereview.chromium.org/11194045/diff/1/src/trusted/service_runtime/a...
> File src/trusted/service_runtime/arch/arm/nacl_text_pad_test.S (right):
> 
>
https://codereview.chromium.org/11194045/diff/1/src/trusted/service_runtime/a...
> src/trusted/service_runtime/arch/arm/nacl_text_pad_test.S:9: #define halt
> 0xE7FEDEFF
> Ditto.  Can this file reuse NACL_HALT_OPCODE?
> 
>
https://codereview.chromium.org/11194045/diff/1/tests/dynamic_code_loading/dy...
> File tests/dynamic_code_loading/dynamic_load_test.c (right):
> 
>
https://codereview.chromium.org/11194045/diff/1/tests/dynamic_code_loading/dy...
> tests/dynamic_code_loading/dynamic_load_test.c:84: fill_int32(data, size,
> 0xE7FEDEFF); /* UDF #0xEDEF */
> Ditto

Well this is embarrassing. I updated the code with hard-coded values for now,
I'll run try jobs first and figure out #defines tomorrow: I want to get
dependencies correctly (which module is The canonical repository of all ARM
knowledge?). Also, .word NACL_MY_MACRO is ugly, there must be a better way.

[Roland McGrath, 2012-10-18 17:31:30]

https://codereview.chromium.org/11194045/diff/23/tests/dynamic_code_loading/d...
File tests/dynamic_code_loading/dynamic_load_test.c (right):

https://codereview.chromium.org/11194045/diff/23/tests/dynamic_code_loading/d...
tests/dynamic_code_loading/dynamic_load_test.c:491: { 0xFF, 0xDE, 0xFE, 0xE7 };
/* 0xE7FEDEFF - UDF #0xEDEF */
I agree with Mark that all the constants should be using macros instead.  I'd
recommend doing it even for this one, where that also means moving the whole
definition inside the #if so that it can be uint32_t for the ARM case.

[JF, 2012-10-18 21:49:35]

Updated as suggested, PTAL.

[Mark Seaborn, 2012-10-18 22:05:30]

https://codereview.chromium.org/11194045/diff/28/src/include/arm_sandbox.h
File src/include/arm_sandbox.h (right):

https://codereview.chromium.org/11194045/diff/28/src/include/arm_sandbox.h#ne...
src/include/arm_sandbox.h:7: /* @file
We don't support Doxygen any more -- you can omit this.

https://codereview.chromium.org/11194045/diff/28/src/include/arm_sandbox.h#ne...
src/include/arm_sandbox.h:18: // Specially chosen BKPT and UDF instructions that
also correspond to
Please use C-style comments

https://codereview.chromium.org/11194045/diff/28/tests/dynamic_code_loading/d...
File tests/dynamic_code_loading/dynamic_load_test.c (right):

https://codereview.chromium.org/11194045/diff/28/tests/dynamic_code_loading/d...
tests/dynamic_code_loading/dynamic_load_test.c:492: (NACL_INSTR_HALT_FILL >>  0)
& 0xFF,
You can just do:

#if x86...
uint8_t halts = 0xf4;
#if arm...
uint32_t halts = NACL_INSTR_HALT_FILL;
#else ...

since we just use memcmp() below.

[JF, 2012-10-18 23:00:47]

On 2012/10/18 22:05:30, Mark Seaborn wrote:
> https://codereview.chromium.org/11194045/diff/28/src/include/arm_sandbox.h
> File src/include/arm_sandbox.h (right):
> 
>
https://codereview.chromium.org/11194045/diff/28/src/include/arm_sandbox.h#ne...
> src/include/arm_sandbox.h:7: /* @file
> We don't support Doxygen any more -- you can omit this.
> 
>
https://codereview.chromium.org/11194045/diff/28/src/include/arm_sandbox.h#ne...
> src/include/arm_sandbox.h:18: // Specially chosen BKPT and UDF instructions
that
> also correspond to
> Please use C-style comments
> 
>
https://codereview.chromium.org/11194045/diff/28/tests/dynamic_code_loading/d...
> File tests/dynamic_code_loading/dynamic_load_test.c (right):
> 
>
https://codereview.chromium.org/11194045/diff/28/tests/dynamic_code_loading/d...
> tests/dynamic_code_loading/dynamic_load_test.c:492: (NACL_INSTR_HALT_FILL >> 
0)
> & 0xFF,
> You can just do:
> 
> #if x86...
> uint8_t halts = 0xf4;
> #if arm...
> uint32_t halts = NACL_INSTR_HALT_FILL;
> #else ...
> 
> since we just use memcmp() below.

Done. Re-uploaded, tryjobs still running.

[Karl, 2012-10-19 17:52:16]

For the code in src/trusted/validator_arm, LGTM.

[Mark Seaborn, 2012-10-19 18:08:43]

LGTM for all except validator_arm

http://codereview.chromium.org/11194045/diff/19003/src/include/arm_sandbox.h
File src/include/arm_sandbox.h (right):

http://codereview.chromium.org/11194045/diff/19003/src/include/arm_sandbox.h#...
src/include/arm_sandbox.h:7: /* Minimal ARM sandbox constants.
Nit: NaCl comment style is
/*
 * blah...
 */

i.e. First line is '/*' on its own.

http://codereview.chromium.org/11194045/diff/19003/src/include/arm_sandbox.h#...
src/include/arm_sandbox.h:22: *  - UDF #0xEDE0 is used as abort-now (such as
__builtin_trap).
Why do you want to distinguish between abort-now and halt-fill?  Can you add
comments to explain when the differences are for debugging purposes, and when
they affect safety?

http://codereview.chromium.org/11194045/diff/19003/src/include/arm_sandbox.h#...
src/include/arm_sandbox.h:23: *  - UDF #0xEDE1 is used to always fail
validation.
Please comment that this is one of many, but it's the one we are guaranteeing.

http://codereview.chromium.org/11194045/diff/19003/tests/dynamic_code_loading...
File tests/dynamic_code_loading/dyncode_demand_alloc_test.c (right):

http://codereview.chromium.org/11194045/diff/19003/tests/dynamic_code_loading...
tests/dynamic_code_loading/dyncode_demand_alloc_test.c:18: uint8_t halts = 0xf4;
/* HLT */
Remove the indentation here

[JF, 2012-10-19 22:50:31]

Fixed comments as discussed. Also fixed unaligned_data_irt.nexe

[Mark Seaborn, 2012-10-19 22:53:17]

LGTM

https://codereview.chromium.org/11194045/diff/19008/src/include/arm_sandbox.h
File src/include/arm_sandbox.h (right):

https://codereview.chromium.org/11194045/diff/19008/src/include/arm_sandbox.h...
src/include/arm_sandbox.h:32: * BREAKPOINT, HALT_FILL and ABORT_NOW are intended
to be equivalent
Please expand to "NACL_INSTR_BREAKPOINT, NACL_INSTR_HALT_FILL and
NACL_INSTR_ABORT_NOW" for greppability

https://codereview.chromium.org/11194045/diff/19008/src/include/arm_sandbox.h...
src/include/arm_sandbox.h:36: * past one of these (unlike POOL_HEAD), because
the validator validates
Please expand to "NACL_INSTR_POOL_HEAD"

[commit-bot: I haz the power, 2012-10-19 22:56:19]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/jfb@chromium.org/11194045/27036

[JF, 2012-10-19 22:56:25]

Done, going through commit queue.

[commit-bot: I haz the power, 2012-10-19 23:57:46]

Try job failure for 11194045-27036 (retry) on
nacl-lucid_64-newlib-arm_qemu-pnacl for steps "annotate, scons [arm]
[small_tests medium_tests large_tests] [], scons [arm] [small_tests
medium_tests] [pic no_pexe ], summary".
It's a second try, previously, steps "annotate, scons [arm] [small_tests
medium_tests large_tests] [], scons [arm] [small_tests medium_tests] [pic
no_pexe ], summary" failed.
http://build.chromium.org/p/tryserver.nacl/buildstatus?builder=nacl-lucid_64-...