Fixes some bugs with memory setup and halt-sled allocation and...

Fixes some bugs with memory setup and halt-sled allocation and
initialization.

Tests in assembly language are written to be text-only -- no rodata
nor data segments are present in one set of tests -- with varying
sizes of padding so that the text segment ends exactly at a multiple
of 65536, just 32 bytes shy of it (the halt sled size), many bytes shy
of a multiple, and 28 bytes shy of 65536 (too small gap).  The text
prints out the break address and the end-of-text address to check the
rounding that's done by the sel_ldr.

In addition to the 4 text-only tests, this CL also builds a set with
rodata -- containing a string -- at its "natural" position, and
another set with rodata at 0x100000, leaving space for dynamic code.
These additional eight tests have _ro and _ro_dyn in their names.

In order to get exact memory layout, the tests are built by directly
invoking the loader, rather than indirectly through nacl-g++.  For the
x86-* architectures, we are using nacl-ld; for the arm, we are using
code sourcery's ld and patching the generated exectuable with the
elf_patcher.py script to modify the Elf headers to be nexes as opposed
to standard executables.

Because there appears to be a linker bug on the ARM, the _ro_dyn tests
are not built -- the text is moved to an address other than 0x20000,
despite command-line flags to the contrary.  One ARM test
(too_small_ro) triggers a LOG_FATAL because the ARM linker will not
automatically move the .rodata section to ensure that there is enough
room for the halt sled.  If we could use linker scripts so that full
control is feasible but defaulting to automatically always reserving
gap space, that would be ideal behavior....

The next set of CLs will add tests where data segments *are* present
as well, and where no rodata is present but rw data is present -- more
test case combinatorics.  And the address space setup code needs to be
cleaned up still.

BUG= http://code.google.com/p/nativeclient/issues/detail?id=506 http://code.google.com/p/nativeclient/issues/detail?id=877
TEST= run_nacl_text_no_pad_test run_nacl_text_small_pad_test run_nacl_text_large_pad_test run_nacl_text_too_small_pad_test run_nacl_text_no_pad_ro_test run_nacl_text_small_pad_ro_test run_nacl_text_large_pad_ro_test run_nacl_text_too_small_pad_ro_test run_nacl_text_no_pad_ro_dyn_test run_nacl_text_small_pad_ro_dyn_test run_nacl_text_large_pad_ro_dyn_test run_nacl_text_too_small_pad_ro_dyn_test


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

[bsy, 2010-11-04 23:55:07]

PTAL

[Cliff L. Biffle, 2010-11-05 20:32:02]

LGTM - no objections to you landing this.  I have a few thoughts on further
simplifying things below.

http://codereview.chromium.org/4181005/diff/48001/49003
File src/trusted/service_runtime/arch/arm/nacl_text_pad_test.S (right):

http://codereview.chromium.org/4181005/diff/48001/49003#newcode11
src/trusted/service_runtime/arch/arm/nacl_text_pad_test.S:11: * This is a
translation from the x86-{32,64} code, and is
I'm not convinced this actually makes it easier to read for anyone. 
x86-speakers will have a hard time reading/reasoning about ARM conventions. 
ARM-speakers (like me) have a hard time with this sort of glossed code.  If I
were to rewrite this after you checked in, would it make you unhappy?

http://codereview.chromium.org/4181005/diff/48001/49003#newcode18
src/trusted/service_runtime/arch/arm/nacl_text_pad_test.S:18: .global
__aeabi_uidivmod
You can remove this, no?

http://codereview.chromium.org/4181005/diff/48001/49003#newcode26
src/trusted/service_runtime/arch/arm/nacl_text_pad_test.S:26: * use decimal
output (yet).
If we don't use decimal output, can we remove this whole comment?  I think
hex-only seems reasonable.

http://codereview.chromium.org/4181005/diff/48001/49006
File src/trusted/service_runtime/nacl.scons (right):

http://codereview.chromium.org/4181005/diff/48001/49006#newcode1
src/trusted/service_runtime/nacl.scons:1: # -*- python -*-
Is src/trusted/service_runtime the right place for all this?  Shouldn't it be
under tests/ somewhere?

http://codereview.chromium.org/4181005/diff/48001/49006#newcode121
src/trusted/service_runtime/nacl.scons:121: # directive is used, so none of the
tests that leaves room for
Have you filed a bug for this behavior?  It seems...bad.

http://codereview.chromium.org/4181005/diff/48001/49006#newcode178
src/trusted/service_runtime/nacl.scons:178: env.AddNodeToTestSuite(node,
['small_tests'], 'run_' + base_name)
Heeeeey...would you be willing to add a new test suite for this?  Since the full
set of names isn't statically obvious.  We could call it 'sel_ldr_sled_tests' or
something.  It is literally as easy as inserting a new element in this here
array.

http://codereview.chromium.org/4181005/diff/48001/49010
File src/trusted/service_runtime/sel_ldr_standard.c (right):

http://codereview.chromium.org/4181005/diff/48001/49010#newcode53
src/trusted/service_runtime/sel_ldr_standard.c:53: * check for running off the
end of the x86 code segment.
Methinks this safeguard is not sufficient for such a bug -- unless the CPU only
enforced segmentation on branches but not straight-line code, which seems like
we would've heard about it by now.

http://codereview.chromium.org/4181005/diff/48001/49013
File tools/elf_patcher.py (right):

http://codereview.chromium.org/4181005/diff/48001/49013#newcode24
tools/elf_patcher.py:24: extractors = [ r'#\s*define\s+' + s + '\s+([x\d]+)'
The right side of this string lacks the 'r' prefix, but it sure looks like it
needs it.  I suggest using % to avoid this.

http://codereview.chromium.org/4181005/diff/48001/49013#newcode27
tools/elf_patcher.py:27: nfas = [ re.compile(extractor) for extractor in
extractors ]
You don't use 'extractors' anywhere else, so you could combine these expressions
pretty easily.

http://codereview.chromium.org/4181005/diff/48001/49013#newcode40
tools/elf_patcher.py:40: return in_str[:pos] + replacement + in_str[pos+1:]
The name of this function doesn't make its behavior clear to me -- this sure
looks like a really complicated way of doing
  str[pos] = replacement

http://codereview.chromium.org/4181005/diff/48001/49014
File tools/set_abi_version.py (right):

http://codereview.chromium.org/4181005/diff/48001/49014#newcode51
tools/set_abi_version.py:51: os_number_nacl = 123
So why is *this* one lowercase?

http://codereview.chromium.org/4181005/diff/48001/49014#newcode54
tools/set_abi_version.py:54: nfa_os =
re.compile(r'#\s*define\s+ELFOSABI_NACL\s+(\d+)')
This duplicates some logic in the other file, should we move it to something
central?

[bsy, 2010-11-06 23:47:01]

PTAL

http://codereview.chromium.org/4181005/diff/48001/49003
File src/trusted/service_runtime/arch/arm/nacl_text_pad_test.S (right):

http://codereview.chromium.org/4181005/diff/48001/49003#newcode11
src/trusted/service_runtime/arch/arm/nacl_text_pad_test.S:11: * This is a
translation from the x86-{32,64} code, and is
On 2010/11/05 20:32:02, Cliff L. Biffle wrote:
> I'm not convinced this actually makes it easier to read for anyone. 
> x86-speakers will have a hard time reading/reasoning about ARM conventions. 
> ARM-speakers (like me) have a hard time with this sort of glossed code.  If I
> were to rewrite this after you checked in, would it make you unhappy?

it would be good if you would rewrite it to be more idiomatic, since that would
improve readability for those familiar with arm assembler.  as long as it's not
so different that x86 speakers can't read it any more...

http://codereview.chromium.org/4181005/diff/48001/49003#newcode18
src/trusted/service_runtime/arch/arm/nacl_text_pad_test.S:18: .global
__aeabi_uidivmod
On 2010/11/05 20:32:02, Cliff L. Biffle wrote:
> You can remove this, no?

Done.

http://codereview.chromium.org/4181005/diff/48001/49003#newcode26
src/trusted/service_runtime/arch/arm/nacl_text_pad_test.S:26: * use decimal
output (yet).
On 2010/11/05 20:32:02, Cliff L. Biffle wrote:
> If we don't use decimal output, can we remove this whole comment?  I think
> hex-only seems reasonable.

i think it's useful to retain the potential alternative below, should we decide
that we need other glibc code in case we need to extend this test.  we could
omit the part about decimal output, since that only makes sense in contrast with
the x86 code and only serve to provide the motivation that led to the scheme
below.

http://codereview.chromium.org/4181005/diff/48001/49010
File src/trusted/service_runtime/sel_ldr_standard.c (right):

http://codereview.chromium.org/4181005/diff/48001/49010#newcode53
src/trusted/service_runtime/sel_ldr_standard.c:53: * check for running off the
end of the x86 code segment.
On 2010/11/05 20:32:02, Cliff L. Biffle wrote:
> Methinks this safeguard is not sufficient for such a bug -- unless the CPU
only
> enforced segmentation on branches but not straight-line code, which seems like
> we would've heard about it by now.

i think the reason why we added this was that we heard (rumors?) of such a
fall-through bug, albeit it might have been one that required a long instruction
that crossed the page boundary -- one which we should have discovered in the
validator and caused us to reject the nexe.  belt-and-suspenders.

http://codereview.chromium.org/4181005/diff/48001/49013
File tools/elf_patcher.py (right):

http://codereview.chromium.org/4181005/diff/48001/49013#newcode24
tools/elf_patcher.py:24: extractors = [ r'#\s*define\s+' + s + '\s+([x\d]+)'
On 2010/11/05 20:32:02, Cliff L. Biffle wrote:
> The right side of this string lacks the 'r' prefix, but it sure looks like it
> needs it.  I suggest using % to avoid this.

Done.

http://codereview.chromium.org/4181005/diff/48001/49013#newcode27
tools/elf_patcher.py:27: nfas = [ re.compile(extractor) for extractor in
extractors ]
On 2010/11/05 20:32:02, Cliff L. Biffle wrote:
> You don't use 'extractors' anywhere else, so you could combine these
expressions
> pretty easily.

i prefer this for better readability.

http://codereview.chromium.org/4181005/diff/48001/49013#newcode40
tools/elf_patcher.py:40: return in_str[:pos] + replacement + in_str[pos+1:]
On 2010/11/05 20:32:02, Cliff L. Biffle wrote:
> The name of this function doesn't make its behavior clear to me -- this sure
> looks like a really complicated way of doing
>   str[pos] = replacement

python strings are immutable, viz:


$ python
Python 2.6.5 (r265:79063, Apr 16 2010, 13:57:41) 
[GCC 4.4.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> s="deadbeef"
>>> s[3]="f"
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'str' object does not support item assignment
>>> 

thus resurrection at the cost of hearing is impossible.

http://codereview.chromium.org/4181005/diff/48001/49014
File tools/set_abi_version.py (right):

http://codereview.chromium.org/4181005/diff/48001/49014#newcode51
tools/set_abi_version.py:51: os_number_nacl = 123
On 2010/11/05 20:32:02, Cliff L. Biffle wrote:
> So why is *this* one lowercase?

i have no idea.  i plead temporary insanity.

http://codereview.chromium.org/4181005/diff/48001/49014#newcode54
tools/set_abi_version.py:54: nfa_os =
re.compile(r'#\s*define\s+ELFOSABI_NACL\s+(\d+)')
Done.

[sehr (please use chromium), 2010-11-08 17:01:23]

One egregious agreement.  Otherwise, LGTM.

http://codereview.chromium.org/4181005/diff/48001/49006
File src/trusted/service_runtime/nacl.scons (right):

http://codereview.chromium.org/4181005/diff/48001/49006#newcode178
src/trusted/service_runtime/nacl.scons:178: env.AddNodeToTestSuite(node,
['small_tests'], 'run_' + base_name)
On 2010/11/05 20:32:02, Cliff L. Biffle wrote:
> Heeeeey...would you be willing to add a new test suite for this?  Since the
full
> set of names isn't statically obvious.  We could call it 'sel_ldr_sled_tests'
or
> something.  It is literally as easy as inserting a new element in this here
> array.

Amen.  Test base factoring is really nice to have.

[bsy, 2010-11-08 18:48:25]

thx.

http://codereview.chromium.org/4181005/diff/48001/49006
File src/trusted/service_runtime/nacl.scons (right):

http://codereview.chromium.org/4181005/diff/48001/49006#newcode1
src/trusted/service_runtime/nacl.scons:1: # -*- python -*-
On 2010/11/05 20:32:02, Cliff L. Biffle wrote:
> Is src/trusted/service_runtime the right place for all this?  Shouldn't it be
> under tests/ somewhere?

the convention that we've been using afaik is that things that test a particular
functionality and is fast are unit tests (or small/medium in the
small/medium/large terminology) and are colocated with the code that they're
testing, with test source files named *_test.*, but larger, more system testing
style stuff are in tests/.  these test do require most of the service runtime to
be operational, but only test (and depend on) only a tiny bit of functionality
(number of syscalls, number of libraries), so i think this is the appropriate
place.

http://codereview.chromium.org/4181005/diff/48001/49006#newcode121
src/trusted/service_runtime/nacl.scons:121: # directive is used, so none of the
tests that leaves room for
On 2010/11/05 20:32:02, Cliff L. Biffle wrote:
> Have you filed a bug for this behavior?  It seems...bad.

Done.  This is the code sourcery linker, so unclear how much we can do....

http://codereview.chromium.org/4181005/diff/48001/49006#newcode178
src/trusted/service_runtime/nacl.scons:178: env.AddNodeToTestSuite(node,
['small_tests'], 'run_' + base_name)
On 2010/11/08 17:01:23, sehr wrote:
> On 2010/11/05 20:32:02, Cliff L. Biffle wrote:
> > Heeeeey...would you be willing to add a new test suite for this?  Since the
> full
> > set of names isn't statically obvious.  We could call it
'sel_ldr_sled_tests'
> or
> > something.  It is literally as easy as inserting a new element in this here
> > array.
> 
> Amen.  Test base factoring is really nice to have.

Done.  missed the comments in this file on the first go-around somehow....

[Mark Seaborn, 2010-11-08 19:49:25]

On 2010/11/08 18:48:25, bsy wrote:
> http://codereview.chromium.org/4181005/diff/48001/49006
> File src/trusted/service_runtime/nacl.scons (right):
> 
> http://codereview.chromium.org/4181005/diff/48001/49006#newcode1
> src/trusted/service_runtime/nacl.scons:1: # -*- python -*-
> On 2010/11/05 20:32:02, Cliff L. Biffle wrote:
> > Is src/trusted/service_runtime the right place for all this?
> > Shouldn't it be under tests/ somewhere?

> the convention that we've been using afaik is that things that test
> a particular functionality and is fast are unit tests (or
> small/medium in the small/medium/large terminology) and are
> colocated with the code that they're testing, with test source files
> named *_test.*, but larger, more system testing style stuff are in
> tests/.  these test do require most of the service runtime to be
> operational, but only test (and depend on) only a tiny bit of
> functionality (number of syscalls, number of libraries), so i think
> this is the appropriate place.

I'd say if it's testing a public interface (e.g. invoking sel_ldr from
the command line), put it in "tests".  If it's testing internal
interfaces (e.g. producing a new trusted executable and linking
against internal libraries), put it in "src/trusted".

That seeems to be the convention that has emerged.  This is
potentially useful, because we could (in principle) mix and match
revisions of "src" and "tests" in order to test whether public
interfaces have changed.

By that convention, this new test would go in "tests", AAUI.

The small/medium/large distinction isn't doing much these days, as few
tests are in "medium" or "large".

Mark

[bsy, 2010-11-09 02:53:06]

r3635