MinSFI: Add loader

Add MinSFI loader

This patch implements a memory initialization routine as part of MinSFI's
trusted runtime. The code reads the manifest of the sandbox it was compiled
against, mmaps a sufficiently large memory region and jumps into the
sandbox's entry function. There is no support for multiple simultaneously 
active sandboxes or multithreading.

BUG= https://code.google.com/p/chromium/issues/detail?id=397177
TEST= ./scons bitcode=1 minsfi=1 minsfi_tests
NOTRY=true

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

[dbrazdil, 2014-09-03 22:02:38]

Patchset #2 (id:20001) has been deleted

[dbrazdil, 2014-09-04 01:12:47]

Patchset #2 (id:40001) has been deleted

[dbrazdil, 2014-09-04 01:12:51]

Patchset #1 (id:1) has been deleted

[dbrazdil, 2014-09-04 01:20:21]

Patchset #2 (id:80001) has been deleted

[dbrazdil, 2014-09-04 01:31:40]

Patchset #3 (id:120001) has been deleted

[dbrazdil, 2014-09-04 01:31:44]

Patchset #2 (id:100001) has been deleted

[dbrazdil, 2014-09-04 02:07:12]

Patchset #2 (id:140001) has been deleted

[dbrazdil, 2014-09-04 02:07:17]

Patchset #1 (id:60001) has been deleted

[dbrazdil, 2014-09-04 19:32:40]

Patchset #1 (id:160001) has been deleted

[dbrazdil, 2014-09-04 19:54:03]

Patchset #1 (id:180001) has been deleted

[dbrazdil, 2014-09-04 20:00:38]

dbrazdil@google.com changed reviewers:
+ jvoung@chromium.org, mseaborn@chromium.org

[jvoung (off chromium), 2014-09-05 00:32:00]

https://codereview.chromium.org/539683002/diff/200001/site_scons/site_tools/n...
File site_scons/site_tools/naclsdk.py (right):

https://codereview.chromium.org/539683002/diff/200001/site_scons/site_tools/n...
site_scons/site_tools/naclsdk.py:185: llc_cpu = 'x86_64'
Not necessarily for this CL, but would be curious if this would actually work on
the ARM HW panda bots (or the qemu bots)? The gnu part would have to be
different though, for ARM (gnueabi).

You would also need the hard-float/soft-float to match the trusted code.

https://codereview.chromium.org/539683002/diff/200001/src/include/minsfi_priv.h
File src/include/minsfi_priv.h (right):

https://codereview.chromium.org/539683002/diff/200001/src/include/minsfi_priv...
src/include/minsfi_priv.h:20: } manifest;
If we're following NaCl style, I think most of the NaCl structs are CamelCase.

Maybe prefix with MinSFI also.

https://codereview.chromium.org/539683002/diff/200001/src/include/minsfi_priv...
src/include/minsfi_priv.h:38: bool generate_layout(manifest *sb, uint32_t
page_size, layout *mem);
Could this be "const manifest *sb" ?

Similar below.

https://codereview.chromium.org/539683002/diff/200001/src/minsfi/trusted/buil...
File src/minsfi/trusted/build.scons (right):

https://codereview.chromium.org/539683002/diff/200001/src/minsfi/trusted/buil...
src/minsfi/trusted/build.scons:8: if not env.Bit('posix'):
Did you want to restrict this to env.Bit('minsfi') also?

https://codereview.chromium.org/539683002/diff/200001/src/minsfi/trusted/entry.c
File src/minsfi/trusted/entry.c (right):

https://codereview.chromium.org/539683002/diff/200001/src/minsfi/trusted/entr...
src/minsfi/trusted/entry.c:28: uint64_t __sfi_memory_base = 0;
Could add some emphasis that this is exported, but imported by the untrusted
code via the compiler transforms. Maybe declare as extern in the minsfi.h ?

https://codereview.chromium.org/539683002/diff/200001/src/minsfi/trusted/entr...
src/minsfi/trusted/entry.c:60: return destroy_sandbox((char*) ((uintptr_t)
__sfi_memory_base), &sb);
Should this modify __sfi_memory_base back to 0 before returning?

https://codereview.chromium.org/539683002/diff/200001/src/minsfi/trusted/load...
File src/minsfi/trusted/loader.c (right):

https://codereview.chromium.org/539683002/diff/200001/src/minsfi/trusted/load...
src/minsfi/trusted/loader.c:34: return (start < end) &&  /* implies reg.length
>= page_size */
For the "implies", it's this check plus the page_alignment checks you mean?
(Wasn't so clear because it's separated by other checks.)

https://codereview.chromium.org/539683002/diff/200001/src/minsfi/trusted/load...
src/minsfi/trusted/loader.c:48: static inline uint64_t
address_space_size(manifest *sb) {
could make these pointers const too?

https://codereview.chromium.org/539683002/diff/200001/src/minsfi/trusted/load...
src/minsfi/trusted/loader.c:106: PROT_READ | PROT_WRITE,
Hmm, didn't really think of this earlier, but we've combined the constant data
w/ the read/write-able data. If later we split the data segments into two, then
we could just PROT_READ the constant data segment.

https://codereview.chromium.org/539683002/diff/200001/src/minsfi/trusted/load...
src/minsfi/trusted/loader.c:120: base = mmap(NULL, total_mem, 0, MAP_ANON |
MAP_PRIVATE, -1, 0);
Should we have MAP_NORESERVE also?

https://codereview.chromium.org/539683002/diff/200001/tests/minsfi/init_trust...
File tests/minsfi/init_trusted.c (right):

https://codereview.chromium.org/539683002/diff/200001/tests/minsfi/init_trust...
tests/minsfi/init_trusted.c:17: sb.dataseg_size = 0x400;
Is it important that dataseg_size < page_size? (to test rounding up?)

https://codereview.chromium.org/539683002/diff/200001/tests/minsfi/init_trust...
tests/minsfi/init_trusted.c:70: sb.dataseg_offset = (1 << 25);
Maybe the "25" could have been based off sb.ptr_size instead, so that it's
easier to change.

Similar for other spots that refer to 24?

https://codereview.chromium.org/539683002/diff/200001/tests/minsfi/init_trust...
tests/minsfi/init_trusted.c:111: ASSERT_EQ(ret_val, minsfi_exec());
Do you expect that sandbox base to be random/re-initialized?

https://codereview.chromium.org/539683002/diff/200001/tests/minsfi/nacl.scons
File tests/minsfi/nacl.scons (right):

https://codereview.chromium.org/539683002/diff/200001/tests/minsfi/nacl.scons...
tests/minsfi/nacl.scons:7: trusted_env = env['TRUSTED_ENV']
You might have to test that trusted_env is not some sort of null first. A bunch
of nacl.scons files check if 'TRUSTED_ENV' not in env: bail.

I don't remember exactly when that happens, but it might show up in the trybots
(unless it's for some obscure configuration like coverage tests).

[dbrazdil, 2014-09-05 19:24:54]

Patchset #3 (id:240001) has been deleted

[dbrazdil, 2014-09-05 19:25:00]

Patchset #2 (id:220001) has been deleted

[dbrazdil, 2014-09-05 19:26:14]

Patchset #2 (id:260001) has been deleted

[dbrazdil, 2014-09-05 19:38:25]

Patchset #2 (id:280001) has been deleted

[dbrazdil, 2014-09-05 19:41:24]

Patchset #2 (id:300001) has been deleted

[dbrazdil, 2014-09-05 19:41:38]

https://codereview.chromium.org/539683002/diff/200001/site_scons/site_tools/n...
File site_scons/site_tools/naclsdk.py (right):

https://codereview.chromium.org/539683002/diff/200001/site_scons/site_tools/n...
site_scons/site_tools/naclsdk.py:185: llc_cpu = 'x86_64'
On 2014/09/05 00:31:59, jvoung wrote:
> Not necessarily for this CL, but would be curious if this would actually work
on
> the ARM HW panda bots (or the qemu bots)? The gnu part would have to be
> different though, for ARM (gnueabi).
> 
> You would also need the hard-float/soft-float to match the trusted code.

Acknowledged.

https://codereview.chromium.org/539683002/diff/200001/src/include/minsfi_priv.h
File src/include/minsfi_priv.h (right):

https://codereview.chromium.org/539683002/diff/200001/src/include/minsfi_priv...
src/include/minsfi_priv.h:20: } manifest;
On 2014/09/05 00:31:59, jvoung wrote:
> If we're following NaCl style, I think most of the NaCl structs are CamelCase.
> 
> Maybe prefix with MinSFI also.

Done.

https://codereview.chromium.org/539683002/diff/200001/src/include/minsfi_priv...
src/include/minsfi_priv.h:38: bool generate_layout(manifest *sb, uint32_t
page_size, layout *mem);
On 2014/09/05 00:31:59, jvoung wrote:
> Could this be "const manifest *sb" ?
> 
> Similar below.

Done.

https://codereview.chromium.org/539683002/diff/200001/src/minsfi/trusted/buil...
File src/minsfi/trusted/build.scons (right):

https://codereview.chromium.org/539683002/diff/200001/src/minsfi/trusted/buil...
src/minsfi/trusted/build.scons:8: if not env.Bit('posix'):
On 2014/09/05 00:31:59, jvoung wrote:
> Did you want to restrict this to env.Bit('minsfi') also?

Done. Also changed the description of the flag to state that it both builds and
tests MinSFI.

https://codereview.chromium.org/539683002/diff/200001/src/minsfi/trusted/entry.c
File src/minsfi/trusted/entry.c (right):

https://codereview.chromium.org/539683002/diff/200001/src/minsfi/trusted/entr...
src/minsfi/trusted/entry.c:28: uint64_t __sfi_memory_base = 0;
On 2014/09/05 00:31:59, jvoung wrote:
> Could add some emphasis that this is exported, but imported by the untrusted
> code via the compiler transforms. Maybe declare as extern in the minsfi.h ?

Done.

https://codereview.chromium.org/539683002/diff/200001/src/minsfi/trusted/entr...
src/minsfi/trusted/entry.c:60: return destroy_sandbox((char*) ((uintptr_t)
__sfi_memory_base), &sb);
On 2014/09/05 00:31:59, jvoung wrote:
> Should this modify __sfi_memory_base back to 0 before returning?

Done. Added tests which check that the base has been reset.

https://codereview.chromium.org/539683002/diff/200001/src/minsfi/trusted/load...
File src/minsfi/trusted/loader.c (right):

https://codereview.chromium.org/539683002/diff/200001/src/minsfi/trusted/load...
src/minsfi/trusted/loader.c:34: return (start < end) &&  /* implies reg.length
>= page_size */
On 2014/09/05 00:31:59, jvoung wrote:
> For the "implies", it's this check plus the page_alignment checks you mean?
> (Wasn't so clear because it's separated by other checks.)

Done.

https://codereview.chromium.org/539683002/diff/200001/src/minsfi/trusted/load...
src/minsfi/trusted/loader.c:48: static inline uint64_t
address_space_size(manifest *sb) {
On 2014/09/05 00:32:00, jvoung wrote:
> could make these pointers const too?

Done.

https://codereview.chromium.org/539683002/diff/200001/src/minsfi/trusted/load...
src/minsfi/trusted/loader.c:106: PROT_READ | PROT_WRITE,
On 2014/09/05 00:31:59, jvoung wrote:
> Hmm, didn't really think of this earlier, but we've combined the constant data
> w/ the read/write-able data. If later we split the data segments into two,
then
> we could just PROT_READ the constant data segment.

Yep, thought of it too when I was writing this. I'll note down to put this into
the AllocateDataSegment comments.

https://codereview.chromium.org/539683002/diff/200001/src/minsfi/trusted/load...
src/minsfi/trusted/loader.c:120: base = mmap(NULL, total_mem, 0, MAP_ANON |
MAP_PRIVATE, -1, 0);
On 2014/09/05 00:31:59, jvoung wrote:
> Should we have MAP_NORESERVE also?

I'm not entirely sure what that does. But it sounds like reserving swap space
for the inaccessible pages is undesirable.

https://codereview.chromium.org/539683002/diff/200001/tests/minsfi/init_trust...
File tests/minsfi/init_trusted.c (right):

https://codereview.chromium.org/539683002/diff/200001/tests/minsfi/init_trust...
tests/minsfi/init_trusted.c:17: sb.dataseg_size = 0x400;
On 2014/09/05 00:32:00, jvoung wrote:
> Is it important that dataseg_size < page_size? (to test rounding up?)

It's more the fact that dataseg_size is not page size-aligned, which
reflects what will happen with real manifests. I should put a comment
there and even make it bigger than page_size.

https://codereview.chromium.org/539683002/diff/200001/tests/minsfi/init_trust...
tests/minsfi/init_trusted.c:70: sb.dataseg_offset = (1 << 25);
On 2014/09/05 00:32:00, jvoung wrote:
> Maybe the "25" could have been based off sb.ptr_size instead, so that it's
> easier to change.
> 
> Similar for other spots that refer to 24?

Done. All of these constants are quite fragile so I was trying to avoid
adding logic into the tests and rather have the tests fail if something
changes than potentially stop testing the right thing. But I guess this
is harmless.

https://codereview.chromium.org/539683002/diff/200001/tests/minsfi/init_trust...
tests/minsfi/init_trusted.c:111: ASSERT_EQ(ret_val, minsfi_exec());
On 2014/09/05 00:32:00, jvoung wrote:
> Do you expect that sandbox base to be random/re-initialized?

Possibly in the future? The extension unpacking won't do that
but having the flexibility seems like a useful thing for 
features that might come next (threads, multiple sandboxes).

https://codereview.chromium.org/539683002/diff/200001/tests/minsfi/nacl.scons
File tests/minsfi/nacl.scons (right):

https://codereview.chromium.org/539683002/diff/200001/tests/minsfi/nacl.scons...
tests/minsfi/nacl.scons:7: trusted_env = env['TRUSTED_ENV']
On 2014/09/05 00:32:00, jvoung wrote:
> You might have to test that trusted_env is not some sort of null first. A
bunch
> of nacl.scons files check if 'TRUSTED_ENV' not in env: bail.
> 
> I don't remember exactly when that happens, but it might show up in the
trybots
> (unless it's for some obscure configuration like coverage tests).

Done.

[dbrazdil, 2014-09-05 21:15:36]

Patchset #3 (id:340001) has been deleted

[dbrazdil, 2014-09-06 18:04:57]

Patchset #4 (id:380001) has been deleted

[dbrazdil, 2014-09-06 18:18:32]

Patchset #4 (id:400001) has been deleted

[dbrazdil, 2014-09-06 23:16:49]

Patchset #4 (id:420001) has been deleted

[dbrazdil, 2014-09-07 16:01:33]

Patchset #4 (id:440001) has been deleted

[dbrazdil, 2014-09-07 16:01:38]

Patchset #4 (id:460001) has been deleted

[dbrazdil, 2014-09-07 18:33:06]

Patchset #4 (id:480001) has been deleted

[dbrazdil, 2014-09-07 18:35:10]

Patchset #4 (id:500001) has been deleted

[dbrazdil, 2014-09-07 18:38:37]

Patchset #4 (id:520001) has been deleted

[dbrazdil, 2014-09-07 18:41:58]

Added an interface for retrieving the active sandbox instance.
It doesn't do anything special at the moment, but it will make
adding support for multiple sandboxes much easier in the future.

[jvoung (off chromium), 2014-09-08 23:18:48]

https://codereview.chromium.org/539683002/diff/540001/src/include/minsfi.h
File src/include/minsfi.h (right):

https://codereview.chromium.org/539683002/diff/540001/src/include/minsfi.h#ne...
src/include/minsfi.h:7: #ifndef SRC_INCLUDE_MINSFI_H_
I think the full NaCl header guards are:

NATIVE_CLIENT_SRC_INCLUDE_BLAH_H_

https://codereview.chromium.org/539683002/diff/540001/src/minsfi/trusted/load...
File src/minsfi/trusted/loader.c (right):

https://codereview.chromium.org/539683002/diff/540001/src/minsfi/trusted/load...
src/minsfi/trusted/loader.c:34: /* Run the checks. Note that page alignment
together with start != end imply
nit:

/*
 * blah
 */

like the comment style below.

https://codereview.chromium.org/539683002/diff/540001/src/minsfi/trusted/load...
src/minsfi/trusted/loader.c:119: /* Compute the boundaries of the data segment,
heap and stack. Verify
/*
 * Blah
 */

here and below

https://codereview.chromium.org/539683002/diff/540001/src/minsfi/trusted/load...
src/minsfi/trusted/loader.c:129: sb->mem_base = mmap(NULL, sb->mem_alloc_size,
0,
Could use PROT_NONE instead of 0, to be clear.

https://codereview.chromium.org/539683002/diff/540001/tests/minsfi/nacl.scons
File tests/minsfi/nacl.scons (right):

https://codereview.chromium.org/539683002/diff/540001/tests/minsfi/nacl.scons...
tests/minsfi/nacl.scons:23: srcs_untrusted = [ 'sandbox_%s.c' % sandbox_name ]
Might be better to just have |sandbox_name| be the name of the full source. That
way it's more greppable what's referencing the files (if someone wants to rename
the file).

https://codereview.chromium.org/539683002/diff/540001/tests/minsfi/nacl.scons...
tests/minsfi/nacl.scons:28: test_file =
bc_env.ComponentObject(name_untrusted_obj, srcs_untrusted)
Do you need this separate ComponentObject() invocation?

Could you just pass srcs_untrusted to ComponentProgram?

https://codereview.chromium.org/539683002/diff/540001/tests/minsfi/nacl.scons...
tests/minsfi/nacl.scons:49: prog = trusted_env.ComponentProgram(test_name ,
[llc_result, main],
same, might be able to just pass srcs_trusted + llc_result to ComponentProgram

https://codereview.chromium.org/539683002/diff/540001/tests/minsfi/test_initi...
File tests/minsfi/test_initializer.c (right):

https://codereview.chromium.org/539683002/diff/540001/tests/minsfi/test_initi...
tests/minsfi/test_initializer.c:6: 
Short description of what this tests?

https://codereview.chromium.org/539683002/diff/540001/tests/minsfi/test_memor...
File tests/minsfi/test_memory_layout.c (right):

https://codereview.chromium.org/539683002/diff/540001/tests/minsfi/test_memor...
tests/minsfi/test_memory_layout.c:51: 
Might be good to have a 32-bit one, to show that 33 is right at the border.

[dbrazdil, 2014-09-09 00:57:53]

https://codereview.chromium.org/539683002/diff/540001/src/include/minsfi.h
File src/include/minsfi.h (right):

https://codereview.chromium.org/539683002/diff/540001/src/include/minsfi.h#ne...
src/include/minsfi.h:7: #ifndef SRC_INCLUDE_MINSFI_H_
On 2014/09/08 23:18:47, jvoung wrote:
> I think the full NaCl header guards are:
> 
> NATIVE_CLIENT_SRC_INCLUDE_BLAH_H_

Done. (Lint said it was this but I see you're right)

https://codereview.chromium.org/539683002/diff/540001/src/minsfi/trusted/load...
File src/minsfi/trusted/loader.c (right):

https://codereview.chromium.org/539683002/diff/540001/src/minsfi/trusted/load...
src/minsfi/trusted/loader.c:34: /* Run the checks. Note that page alignment
together with start != end imply
On 2014/09/08 23:18:47, jvoung wrote:
> nit:
> 
> /*
>  * blah
>  */
> 
> like the comment style below.

Done.

https://codereview.chromium.org/539683002/diff/540001/src/minsfi/trusted/load...
src/minsfi/trusted/loader.c:119: /* Compute the boundaries of the data segment,
heap and stack. Verify
On 2014/09/08 23:18:47, jvoung wrote:
> /*
>  * Blah
>  */
> 
> here and below

Done.

https://codereview.chromium.org/539683002/diff/540001/src/minsfi/trusted/load...
src/minsfi/trusted/loader.c:129: sb->mem_base = mmap(NULL, sb->mem_alloc_size,
0,
On 2014/09/08 23:18:47, jvoung wrote:
> Could use PROT_NONE instead of 0, to be clear.

Done.

https://codereview.chromium.org/539683002/diff/540001/tests/minsfi/nacl.scons
File tests/minsfi/nacl.scons (right):

https://codereview.chromium.org/539683002/diff/540001/tests/minsfi/nacl.scons...
tests/minsfi/nacl.scons:23: srcs_untrusted = [ 'sandbox_%s.c' % sandbox_name ]
On 2014/09/08 23:18:47, jvoung wrote:
> Might be better to just have |sandbox_name| be the name of the full source.
That
> way it's more greppable what's referencing the files (if someone wants to
rename
> the file).

Done.

https://codereview.chromium.org/539683002/diff/540001/tests/minsfi/nacl.scons...
tests/minsfi/nacl.scons:28: test_file =
bc_env.ComponentObject(name_untrusted_obj, srcs_untrusted)
On 2014/09/08 23:18:47, jvoung wrote:
> Do you need this separate ComponentObject() invocation?
> 
> Could you just pass srcs_untrusted to ComponentProgram?

I do need it, actually. If I gave it straight to ComponentProgram,
the object would be given a default name (e.g. "sandbox_dummy.bc")
and that would prevent me from reusing that source file, like I do
in these tests (SCons would complain that the .bc file has multiple
definitions).

https://codereview.chromium.org/539683002/diff/540001/tests/minsfi/nacl.scons...
tests/minsfi/nacl.scons:49: prog = trusted_env.ComponentProgram(test_name ,
[llc_result, main],
On 2014/09/08 23:18:47, jvoung wrote:
> same, might be able to just pass srcs_trusted + llc_result to ComponentProgram

Done. Here's it's okay because it doesn't make sense to reuse
the same exerciser in multiple tests.

https://codereview.chromium.org/539683002/diff/540001/tests/minsfi/test_initi...
File tests/minsfi/test_initializer.c (right):

https://codereview.chromium.org/539683002/diff/540001/tests/minsfi/test_initi...
tests/minsfi/test_initializer.c:6: 
On 2014/09/08 23:18:47, jvoung wrote:
> Short description of what this tests?

Done.

https://codereview.chromium.org/539683002/diff/540001/tests/minsfi/test_memor...
File tests/minsfi/test_memory_layout.c (right):

https://codereview.chromium.org/539683002/diff/540001/tests/minsfi/test_memor...
tests/minsfi/test_memory_layout.c:51: 
On 2014/09/08 23:18:48, jvoung wrote:
> Might be good to have a 32-bit one, to show that 33 is right at the border.

Done.

[jvoung (off chromium), 2014-09-09 01:32:50]

https://codereview.chromium.org/539683002/diff/560001/SConstruct
File SConstruct (right):

https://codereview.chromium.org/539683002/diff/560001/SConstruct#newcode333
SConstruct:333: BitFromArgument(env, 'minsfi', default=env.Bit('bitcode'),
Wait... with minsfi=True whenever bitcode=1 means that the pnacl_ld_flags are
changed to the minsfi ones all of the time for PNaCl.

I think you need to run them in a separate scons invocations, for a subset of
the tests. So, it would be like nonsfi_nacl, which is default False and then the
bot scripts run it on a subset of the tests.

[dbrazdil, 2014-09-09 01:53:39]

https://codereview.chromium.org/539683002/diff/560001/SConstruct
File SConstruct (right):

https://codereview.chromium.org/539683002/diff/560001/SConstruct#newcode333
SConstruct:333: BitFromArgument(env, 'minsfi', default=env.Bit('bitcode'),
On 2014/09/09 01:32:50, jvoung wrote:
> Wait... with minsfi=True whenever bitcode=1 means that the pnacl_ld_flags are
> changed to the minsfi ones all of the time for PNaCl.
> 
> I think you need to run them in a separate scons invocations, for a subset of
> the tests. So, it would be like nonsfi_nacl, which is default False and then
the
> bot scripts run it on a subset of the tests.

Crap, that would explain why some unrelated tests are failing. Thanks,
couldn't figure it out.

[dbrazdil, 2014-09-09 15:45:04]

Patchset #6 (id:580001) has been deleted

[dbrazdil, 2014-09-09 16:04:46]

Patchset #6 (id:600001) has been deleted

[dbrazdil, 2014-09-09 16:50:43]

Patchset #6 (id:620001) has been deleted

[dbrazdil, 2014-09-09 20:24:31]

On 2014/09/09 01:53:39, dbrazdil wrote:
> https://codereview.chromium.org/539683002/diff/560001/SConstruct
> File SConstruct (right):
> 
> https://codereview.chromium.org/539683002/diff/560001/SConstruct#newcode333
> SConstruct:333: BitFromArgument(env, 'minsfi', default=env.Bit('bitcode'),
> On 2014/09/09 01:32:50, jvoung wrote:
> > Wait... with minsfi=True whenever bitcode=1 means that the pnacl_ld_flags
are
> > changed to the minsfi ones all of the time for PNaCl.
> > 
> > I think you need to run them in a separate scons invocations, for a subset
of
> > the tests. So, it would be like nonsfi_nacl, which is default False and then
> the
> > bot scripts run it on a subset of the tests.
> 
> Crap, that would explain why some unrelated tests are failing. Thanks,
> couldn't figure it out.

Done. The minsfi flag is False by default, tests are invoked directly from
the python/shell scripts and all trybots pass.

[jvoung (off chromium), 2014-09-09 21:15:45]

LGTM otherwise

https://codereview.chromium.org/539683002/diff/640001/buildbot/buildbot_pnacl.py
File buildbot/buildbot_pnacl.py (right):

https://codereview.chromium.org/539683002/diff/640001/buildbot/buildbot_pnacl...
buildbot/buildbot_pnacl.py:121: args=flags_run + ['bitcode=1', 'minsfi=1',
'minsfi_tests'])
I don't think you need bitcode=1

The context should have had the 'pnacl' entry set to 1, which will tell
buildbot_lib to add bitcode=1

https://codereview.chromium.org/539683002/diff/640001/buildbot/buildbot_pnacl.sh
File buildbot/buildbot_pnacl.sh (right):

https://codereview.chromium.org/539683002/diff/640001/buildbot/buildbot_pnacl...
buildbot/buildbot_pnacl.sh:440: scons-stage-noirt "${arch}" "${scons_flags} -j8
bitcode=1 minsfi=1" \
bitcode=1 is already added by SCONS_COMMON

https://codereview.chromium.org/539683002/diff/640001/tests/minsfi/nacl.scons
File tests/minsfi/nacl.scons (right):

https://codereview.chromium.org/539683002/diff/640001/tests/minsfi/nacl.scons...
tests/minsfi/nacl.scons:28: [ sandbox_src ])
nit: style guide says to just have [foo] without the whitespace

https://codereview.chromium.org/539683002/diff/640001/tests/minsfi/nacl.scons...
tests/minsfi/nacl.scons:53: '${LLC} '
You could probably just have ${LLC} ${SOURCES} -o ... on one line?

[dbrazdil, 2014-09-09 22:17:43]

https://codereview.chromium.org/539683002/diff/640001/buildbot/buildbot_pnacl.py
File buildbot/buildbot_pnacl.py (right):

https://codereview.chromium.org/539683002/diff/640001/buildbot/buildbot_pnacl...
buildbot/buildbot_pnacl.py:121: args=flags_run + ['bitcode=1', 'minsfi=1',
'minsfi_tests'])
On 2014/09/09 21:15:45, jvoung wrote:
> I don't think you need bitcode=1
> 
> The context should have had the 'pnacl' entry set to 1, which will tell
> buildbot_lib to add bitcode=1

Done.

https://codereview.chromium.org/539683002/diff/640001/buildbot/buildbot_pnacl.sh
File buildbot/buildbot_pnacl.sh (right):

https://codereview.chromium.org/539683002/diff/640001/buildbot/buildbot_pnacl...
buildbot/buildbot_pnacl.sh:440: scons-stage-noirt "${arch}" "${scons_flags} -j8
bitcode=1 minsfi=1" \
On 2014/09/09 21:15:45, jvoung wrote:
> bitcode=1 is already added by SCONS_COMMON

Done.

https://codereview.chromium.org/539683002/diff/640001/tests/minsfi/nacl.scons
File tests/minsfi/nacl.scons (right):

https://codereview.chromium.org/539683002/diff/640001/tests/minsfi/nacl.scons...
tests/minsfi/nacl.scons:28: [ sandbox_src ])
On 2014/09/09 21:15:45, jvoung wrote:
> nit: style guide says to just have [foo] without the whitespace

Done.

https://codereview.chromium.org/539683002/diff/640001/tests/minsfi/nacl.scons...
tests/minsfi/nacl.scons:53: '${LLC} '
On 2014/09/09 21:15:45, jvoung wrote:
> You could probably just have ${LLC} ${SOURCES} -o ... on one line?

Done.

[dbrazdil, 2014-09-09 23:22:43]

The CQ bit was checked by dbrazdil@google.com

[commit-bot: I haz the power, 2014-09-09 23:23:20]

CQ is trying da patch. Follow status at
 https://nativeclient-status.appspot.com/cq/dbrazdil@google.com/539683002/660001

[commit-bot: I haz the power, 2014-09-09 23:23:57]

Committed patchset #7 (id:660001) as 13722