[MIPS] Add remaining parts for building tests for MIPS

[MIPS] Add remaining parts for building tests for MIPS

This change includes remaining parts of the patch for building tests for
MIPS. Some tests have been temporarily disabled for execution inside of
the emulator.

Minor change in stack_frame.cc in code unrelated to this CL to satisfy
presubmit script.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=2275
TEST= ./scons hello_world --mode=nacl platform=mips32 --verbose

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

[Roland McGrath, 2013-02-13 19:28:39]

https://codereview.chromium.org/12256018/diff/1/tests/barebones/barebones.h
File tests/barebones/barebones.h (right):

https://codereview.chromium.org/12256018/diff/1/tests/barebones/barebones.h#n...
tests/barebones/barebones.h:39: # define NACL_STACK_ALIGN_MASK      (0x7)
We want this to be consistent across machines at 16.
Even if there is no special benefit on mips to a 16-byte aligned stack, we want
to reduce the differences between machines.

[Mark Seaborn, 2013-02-13 22:14:09]

https://codereview.chromium.org/12256018/diff/1/run.py
File run.py (right):

https://codereview.chromium.org/12256018/diff/1/run.py#newcode77
run.py:77: 'toolchain', 'linux_mips-trusted')
Nit: fix indentation

https://codereview.chromium.org/12256018/diff/1/run.py#newcode78
run.py:78: env.qemu_mips32 = os.path.join(env.mips_root,
'run_under_qemu_mips32')
Can you be consistent in whether the var names use "mips" or "mips32"?

https://codereview.chromium.org/12256018/diff/1/run.py#newcode574
run.py:574: arch = 'mips32'
Fix indentation

https://codereview.chromium.org/12256018/diff/1/tests/barebones/barebones.h
File tests/barebones/barebones.h (right):

https://codereview.chromium.org/12256018/diff/1/tests/barebones/barebones.h#n...
tests/barebones/barebones.h:39: # define NACL_STACK_ALIGN_MASK      (0x7)
On 2013/02/13 19:28:39, Roland McGrath wrote:
> We want this to be consistent across machines at 16.
> Even if there is no special benefit on mips to a 16-byte aligned stack, we
want
> to reduce the differences between machines.

Right.  While we don't need MIPS to over-align in generated code, and PNaCl
bitcode specifically shouldn't need to know about stack alignment, there's no
need to complicate the tests with ifdefs for smaller alignments when larger
alignments will do.

https://codereview.chromium.org/12256018/diff/1/tests/barebones/barebones_sta...
File tests/barebones/barebones_stack_alignment16.c (right):

https://codereview.chromium.org/12256018/diff/1/tests/barebones/barebones_sta...
tests/barebones/barebones_stack_alignment16.c:11: NACL_SYSCALL(exit)(55 +
((int)__builtin_dwarf_cfa() & NACL_STACK_ALIGN_MASK));
This test is bizarre.  I'm not sure what it's supposed to be testing.  Maybe we
should just remove it (in a separate change)?

https://codereview.chromium.org/12256018/diff/1/tests/callingconv/nacl.scons
File tests/callingconv/nacl.scons (right):

https://codereview.chromium.org/12256018/diff/1/tests/callingconv/nacl.scons#...
tests/callingconv/nacl.scons:12: # TODO(petarj): Add nacl-gcc for MIPS.
I doubt you plan to do this. :-)  Maybe instead say "This test is disabled for
MIPS because we do not have a MIPS-enabled nacl-gcc to test PNaCl against."

Same for the other callingconv files.

https://codereview.chromium.org/12256018/diff/1/tests/data_not_executable/exe...
File tests/data_not_executable/execute_data.c (right):

https://codereview.chromium.org/12256018/diff/1/tests/data_not_executable/exe...
tests/data_not_executable/execute_data.c:17: uint32_t code[]
__attribute__((aligned(32)))= { 0x03e00008, /* JR RA */
Add a space before '='

https://codereview.chromium.org/12256018/diff/1/tests/data_not_executable/nac...
File tests/data_not_executable/nacl.scons (right):

https://codereview.chromium.org/12256018/diff/1/tests/data_not_executable/nac...
tests/data_not_executable/nacl.scons:29: env.Bit('target_mips32')) and
Please just remove the env.Bit('target_arm') (and MIPS) condition here for
conciseness.  Same for other cases.

env.UsingEmulator() is basically synonymous with non-whole-machine QEMU, and the
target_arm was kind of there just to make this more self-documenting.

If you want to update the comment you could say "qemu-user" or
"non-whole-machine QEMU" instead of "qemu-arm".

https://codereview.chromium.org/12256018/diff/1/tests/egyptian_cotton/nacl.scons
File tests/egyptian_cotton/nacl.scons (right):

https://codereview.chromium.org/12256018/diff/1/tests/egyptian_cotton/nacl.sc...
tests/egyptian_cotton/nacl.scons:78: high_thread_count_nthreads = 3500
Is this based on a particular hardware device?  Maybe add a comment.

https://codereview.chromium.org/12256018/diff/1/tests/exception_test/exceptio...
File tests/exception_test/exception_crash_test.c (right):

https://codereview.chromium.org/12256018/diff/1/tests/exception_test/exceptio...
tests/exception_test/exception_crash_test.c:82: void
bad_stack_exception_handler();
This probably fails to compile due to -Wstrict-prototypes.  Make this match the
other definitions.

https://codereview.chromium.org/12256018/diff/1/tests/exception_test/exceptio...
tests/exception_test/exception_crash_test.c:83:
__asm__("bad_stack_exception_handler:\n"
Nit: remove indentation

https://codereview.chromium.org/12256018/diff/1/tests/exception_test/exceptio...
tests/exception_test/exception_crash_test.c:84: ".text\n"
Use pushsection+popsection instead of .text

https://codereview.chromium.org/12256018/diff/1/tests/exception_test/exceptio...
File tests/exception_test/exception_test.c (right):

https://codereview.chromium.org/12256018/diff/1/tests/exception_test/exceptio...
tests/exception_test/exception_test.c:67: assert((uintptr_t) pointer % 8 == 0);
But we declared it with __attribute__((aligned(16))), so no conditional should
be needed for this to pass

https://codereview.chromium.org/12256018/diff/1/tests/fib/nacl.scons
File tests/fib/nacl.scons (right):

https://codereview.chromium.org/12256018/diff/1/tests/fib/nacl.scons#newcode9
tests/fib/nacl.scons:9: is_broken=(env.Bit('build_mips32') and
env.UsingEmulator())
Style is
  foo = bar
rather than
  foo=(bar)

https://codereview.chromium.org/12256018/diff/1/tests/infoleak/nacl.scons
File tests/infoleak/nacl.scons (right):

https://codereview.chromium.org/12256018/diff/1/tests/infoleak/nacl.scons#new...
tests/infoleak/nacl.scons:8: if env.Bit('target_mips32'):
Please add a comment, either "TODO: port to MIPS" or say why the test isn't
relevant (no FP support, perhaps?)

https://codereview.chromium.org/12256018/diff/1/tests/mandel/nacl.scons
File tests/mandel/nacl.scons (right):

https://codereview.chromium.org/12256018/diff/1/tests/mandel/nacl.scons#newco...
tests/mandel/nacl.scons:43: # TODO(petarj): The test hangs indefinite on QEMU
MIPS. Enable it when fixed.
'indefinitely'

https://codereview.chromium.org/12256018/diff/1/tests/mandel/nacl.scons#newco...
tests/mandel/nacl.scons:49: env.UsingEmulator())
Nit: indent as
is_broken=(x and
           y)

Same in other cases

https://codereview.chromium.org/12256018/diff/1/tests/mmap/nacl.scons
File tests/mmap/nacl.scons (right):

https://codereview.chromium.org/12256018/diff/1/tests/mmap/nacl.scons#newcode24
tests/mmap/nacl.scons:24: ['small_tests', 'sel_ldr_tests'], 'run_mmap_test',
This seems like a more serious case to not work.  How does it fail?  Please add
a TODO

https://codereview.chromium.org/12256018/diff/1/tests/stubout_mode/nacl.scons
File tests/stubout_mode/nacl.scons (right):

https://codereview.chromium.org/12256018/diff/1/tests/stubout_mode/nacl.scons...
tests/stubout_mode/nacl.scons:91: if not env.Bit('target_mips32'):
Use the is_broken argument instead

https://codereview.chromium.org/12256018/diff/1/tests/stubout_mode/partly_inv...
File tests/stubout_mode/partly_invalid.c (right):

https://codereview.chromium.org/12256018/diff/1/tests/stubout_mode/partly_inv...
tests/stubout_mode/partly_invalid.c:38: __asm__(".p2align 4 \n"
Nit: For consistency, no space before \n

https://codereview.chromium.org/12256018/diff/1/tests/thread_capture/arch/mip...
File tests/thread_capture/arch/mips/thread_capture_test_injection.c (right):

https://codereview.chromium.org/12256018/diff/1/tests/thread_capture/arch/mip...
tests/thread_capture/arch/mips/thread_capture_test_injection.c:19:
NaClCopyInDropLock(natp->nap);
This needs updating in order to compile

[petarj, 2013-02-14 18:04:14]

https://codereview.chromium.org/12256018/diff/1/tests/barebones/barebones.h
File tests/barebones/barebones.h (right):

https://codereview.chromium.org/12256018/diff/1/tests/barebones/barebones.h#n...
tests/barebones/barebones.h:39: # define NACL_STACK_ALIGN_MASK      (0x7)
On 2013/02/13 22:14:09, Mark Seaborn wrote:
> On 2013/02/13 19:28:39, Roland McGrath wrote:
> > We want this to be consistent across machines at 16.
> > Even if there is no special benefit on mips to a 16-byte aligned stack, we
> want
> > to reduce the differences between machines.
> 
> Right.  While we don't need MIPS to over-align in generated code, and PNaCl
> bitcode specifically shouldn't need to know about stack alignment, there's no
> need to complicate the tests with ifdefs for smaller alignments when larger
> alignments will do.

The return value specified in barebones_stack_alignment16.c as 
(55 + ((int)__builtin_dwarf_cfa() & NACL_STACK_ALIGN_MASK))

will be different for MIPS if we leave the same stack alignment, and
the test will fail.

[Mark Seaborn, 2013-02-14 18:13:35]

On 14 February 2013 10:04, <petarj@mips.com> wrote:

>
> https://codereview.chromium.**org/12256018/diff/1/tests/**
>
barebones/barebones.h<https://codereview.chromium.org/12256018/diff/1/tests/barebones/barebones.h>
> File tests/barebones/barebones.h (right):
>
> https://codereview.chromium.**org/12256018/diff/1/tests/**
>
barebones/barebones.h#**newcode39<https://codereview.chromium.org/12256018/diff/1/tests/barebones/barebones.h#newcode39>
> tests/barebones/barebones.h:**39: # define NACL_STACK_ALIGN_MASK
> (0x7)
> On 2013/02/13 22:14:09, Mark Seaborn wrote:
>
>> On 2013/02/13 19:28:39, Roland McGrath wrote:
>> > We want this to be consistent across machines at 16.
>> > Even if there is no special benefit on mips to a 16-byte aligned stack,
>> we
>>
> > want to reduce the differences between machines.
>>
>
>  Right.  While we don't need MIPS to over-align in generated code, and
>> PNaCl
>>
> bitcode specifically shouldn't need to know about stack alignment, there's
>> no
>>
> need to complicate the tests with ifdefs for smaller alignments when larger
>>
> alignments will do.
>>
>
> The return value specified in barebones_stack_alignment16.c as
> (55 + ((int)__builtin_dwarf_cfa() & NACL_STACK_ALIGN_MASK))
>
> will be different for MIPS if we leave the same stack alignment, and
> the test will fail.
>

I know, which is why I also wrote: "This test is bizarre.  I'm not sure
what it's supposed to be testing. Maybe we should just remove it (in a
separate change)?"

Cheers,
Mark

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

[petarj, 2013-03-05 17:50:03]

Second patch uploaded.

https://codereview.chromium.org/12256018/diff/1/run.py
File run.py (right):

https://codereview.chromium.org/12256018/diff/1/run.py#newcode77
run.py:77: 'toolchain', 'linux_mips-trusted')
On 2013/02/13 22:14:09, Mark Seaborn wrote:
> Nit: fix indentation

Done.

https://codereview.chromium.org/12256018/diff/1/run.py#newcode78
run.py:78: env.qemu_mips32 = os.path.join(env.mips_root,
'run_under_qemu_mips32')
On 2013/02/13 22:14:09, Mark Seaborn wrote:
> Can you be consistent in whether the var names use "mips" or "mips32"?

Done.

https://codereview.chromium.org/12256018/diff/1/run.py#newcode574
run.py:574: arch = 'mips32'
On 2013/02/13 22:14:09, Mark Seaborn wrote:
> Fix indentation

Done.

https://codereview.chromium.org/12256018/diff/1/tests/barebones/barebones.h
File tests/barebones/barebones.h (right):

https://codereview.chromium.org/12256018/diff/1/tests/barebones/barebones.h#n...
tests/barebones/barebones.h:39: # define NACL_STACK_ALIGN_MASK      (0x7)
On 2013/02/14 18:04:14, petarj wrote:
> On 2013/02/13 22:14:09, Mark Seaborn wrote:
> > On 2013/02/13 19:28:39, Roland McGrath wrote:
> > > We want this to be consistent across machines at 16.
> > > Even if there is no special benefit on mips to a 16-byte aligned stack, we
> > want
> > > to reduce the differences between machines.
> > 
> > Right.  While we don't need MIPS to over-align in generated code, and PNaCl
> > bitcode specifically shouldn't need to know about stack alignment, there's
no
> > need to complicate the tests with ifdefs for smaller alignments when larger
> > alignments will do.
> 
> The return value specified in barebones_stack_alignment16.c as 
> (55 + ((int)__builtin_dwarf_cfa() & NACL_STACK_ALIGN_MASK))
> 
> will be different for MIPS if we leave the same stack alignment, and
> the test will fail.

I will revert this part of the change and remove
barebones_stack_alignment16.c in a separate change.

https://codereview.chromium.org/12256018/diff/1/tests/barebones/barebones_sta...
File tests/barebones/barebones_stack_alignment16.c (right):

https://codereview.chromium.org/12256018/diff/1/tests/barebones/barebones_sta...
tests/barebones/barebones_stack_alignment16.c:11: NACL_SYSCALL(exit)(55 +
((int)__builtin_dwarf_cfa() & NACL_STACK_ALIGN_MASK));
On 2013/02/13 22:14:09, Mark Seaborn wrote:
> This test is bizarre.  I'm not sure what it's supposed to be testing.  Maybe
we
> should just remove it (in a separate change)?

Ok, I will remove it in a separate change.

https://codereview.chromium.org/12256018/diff/1/tests/callingconv/nacl.scons
File tests/callingconv/nacl.scons (right):

https://codereview.chromium.org/12256018/diff/1/tests/callingconv/nacl.scons#...
tests/callingconv/nacl.scons:12: # TODO(petarj): Add nacl-gcc for MIPS.
On 2013/02/13 22:14:09, Mark Seaborn wrote:
> I doubt you plan to do this. :-)  Maybe instead say "This test is disabled for
> MIPS because we do not have a MIPS-enabled nacl-gcc to test PNaCl against."
> 
> Same for the other callingconv files.

Done.

https://codereview.chromium.org/12256018/diff/1/tests/data_not_executable/exe...
File tests/data_not_executable/execute_data.c (right):

https://codereview.chromium.org/12256018/diff/1/tests/data_not_executable/exe...
tests/data_not_executable/execute_data.c:17: uint32_t code[]
__attribute__((aligned(32)))= { 0x03e00008, /* JR RA */
On 2013/02/13 22:14:09, Mark Seaborn wrote:
> Add a space before '='

Done.

https://codereview.chromium.org/12256018/diff/1/tests/data_not_executable/nac...
File tests/data_not_executable/nacl.scons (right):

https://codereview.chromium.org/12256018/diff/1/tests/data_not_executable/nac...
tests/data_not_executable/nacl.scons:29: env.Bit('target_mips32')) and
On 2013/02/13 22:14:09, Mark Seaborn wrote:
> Please just remove the env.Bit('target_arm') (and MIPS) condition here for
> conciseness.  Same for other cases.
> 
> env.UsingEmulator() is basically synonymous with non-whole-machine QEMU, and
the
> target_arm was kind of there just to make this more self-documenting.
> 
> If you want to update the comment you could say "qemu-user" or
> "non-whole-machine QEMU" instead of "qemu-arm".

Done.

https://codereview.chromium.org/12256018/diff/1/tests/egyptian_cotton/nacl.scons
File tests/egyptian_cotton/nacl.scons (right):

https://codereview.chromium.org/12256018/diff/1/tests/egyptian_cotton/nacl.sc...
tests/egyptian_cotton/nacl.scons:78: high_thread_count_nthreads = 3500
On 2013/02/13 22:14:09, Mark Seaborn wrote:
> Is this based on a particular hardware device?  Maybe add a comment.

Done.

https://codereview.chromium.org/12256018/diff/1/tests/exception_test/exceptio...
File tests/exception_test/exception_crash_test.c (right):

https://codereview.chromium.org/12256018/diff/1/tests/exception_test/exceptio...
tests/exception_test/exception_crash_test.c:82: void
bad_stack_exception_handler();
On 2013/02/13 22:14:09, Mark Seaborn wrote:
> This probably fails to compile due to -Wstrict-prototypes.  Make this match
the
> other definitions.

Done.

https://codereview.chromium.org/12256018/diff/1/tests/exception_test/exceptio...
tests/exception_test/exception_crash_test.c:83:
__asm__("bad_stack_exception_handler:\n"
On 2013/02/13 22:14:09, Mark Seaborn wrote:
> Nit: remove indentation

Done.

https://codereview.chromium.org/12256018/diff/1/tests/exception_test/exceptio...
tests/exception_test/exception_crash_test.c:84: ".text\n"
On 2013/02/13 22:14:09, Mark Seaborn wrote:
> Use pushsection+popsection instead of .text

Done.

https://codereview.chromium.org/12256018/diff/1/tests/exception_test/exceptio...
File tests/exception_test/exception_test.c (right):

https://codereview.chromium.org/12256018/diff/1/tests/exception_test/exceptio...
tests/exception_test/exception_test.c:67: assert((uintptr_t) pointer % 8 == 0);
On 2013/02/13 22:14:09, Mark Seaborn wrote:
> But we declared it with __attribute__((aligned(16))), so no conditional should
> be needed for this to pass

attribute aligned does not work for variables on stack on MIPS.
Speaking of which, what is the meaning of check_stack_is_aligned
if you are putting a 16-byte aligned struct on stack and then
checking if it is aligned? It looks more like you are checking that
attribute aligned works correctly rather than checking that stack
pointer is aligned.

https://codereview.chromium.org/12256018/diff/1/tests/fib/nacl.scons
File tests/fib/nacl.scons (right):

https://codereview.chromium.org/12256018/diff/1/tests/fib/nacl.scons#newcode9
tests/fib/nacl.scons:9: is_broken=(env.Bit('build_mips32') and
env.UsingEmulator())
On 2013/02/13 22:14:09, Mark Seaborn wrote:
> Style is
>   foo = bar
> rather than
>   foo=(bar)

Done.

https://codereview.chromium.org/12256018/diff/1/tests/infoleak/nacl.scons
File tests/infoleak/nacl.scons (right):

https://codereview.chromium.org/12256018/diff/1/tests/infoleak/nacl.scons#new...
tests/infoleak/nacl.scons:8: if env.Bit('target_mips32'):
On 2013/02/13 22:14:09, Mark Seaborn wrote:
> Please add a comment, either "TODO: port to MIPS" or say why the test isn't
> relevant (no FP support, perhaps?)

Removing this. It should fail until fixed.

https://codereview.chromium.org/12256018/diff/1/tests/mandel/nacl.scons
File tests/mandel/nacl.scons (right):

https://codereview.chromium.org/12256018/diff/1/tests/mandel/nacl.scons#newco...
tests/mandel/nacl.scons:43: # TODO(petarj): The test hangs indefinite on QEMU
MIPS. Enable it when fixed.
On 2013/02/13 22:14:09, Mark Seaborn wrote:
> 'indefinitely'

Done.

https://codereview.chromium.org/12256018/diff/1/tests/mandel/nacl.scons#newco...
tests/mandel/nacl.scons:49: env.UsingEmulator())
On 2013/02/13 22:14:09, Mark Seaborn wrote:
> Nit: indent as
> is_broken=(x and
>            y)
> 
> Same in other cases

Done.

https://codereview.chromium.org/12256018/diff/1/tests/mmap/nacl.scons
File tests/mmap/nacl.scons (right):

https://codereview.chromium.org/12256018/diff/1/tests/mmap/nacl.scons#newcode24
tests/mmap/nacl.scons:24: ['small_tests', 'sel_ldr_tests'], 'run_mmap_test',
On 2013/02/13 22:14:09, Mark Seaborn wrote:
> This seems like a more serious case to not work.  How does it fail?  Please
add
> a TODO

Works now. Fixed in https://codereview.chromium.org/12460003.

https://codereview.chromium.org/12256018/diff/1/tests/stubout_mode/nacl.scons
File tests/stubout_mode/nacl.scons (right):

https://codereview.chromium.org/12256018/diff/1/tests/stubout_mode/nacl.scons...
tests/stubout_mode/nacl.scons:91: if not env.Bit('target_mips32'):
On 2013/02/13 22:14:09, Mark Seaborn wrote:
> Use the is_broken argument instead

Done.

https://codereview.chromium.org/12256018/diff/1/tests/stubout_mode/partly_inv...
File tests/stubout_mode/partly_invalid.c (right):

https://codereview.chromium.org/12256018/diff/1/tests/stubout_mode/partly_inv...
tests/stubout_mode/partly_invalid.c:38: __asm__(".p2align 4 \n"
On 2013/02/13 22:14:09, Mark Seaborn wrote:
> Nit: For consistency, no space before \n

Done.

https://codereview.chromium.org/12256018/diff/1/tests/thread_capture/arch/mip...
File tests/thread_capture/arch/mips/thread_capture_test_injection.c (right):

https://codereview.chromium.org/12256018/diff/1/tests/thread_capture/arch/mip...
tests/thread_capture/arch/mips/thread_capture_test_injection.c:19:
NaClCopyInDropLock(natp->nap);
On 2013/02/13 22:14:09, Mark Seaborn wrote:
> This needs updating in order to compile

Done.

[petarj, 2013-03-08 23:24:27]

On 2013/03/05 17:50:03, petarj wrote:
> Second patch uploaded.

Ping.

[Mark Seaborn, 2013-03-11 15:25:16]

On 8 March 2013 15:24, <petarj@mips.com> wrote:

> On 2013/03/05 17:50:03, petarj wrote:
>
>> Second patch uploaded.
>>
>
> Ping.
>

Could you possibly split this into smaller changes?  I was prepared to
review this as one change, but that's harder to do if there's a gap of
weeks between revisions, and large changes between revisions (since you
implemented register_set.{c,h} between the revisions).  Also, I have a
short attention span. ;-)

I'd suggest splitting out these parts into separate changes:

 * Add conditionals to Scons (to callingconv tests etc.) so that "./scons
platform=mips32" does not abort with an assertion error.  Without this, I
can't even run hello_world_test without hacking the Scons files first.
[Then, in no particular order:]
 * Implement register_set.{c,h}
 * Disable tests that fail under MIPS QEMU

Mark

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

[Mark Seaborn, 2013-03-11 15:54:37]

https://codereview.chromium.org/12256018/diff/17001/tests/barebones/barebones.h
File tests/barebones/barebones.h (right):

https://codereview.chromium.org/12256018/diff/17001/tests/barebones/barebones...
tests/barebones/barebones.h:2: * Copyright (c) 2010 The Native Client Authors.
All rights reserved.
This file has not changed otherwise; you don't need to change the boilerplate.

https://codereview.chromium.org/12256018/diff/17001/tests/barebones/top_of_sa...
File tests/barebones/top_of_sandbox.c (right):

https://codereview.chromium.org/12256018/diff/17001/tests/barebones/top_of_sa...
tests/barebones/top_of_sandbox.c:36: "lw  %0, -32767(%0)\n"
Can you write -0x7fff instead for readability?  Same below.

https://codereview.chromium.org/12256018/diff/17001/tests/common/register_set.c
File tests/common/register_set.c (right):

https://codereview.chromium.org/12256018/diff/17001/tests/common/register_set...
tests/common/register_set.c:223: * For t6 and t7 we expect to hold control flow
masks. For t8, which holds TLS
How about: "The registers below are all read-only, so we skip their comparison. 
We expect t6 and t7 to hold control flow and data masks..."

https://codereview.chromium.org/12256018/diff/17001/tests/common/register_set...
tests/common/register_set.c:224: * index, we skip comparison. Zero register
holds zero allways. All of the
"always"

https://codereview.chromium.org/12256018/diff/17001/tests/common/register_set...
tests/common/register_set.c:227: * use by interrupt/trap handler and therefore
volatile.
Does this mean untrusted code can read the values of k0/k1 left behind by
interrupt/trap handlers?  Isn't that a trusted->untrusted information leak? 
What sort of handlers are you referring to?  Userland signal handlers, or
something in the kernel?

https://codereview.chromium.org/12256018/diff/17001/tests/common/register_set.h
File tests/common/register_set.h (right):

https://codereview.chromium.org/12256018/diff/17001/tests/common/register_set...
tests/common/register_set.h:107: #elif NACL_ARCH(NACL_BUILD_ARCH) == NACL_mips
Add empty line before this for consistency with above

https://codereview.chromium.org/12256018/diff/17001/tests/common/register_set...
tests/common/register_set.h:113: "move $a0, %0\n" "nop\n" \
Put 'nop' on separate line, please

https://codereview.chromium.org/12256018/diff/17001/tests/common/register_set...
tests/common/register_set.h:148: : : "r"(regs) : "memory")
Add empty line after this for spacing consistency

https://codereview.chromium.org/12256018/diff/17001/tests/common/register_set...
tests/common/register_set.h:380: "sw $k0, 104($sp)\n" \
It's odd that you skip setting k0 and k1 but you save them here.  What's the
status of these in the sandbox?  Are they read-only?

https://codereview.chromium.org/12256018/diff/17001/tests/common/register_set...
tests/common/register_set.h:403: ".popsection\n")
Add empty line after this for spacing consistency

https://codereview.chromium.org/12256018/diff/17001/tests/exception_test/exce...
File tests/exception_test/exception_crash_test.c (right):

https://codereview.chromium.org/12256018/diff/17001/tests/exception_test/exce...
tests/exception_test/exception_crash_test.c:78: #elif NACL_ARCH(NACL_BUILD_ARCH)
== NACL_mips
Add empty line before for consistent spacing

https://codereview.chromium.org/12256018/diff/17001/tests/exception_test/exce...
tests/exception_test/exception_crash_test.c:93: _exit(1);
Add empty line after for consistent spacing

https://codereview.chromium.org/12256018/diff/17001/tests/manifest_file/nacl....
File tests/manifest_file/nacl.scons (right):

https://codereview.chromium.org/12256018/diff/17001/tests/manifest_file/nacl....
tests/manifest_file/nacl.scons:43: and env.UsingEmulator())
Please indent as:
is_broken=(foo
           and bar)
(brackets optional)

https://codereview.chromium.org/12256018/diff/17001/tests/nameservice/nacl.scons
File tests/nameservice/nacl.scons (right):

https://codereview.chromium.org/12256018/diff/17001/tests/nameservice/nacl.sc...
tests/nameservice/nacl.scons:52: is_broken=env.Bit('target_mips32') and
In other files you added a TODO to comment this -- please be consistent.

It looks like you're disabling all the SelUniversalTest instances on MIPS QEMU. 
Could you change SelUniversalTest in SConstruct to disable them across the board
for MIPS QEMU?

https://codereview.chromium.org/12256018/diff/17001/tests/stack_alignment/sta...
File tests/stack_alignment/stack_alignment_asm_test.c (right):

https://codereview.chromium.org/12256018/diff/17001/tests/stack_alignment/sta...
tests/stack_alignment/stack_alignment_asm_test.c:64: __asm__(
Add empty line before this for spacing consistency

https://codereview.chromium.org/12256018/diff/17001/tests/stubout_mode/nacl.s...
File tests/stubout_mode/nacl.scons (right):

https://codereview.chromium.org/12256018/diff/17001/tests/stubout_mode/nacl.s...
tests/stubout_mode/nacl.scons:24: if env.Bit('build_arm'):
How about just disabling this file for MIPS the same way it's disabled for ARM,
here?

[petarj, 2013-03-14 01:13:03]

Remaining parts of the change uploaded.

https://codereview.chromium.org/12256018/diff/17001/tests/barebones/top_of_sa...
File tests/barebones/top_of_sandbox.c (right):

https://codereview.chromium.org/12256018/diff/17001/tests/barebones/top_of_sa...
tests/barebones/top_of_sandbox.c:36: "lw  %0, -32767(%0)\n"
On 2013/03/11 15:54:38, Mark Seaborn wrote:
> Can you write -0x7fff instead for readability?  Same below.

Done.

https://codereview.chromium.org/12256018/diff/17001/tests/exception_test/exce...
File tests/exception_test/exception_crash_test.c (right):

https://codereview.chromium.org/12256018/diff/17001/tests/exception_test/exce...
tests/exception_test/exception_crash_test.c:78: #elif NACL_ARCH(NACL_BUILD_ARCH)
== NACL_mips
On 2013/03/11 15:54:38, Mark Seaborn wrote:
> Add empty line before for consistent spacing

Done.

https://codereview.chromium.org/12256018/diff/17001/tests/exception_test/exce...
tests/exception_test/exception_crash_test.c:93: _exit(1);
On 2013/03/11 15:54:38, Mark Seaborn wrote:
> Add empty line after for consistent spacing

Done.

https://codereview.chromium.org/12256018/diff/17001/tests/stack_alignment/sta...
File tests/stack_alignment/stack_alignment_asm_test.c (right):

https://codereview.chromium.org/12256018/diff/17001/tests/stack_alignment/sta...
tests/stack_alignment/stack_alignment_asm_test.c:64: __asm__(
On 2013/03/11 15:54:38, Mark Seaborn wrote:
> Add empty line before this for spacing consistency

Done.

https://codereview.chromium.org/12256018/diff/17001/tests/stubout_mode/nacl.s...
File tests/stubout_mode/nacl.scons (right):

https://codereview.chromium.org/12256018/diff/17001/tests/stubout_mode/nacl.s...
tests/stubout_mode/nacl.scons:24: if env.Bit('build_arm'):
On 2013/03/11 15:54:38, Mark Seaborn wrote:
> How about just disabling this file for MIPS the same way it's disabled for
ARM,
> here?
MIPS has stubout mode, so it can execute stubout tests.

[Mark Seaborn, 2013-03-14 15:48:00]

LGTM

https://codereview.chromium.org/12256018/diff/1/tests/exception_test/exceptio...
File tests/exception_test/exception_test.c (right):

https://codereview.chromium.org/12256018/diff/1/tests/exception_test/exceptio...
tests/exception_test/exception_test.c:67: assert((uintptr_t) pointer % 8 == 0);
Answering your earlier question...

On 2013/03/05 17:50:04, petarj wrote:
> On 2013/02/13 22:14:09, Mark Seaborn wrote:
> > But we declared it with __attribute__((aligned(16))), so no conditional
should
> > be needed for this to pass
> 
> attribute aligned does not work for variables on stack on MIPS.

Do you mean it does not actually align the address of the variable?  That would
be a bug, right?  Do you mean that the "align" attribute of LLVM's "alloca"
instruction doesn't work on MIPS?

Or do you just mean the backend does not make any assumptions about the
alignment of the stack?

> Speaking of which, what is the meaning of check_stack_is_aligned
> if you are putting a 16-byte aligned struct on stack and then
> checking if it is aligned? It looks more like you are checking that
> attribute aligned works correctly rather than checking that stack
> pointer is aligned.

No, we are checking that the stack pointer is aligned, but the check only works
if the compiler makes use of the ABI's stack alignment and does not force a
realignment of the stack pointer.

So this check isn't very reliable, and you are welcome to remove
check_stack_is_aligned().  This check is made obsolete by the later check,
assert(frame_base % STACK_ALIGNMENT == 0).

https://codereview.chromium.org/12256018/diff/43002/tests/exception_test/exce...
File tests/exception_test/exception_test.c (right):

https://codereview.chromium.org/12256018/diff/43002/tests/exception_test/exce...
tests/exception_test/exception_test.c:62: } __attribute__((aligned(16)));
Let's just remove check_stack_is_aligned() -- see other comment.

https://codereview.chromium.org/12256018/diff/43002/tests/stubout_mode/nacl.s...
File tests/stubout_mode/nacl.scons (right):

https://codereview.chromium.org/12256018/diff/43002/tests/stubout_mode/nacl.s...
tests/stubout_mode/nacl.scons:93: # This tool exists only for x86.
If stubout mode is implemented for MIPS, can't you enable building ncval_stubout
for MIPS?

https://codereview.chromium.org/12256018/diff/43002/tests/stubout_mode/nacl.s...
tests/stubout_mode/nacl.scons:95: 'partly_invalid_stubout.out',
Don't change the indentation here.

https://codereview.chromium.org/12256018/diff/43002/tests/trusted_crash/crash...
File tests/trusted_crash/crash_in_syscall/nacl.scons (right):

https://codereview.chromium.org/12256018/diff/43002/tests/trusted_crash/crash...
tests/trusted_crash/crash_in_syscall/nacl.scons:51: # qemu-user hang on the
following test:
Should be "qemu-user hangs"

[petarj, 2013-03-15 18:34:07]

Updated patch.
Includes minor cleanup.

https://codereview.chromium.org/12256018/diff/43002/tests/exception_test/exce...
File tests/exception_test/exception_test.c (right):

https://codereview.chromium.org/12256018/diff/43002/tests/exception_test/exce...
tests/exception_test/exception_test.c:62: } __attribute__((aligned(16)));
On 2013/03/14 15:48:00, Mark Seaborn wrote:
> Let's just remove check_stack_is_aligned() -- see other comment.
Ok, I have removed struct AlignedType and check_pointer_is_aligned too,
since they are not used anywhere else. Are you ok with that?
Double-check.

https://codereview.chromium.org/12256018/diff/43002/tests/stubout_mode/nacl.s...
File tests/stubout_mode/nacl.scons (right):

https://codereview.chromium.org/12256018/diff/43002/tests/stubout_mode/nacl.s...
tests/stubout_mode/nacl.scons:93: # This tool exists only for x86.
On 2013/03/14 15:48:00, Mark Seaborn wrote:
> If stubout mode is implemented for MIPS, can't you enable building
ncval_stubout
> for MIPS?

We are still missing ncvalidate_verbose for MIPS.

https://codereview.chromium.org/12256018/diff/43002/tests/stubout_mode/nacl.s...
tests/stubout_mode/nacl.scons:95: 'partly_invalid_stubout.out',
On 2013/03/14 15:48:00, Mark Seaborn wrote:
> Don't change the indentation here.

Done.

https://codereview.chromium.org/12256018/diff/43002/tests/trusted_crash/crash...
File tests/trusted_crash/crash_in_syscall/nacl.scons (right):

https://codereview.chromium.org/12256018/diff/43002/tests/trusted_crash/crash...
tests/trusted_crash/crash_in_syscall/nacl.scons:51: # qemu-user hang on the
following test:
On 2013/03/14 15:48:00, Mark Seaborn wrote:
> Should be "qemu-user hangs"

Done.

[petarj, 2013-03-15 18:44:00]

I forgot to answer your "align" attribute question.
It does work except for variables on stack that require alignment larger than
the ABI's stack alignment which is 8 bytes for MIPS o32 ABI.
We need to implement dynamic stack realignment (which is already on our TODO
list for LLVM).

[Mark Seaborn, 2013-03-15 19:57:56]

LGTM

[commit-bot: I haz the power, 2013-03-15 19:59:08]

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

[commit-bot: I haz the power, 2013-03-15 22:07:37]

Change committed as 11001