[MIPS] Leave room for frame header on stack for MIPS arch

[MIPS] Leave room for frame header on stack for MIPS arch

In addition to proper alignment of an arbitrary stack top, additional space
for frame header for MIPS has to be left. Its size is 16 bytes.

This change also fixes entry value of t9 which needs to be equal to
prog_ctr for PIC code.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=2275
TEST= pnacl/build.sh all


Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=10960

[petarj, 2013-03-05 17:55:01]

Fixes for a few issues with exceptions and stack alignment. This change also
introduces new macro NACL_STACK_ARG_SIZE which is 16 for MIPS and 0 for
other architectures. There is a similar macro (NACL_STACK_PAD_BELOW_ALIGN)
but I think both these should exist.

[Mark Seaborn, 2013-03-06 00:26:58]

https://codereview.chromium.org/12460003/diff/1/src/trusted/service_runtime/n...
File src/trusted/service_runtime/nacl_config.h (right):

https://codereview.chromium.org/12460003/diff/1/src/trusted/service_runtime/n...
src/trusted/service_runtime/nacl_config.h:276: # define NACL_STACK_ARG_SIZE     
  (0x10)
Can you comment this?  e.g. The MIPS ABI requires callers to reserve 16 bytes
above the stack pointer, which the callee can spill arguments to.

https://codereview.chromium.org/12460003/diff/1/src/trusted/service_runtime/p...
File src/trusted/service_runtime/posix/nacl_signal.c (right):

https://codereview.chromium.org/12460003/diff/1/src/trusted/service_runtime/p...
src/trusted/service_runtime/posix/nacl_signal.c:274: regs->stack_ptr =
NaClUserToSys(nap, new_stack_ptr) - NACL_STACK_ARG_SIZE;
The adjustment should be applied before NaClUserToSys() for more safety.

I think it would be cleaner to add this adjustment via an extra field in
NaClExceptionFrame in service_runtime/nacl_exception.h.

https://codereview.chromium.org/12460003/diff/1/src/trusted/service_runtime/p...
src/trusted/service_runtime/posix/nacl_signal.c:275: regs->t9 = regs->prog_ctr;
Can you add a comment?  e.g. "PIC functions assume that r9 contains the
function's address on entry"

[petarj, 2013-03-07 14:52:48]

Second patch set uploaded.

https://codereview.chromium.org/12460003/diff/1/src/trusted/service_runtime/n...
File src/trusted/service_runtime/nacl_config.h (right):

https://codereview.chromium.org/12460003/diff/1/src/trusted/service_runtime/n...
src/trusted/service_runtime/nacl_config.h:276: # define NACL_STACK_ARG_SIZE     
  (0x10)
On 2013/03/06 00:26:58, Mark Seaborn wrote:
> Can you comment this?  e.g. The MIPS ABI requires callers to reserve 16 bytes
> above the stack pointer, which the callee can spill arguments to.

Done.

https://codereview.chromium.org/12460003/diff/1/src/trusted/service_runtime/p...
File src/trusted/service_runtime/posix/nacl_signal.c (right):

https://codereview.chromium.org/12460003/diff/1/src/trusted/service_runtime/p...
src/trusted/service_runtime/posix/nacl_signal.c:274: regs->stack_ptr =
NaClUserToSys(nap, new_stack_ptr) - NACL_STACK_ARG_SIZE;
On 2013/03/06 00:26:58, Mark Seaborn wrote:
> The adjustment should be applied before NaClUserToSys() for more safety.

Done, I added the adjustment just after the alignment adjustment.
> 
> I think it would be cleaner to add this adjustment via an extra field in
> NaClExceptionFrame in service_runtime/nacl_exception.h.

Extra field in NaClExceptionFrame would not be transparent enough, and we could
not apply the same kind of adjustment for NaClSysThreadCreate.

https://codereview.chromium.org/12460003/diff/1/src/trusted/service_runtime/p...
src/trusted/service_runtime/posix/nacl_signal.c:275: regs->t9 = regs->prog_ctr;
On 2013/03/06 00:26:58, Mark Seaborn wrote:
> Can you add a comment?  e.g. "PIC functions assume that r9 contains the
> function's address on entry"

Done.

[Mark Seaborn, 2013-03-07 16:07:53]

LGTM

https://codereview.chromium.org/12460003/diff/16001/src/trusted/service_runti...
File src/trusted/service_runtime/nacl_config.h (right):

https://codereview.chromium.org/12460003/diff/16001/src/trusted/service_runti...
src/trusted/service_runtime/nacl_config.h:280: # define NACL_STACK_ARG_SIZE     
  (0x10)
Maybe calls this "ARGS_SIZE" rather than "ARG_SIZE", because it's the size of
all the arguments passed in registers rather than a single argument.  (Is there
a better term for this?  In Windows' x64 ABI, it's referred to as "shadow space"
here: http://en.wikipedia.org/wiki/X86_calling_conventions.)

https://codereview.chromium.org/12460003/diff/16001/src/trusted/service_runti...
File src/trusted/service_runtime/nacl_syscall_common.c (right):

https://codereview.chromium.org/12460003/diff/16001/src/trusted/service_runti...
src/trusted/service_runtime/nacl_syscall_common.c:2459: - NACL_STACK_ARG_SIZE;
Nit: indent by 1 fewer space, because this is not inside the brackets that the
previous line starts inside.

[petarj, 2013-03-07 17:33:12]

https://codereview.chromium.org/12460003/diff/16001/src/trusted/service_runti...
File src/trusted/service_runtime/nacl_config.h (right):

https://codereview.chromium.org/12460003/diff/16001/src/trusted/service_runti...
src/trusted/service_runtime/nacl_config.h:280: # define NACL_STACK_ARG_SIZE     
  (0x10)
On 2013/03/07 16:07:53, Mark Seaborn wrote:
> Maybe calls this "ARGS_SIZE" rather than "ARG_SIZE", because it's the size of
> all the arguments passed in registers rather than a single argument.

Done.

> Is there
> a better term for this?  In Windows' x64 ABI, it's referred to as "shadow
space"
> here: http://en.wikipedia.org/wiki/X86_calling_conventions.)

There is no common name everybody uses for it.
I used "stack frame header" line for some other projects like v8.
GCC calls it "args_size".

https://codereview.chromium.org/12460003/diff/16001/src/trusted/service_runti...
File src/trusted/service_runtime/nacl_syscall_common.c (right):

https://codereview.chromium.org/12460003/diff/16001/src/trusted/service_runti...
src/trusted/service_runtime/nacl_syscall_common.c:2459: - NACL_STACK_ARG_SIZE;
On 2013/03/07 16:07:53, Mark Seaborn wrote:
> Nit: indent by 1 fewer space, because this is not inside the brackets that the
> previous line starts inside.

Done.

[commit-bot: I haz the power, 2013-03-07 19:44:35]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/petarj@mips.com/12460003/22001

[commit-bot: I haz the power, 2013-03-07 19:51:15]

Try job failure for 12460003-22001 (retry) on nacl-mac10.7_newlib_opt for steps
"annotate, gyp_compile, summary".
It's a second try, previously, steps "annotate, gyp_compile, summary" failed.
http://build.chromium.org/p/tryserver.nacl/buildstatus?builder=nacl-mac10.7_n...

[commit-bot: I haz the power, 2013-03-07 23:10:27]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/petarj@mips.com/12460003/22001

[commit-bot: I haz the power, 2013-03-07 23:17:23]

Try job failure for 12460003-22001 (retry) on nacl-mac10.7_newlib_opt for steps
"annotate, gyp_compile, summary".
It's a second try, previously, steps "annotate, gyp_compile, summary" failed.
http://build.chromium.org/p/tryserver.nacl/buildstatus?builder=nacl-mac10.7_n...

[commit-bot: I haz the power, 2013-03-08 01:49:36]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/petarj@mips.com/12460003/22001

[commit-bot: I haz the power, 2013-03-08 02:05:12]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is nacl-precise64_newlib_dbg, revision is 10955,
job name
was 12460003-22001 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2013-03-08 10:09:09]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/petarj@mips.com/12460003/22001

[commit-bot: I haz the power, 2013-03-08 12:10:11]

Change committed as 10960