first step in factoring out code dealing with elf into a separate library....

first step in factoring  out code dealing with elf into a separate library.
This should eventually allow code reuse by the validor(s).
It also reduces the complexity of the nap struct.


Committed: http://code.google.com/p/nativeclient/source/detail?r=1008

[robertm, 2009-11-12 17:24:23]

Note: this is a somewhat preliminary CL that just barely compiles and the gyp
changes are missing completely.

I am interested whetehr there is support for adding more FATAL messages
which would also allow me to get rid of some of the error codes which 
will make it harder to split this into a separate library.

[robertm, 2009-11-12 20:32:26]

This CL no passed the try bot please have a look.


On 2009/11/12 17:24:23, robertm wrote:
> Note: this is a somewhat preliminary CL that just barely compiles and the gyp
> changes are missing completely.
> 
> I am interested whetehr there is support for adding more FATAL messages
> which would also allow me to get rid of some of the error codes which 
> will make it harder to split this into a separate library.

[Mark Seaborn, 2009-11-12 23:50:02]

A description of what logic has moved where and why would be useful, because a
fair amount of code is being moved here.

elf_util.c is not a good name because this contains very NaCl-specific logic for
what ELF headers are considered acceptable.

I'm not convinced that splitting this logic into even more pieces is beneficial.

Mark

[bsy, 2009-11-13 21:12:00]

some feedback.

btw, i have a cl (shm-for-text) that also touches on some of these files, so it
might be good to serialize.  right now i'm tweaking linker scripts to get arm
tests in-line w/ my changes, and it should be ready for review soon.

http://codereview.chromium.org/389022/diff/5005/2016
File src/trusted/service_runtime/elf_util.c (right):

http://codereview.chromium.org/389022/diff/5005/2016#newcode360
Line 360: struct NaClElfImage *NaClElfImageNew(struct Gio  *gp) {
i generally prefer to let user allocate space rather than force the use of
malloc, so the NaClElfImage object can be allocated as part of another struct. 
this means that the user must know the size of the struct, so a simple forward
declaration does not suffice.  working with C code, i'm happy with a /* private:
*/ comment, but of course YMMV.

http://codereview.chromium.org/389022/diff/5005/2016#newcode481
Line 481: /* TODO(robertm): this code seems wrong - why do we default to 32 bit?
*/
earlier binaries did not contain an e_flags bit to support 16-byte bundles,
since the x86_32 toolchain only supported 32-byte bundles.  the default could
turn into an error now, since there should not be any old binaries built by the
old tool-chain extant.

http://codereview.chromium.org/389022/diff/5005/2015
File src/trusted/service_runtime/mmap_test.c (right):

http://codereview.chromium.org/389022/diff/5005/2015#newcode112
Line 112: /* check abi */ 1);
why not use an enum?

http://codereview.chromium.org/389022/diff/5005/2012
File src/trusted/service_runtime/sel_ldr.h (left):

http://codereview.chromium.org/389022/diff/5005/2012#oldcode318
Line 318: enum NaClAbiMismatchOption {
why did you get rid of this enum?  style discussions often suggest using an enum
w/ mnemonic names rather than true/false or 1/0 with a comment.

[bsy, 2009-11-13 21:14:09]

On 2009/11/12 17:24:23, robertm wrote:
> Note: this is a somewhat preliminary CL that just barely compiles and the gyp
> changes are missing completely.
> 
> I am interested whetehr there is support for adding more FATAL messages
> which would also allow me to get rid of some of the error codes which 
> will make it harder to split this into a separate library.

i think there are use cases for NaCl where it'd be good to be able to handle
malformed untrusted modules w/o aborting.

[Mark Seaborn, 2009-11-16 23:31:14]

http://codereview.chromium.org/389022/diff/6028/6040
File src/trusted/service_runtime/elf_util.c (right):

http://codereview.chromium.org/389022/diff/6028/6040#newcode409
Line 409: /* we delay allocating till the end to avoid cleanup code */
I don't understand the logic behind this comment.  You raise a fatal error if
the allocation fails so it would make no difference to cleanup to do the
allocation at the start, which would be simpler.

http://codereview.chromium.org/389022/diff/6028/6040#newcode431
Line 431: const Elf32_Phdr  *php = &image->phdrs[segnum];
Inconsistent indentation.

[irina.tuduce, 2009-11-18 09:29:51]

I also have changes that touch on some of these files 8-(
Elf64 changes..

[bsy, 2009-11-19 01:34:33]

minor nits.  plz fix.  o/w lgtm.

http://codereview.chromium.org/389022/diff/6028/6040
File src/trusted/service_runtime/elf_util.c (right):

http://codereview.chromium.org/389022/diff/6028/6040#newcode208
Line 208: /* Scan phdrs and do sanity checks in-line.  Verify that the load
/* on a line by itself.

http://codereview.chromium.org/389022/diff/6028/6040#newcode235
Line 235: /* TODO(robertm): check whether this is still valid with
use
/*
 * comment text
 */
style comments plz.

http://codereview.chromium.org/389022/diff/6028/6040#newcode362
Line 362: struct NaClElfImage* result;
use struct NaClElfImage *result to be consistent w/ rest of service runtime code
plz.

http://codereview.chromium.org/389022/diff/6028/6040#newcode422
Line 422: NaClErrorCode NaClElfImageLoad(struct NaClElfImage *image,
indent formal params so they line up like the rest of the code plz.

http://codereview.chromium.org/389022/diff/6028/6037
File src/trusted/service_runtime/elf_util.h (right):

http://codereview.chromium.org/389022/diff/6028/6037#newcode2
Line 2: * Copyright 2008, Google Inc.
use short copyright esp in new files plz.

http://codereview.chromium.org/389022/diff/6028/6029
File src/trusted/service_runtime/sel_ldr_standard.c (right):

http://codereview.chromium.org/389022/diff/6028/6029#newcode71
Line 71: struct NaClElfImage  *image = 0;
use NULL for pointers, 0 for integers (and "booleans") plz.