Adds tls_edit utility which patches irt_core.nexe's TLS usage.

Adds tls_edit utility which patches irt_core.nexe's TLS usage.

Currently we compile in different instructions to manage the 2 TLS
usages between an untrusted nacl app and the IRT. This adds a new
utility (tls_edit) which goes through nexe's and edits the
instructions which use the TLS to point to the correct location.
This way we don't have to do anything special when compiling the
IRT.

The ComponentProgram function has also been separated so creating
of the ComponentProgram is done separately from aliasing programs
to the "COMPONENT_PROGRAM_GROUPS" aliases. This way we can add in
extra steps between the building of the Component Program and
adding the program to the correct component program aliases. This
is now necessary because the IRT core is now built and uses
tls_edit as an intermediate step before the nexe is complete.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=3127
TEST= trybots
R=bradnelson@google.com, mcgrathr@chromium.org, mseaborn@chromium.org

Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=12698

[David Yen, 2014-01-20 20:31:01]

Finally got tls_edit compiling and working on all platforms in both scons and
gyp.

[Roland McGrath, 2014-01-24 18:50:48]

https://codereview.chromium.org/140653005/diff/2570001/SConstruct
File SConstruct (right):

https://codereview.chromium.org/140653005/diff/2570001/SConstruct#newcode3612
SConstruct:3612: # KEEP THIS SORTED PLEASE
This should have a comment saying that it's tls_edit that needs these subdirs.

https://codereview.chromium.org/140653005/diff/2570001/SConstruct#newcode3646
SConstruct:3646: if opt_level not in build_env_map or \
Use parens around a wrapped expr, and no backslash.

https://codereview.chromium.org/140653005/diff/2570001/SConstruct#newcode3717
SConstruct:3717: # build process must built using the current build environment,
not the
s/must built/must be built/

https://codereview.chromium.org/140653005/diff/2570001/src/untrusted/irt/tls_...
File src/untrusted/irt/tls_edit.c (right):

https://codereview.chromium.org/140653005/diff/2570001/src/untrusted/irt/tls_...
src/untrusted/irt/tls_edit.c:1: #include <errno.h>
Needs copyright header at top.

https://codereview.chromium.org/140653005/diff/2570001/src/untrusted/irt/tls_...
src/untrusted/irt/tls_edit.c:92: fprintf(stderr, "open: %s: %s\n",
Say fopen in the message since that's what you called.

https://codereview.chromium.org/140653005/diff/2570001/src/untrusted/irt/tls_...
src/untrusted/irt/tls_edit.c:114: int error_num = ferror(fp);
ferror and feof return a Boolean value, not an errno code.
If ferror returns true, then use strerror(errno) in the message.
If feof returns true, just say "premature EOF" and there is no associated errno
value.

https://codereview.chromium.org/140653005/diff/2570001/src/untrusted/irt/tls_...
src/untrusted/irt/tls_edit.c:117: fprintf(stderr, "read: %s: %s\n",
Say fread in the messages since that's what you called.

https://codereview.chromium.org/140653005/diff/2570001/src/untrusted/irt/tls_...
src/untrusted/irt/tls_edit.c:151: static Bool Process32BitFile(const char*
infile)
brace on end of decl line

https://codereview.chromium.org/140653005/diff/2570001/src/untrusted/irt/tls_...
src/untrusted/irt/tls_edit.c:151: static Bool Process32BitFile(const char*
infile)
'const char *infile'.  This is C, not C++.

https://codereview.chromium.org/140653005/diff/2570001/src/untrusted/irt/tls_...
src/untrusted/irt/tls_edit.c:197: if (phdr[i].p_offset > g_contents_size ||
Don't repeat all this logic in the 32/64 variants.
Just call a common function with the values of p_offset, p_filesz, p_vaddr,
e_machine.

https://codereview.chromium.org/140653005/diff/2570001/src/untrusted/irt/tls_...
src/untrusted/irt/tls_edit.c:233: static Bool Process64BitFile(const char*
infile)
brace at end of decl line

https://codereview.chromium.org/140653005/diff/2570001/src/untrusted/irt/tls_...
src/untrusted/irt/tls_edit.c:318: ((Elf32_Ehdr*)g_contents)->e_ident[EI_CLASS]
== ELFCLASS32) {
space before * and after ): '(Elf32_Ehdr *) g_contents'

[David Yen, 2014-01-24 23:22:34]

On 2014/01/24 18:50:48, Roland McGrath wrote:
> https://codereview.chromium.org/140653005/diff/2570001/SConstruct#newcode3612
> SConstruct:3612: # KEEP THIS SORTED PLEASE
> This should have a comment saying that it's tls_edit that needs these subdirs.

I added a comment with your suggestion, but I'm of the opinion that this build
environment should be like any other environment. With that comment it makes
it seem like this build environment is really only a special case hack for
tls_edit which it really shouldn't be. The list of BUILD_SCONSCRIPTS for
the trusted, nacl, nacl_irt environments are all just the definition of the 
environments. It seems out of place that for this one environment, it needs
to justify why it needs to be constructed in this way. Maybe Brad can chime in
what he thinks?

>
https://codereview.chromium.org/140653005/diff/2570001/src/untrusted/irt/tls_...
> src/untrusted/irt/tls_edit.c:197: if (phdr[i].p_offset > g_contents_size ||
> Don't repeat all this logic in the 32/64 variants.
> Just call a common function with the values of p_offset, p_filesz, p_vaddr,
> e_machine.

I played around with trying to merge these together, but because the structures
are
totally different between 32 bit and 64 bit (Elf32/64_Ehdr, Elf32/64_Phdr,
ELFCLASS32/64) I basically need to pass in about not only the values, but also
all 
the various struct sizes and different enumerators which makes everything much
more
complex very quickly. I guess this is why templates were invented. I could
really
only share 2 checks:
  if (ehdr->e_phoff > g_contents_size)
  if (ehdr->e_ident[EI_DATA] != ELFDATA2LSB)

On top of that, there is no easy way to share the actual computation of the phdr
part
unless it were a macro or something like that. This is because it uses different
structures (Elf32_Phdr vs Elf64_Phdr) to loop through the load segments:

  phdr = (const void *) ((uint8_t *) g_contents + ehdr->e_phoff);
  for (i = 0; i < ehdr->e_phnum; ++i) {
    if (phdr[i].p_type == PT_LOAD && (phdr[i].p_flags & PF_X) != 0)
      break;
    }
  if (i == ehdr->e_phnum) {
    fprintf(stderr, "%s: Could not find executable load segment!\n",
            infile);
    return FALSE;
  }

So even if I were to pass in the 8 variables needed to share code, I would only
be able
to share the validation aspect of it.

On the other hand, the 64 bit version really doesn't do anything other than the
input
validation. Another option would be to just NOP on 64 bit files and delete the
code.

[Roland McGrath, 2014-01-31 21:02:01]

Just a few nits left.

https://codereview.chromium.org/140653005/diff/3340001/src/shared/gio/gio.gyp
File src/shared/gio/gio.gyp (right):

https://codereview.chromium.org/140653005/diff/3340001/src/shared/gio/gio.gyp...
src/shared/gio/gio.gyp:25: 'toolsets': ['host', 'target'],
Add a brief comment saying why it's needed for host too.

https://codereview.chromium.org/140653005/diff/3340001/src/shared/platform/pl...
File src/shared/platform/platform.gyp (right):

https://codereview.chromium.org/140653005/diff/3340001/src/shared/platform/pl...
src/shared/platform/platform.gyp:137: 'toolsets': ['host', 'target'],
Ditto

https://codereview.chromium.org/140653005/diff/3340001/src/trusted/validator_...
File src/trusted/validator_ragel/rdfa_validator.gyp (right):

https://codereview.chromium.org/140653005/diff/3340001/src/trusted/validator_...
src/trusted/validator_ragel/rdfa_validator.gyp:13: 'toolsets': ['host',
'target'],
Ditto

https://codereview.chromium.org/140653005/diff/3340001/src/untrusted/irt/tls_...
File src/untrusted/irt/tls_edit.c (right):

https://codereview.chromium.org/140653005/diff/3340001/src/untrusted/irt/tls_...
src/untrusted/irt/tls_edit.c:75: const uint32_t *insn_offset = ((const uint32_t
*) insn_end) - 1;
You should not assume that the build machine is also x86, and thus should not
assume that a misaligned access like this is OK.  Restore it to testing each
byte individually as I'd written it.  Also, if you need a variable like this, it
should be declared in the innermost scope where it can be; here, inside the
first if.

https://codereview.chromium.org/140653005/diff/3340001/src/untrusted/irt/tls_...
src/untrusted/irt/tls_edit.c:82: * Instruction already offset to the right
location
Punctuate the sentence.  Line up the *s below the first one.

In the long run this will be an unexpected case, so add a TODO saying it's
temporary.

https://codereview.chromium.org/140653005/diff/3340001/src/untrusted/irt/tls_...
src/untrusted/irt/tls_edit.c:102: const NaClCPUFeaturesX86* cpu_features =
&kFullCPUIDFeatures;
Space before * not after.  This is not C++.

https://codereview.chromium.org/140653005/diff/3340001/src/untrusted/irt/tls_...
src/untrusted/irt/tls_edit.c:135: int error_num = ferror(fp);
This is a bad name for this variable, and there is no reason to have these
variables at all.  Just call ferror/feof inside the if expression.

https://codereview.chromium.org/140653005/diff/3340001/src/untrusted/irt/tls_...
src/untrusted/irt/tls_edit.c:144: fprintf(stderr, "fread: %s: unknown error\n",
Make it say something like "short read count" and show the two sizes.

https://codereview.chromium.org/140653005/diff/3340001/src/untrusted/irt/tls_...
src/untrusted/irt/tls_edit.c:173: uint8_t* code_start, size_t code_length,
Space before * not after.  This is not C++.

https://codereview.chromium.org/140653005/diff/3340001/src/untrusted/irt/tls_...
src/untrusted/irt/tls_edit.c:196: * TODO: Until we dep roll native_client
changes, no instructions
Convention is to write TODO(username).

Line up the *s with the first one.

https://codereview.chromium.org/140653005/diff/3340001/src/untrusted/irt/tls_...
src/untrusted/irt/tls_edit.c:349: * We will examine the header to figure out
whether or not we are dealing
s/or not//

https://codereview.chromium.org/140653005/diff/3340001/src/untrusted/irt/tls_...
src/untrusted/irt/tls_edit.c:353: ((Elf32_Ehdr*)g_contents)->e_ident[EI_CLASS]
== ELFCLASS32) {
((Elf32_Ehdr *) g_contents)

https://codereview.chromium.org/140653005/diff/3340001/src/untrusted/irt/tls_...
src/untrusted/irt/tls_edit.c:372: if (g_errors != 0)
Brace on end of 'if' line.

https://codereview.chromium.org/140653005/diff/3340001/src/untrusted/irt/tls_...
src/untrusted/irt/tls_edit.c:373: {
Brace on end of 'if' line.

https://codereview.chromium.org/140653005/diff/3340001/src/untrusted/irt/tls_...
src/untrusted/irt/tls_edit.c:374: fprintf(stderr, "blahblah");
Make it a useful message, and add a TODO comment about why no-op is correct for
now.

[bradn, 2014-02-03 20:49:35]

One mistake otherwise LGTM

https://codereview.chromium.org/140653005/diff/3870001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/140653005/diff/3870001/build/common.gypi#newc...
build/common.gypi:304: [ 'target_arch=="arm"', {
The extra space here is kinda weird, drop it while you're in there.

https://codereview.chromium.org/140653005/diff/3870001/build/common.gypi#newc...
build/common.gypi:305: 'target_conditions': [
This is indented oddly, it should be two spaces at each level.

[David Yen, 2014-02-03 21:14:38]

I fixed up the whitespace issues Brad pointed out, I also added Mark to the
reviewer list so he can approve the rdfa_validator.gyp change

[Roland McGrath, 2014-02-03 21:25:13]

lgtm

https://codereview.chromium.org/140653005/diff/200011/src/untrusted/irt/tls_e...
File src/untrusted/irt/tls_edit.c (right):

https://codereview.chromium.org/140653005/diff/200011/src/untrusted/irt/tls_e...
src/untrusted/irt/tls_edit.c:26: # define O_BINARY 0
This is unused since you switched to stdio.  Drop it.
Probably can also drop #include <fcntl.h>.

[Mark Seaborn, 2014-02-03 22:31:47]

https://codereview.chromium.org/140653005/diff/200011/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/140653005/diff/200011/build/common.gypi#newco...
build/common.gypi:359: 'conditions': [
The indentation here doesn't match that of 'asflags' below, so this should be
indented more to match.

https://codereview.chromium.org/140653005/diff/200011/site_scons/site_tools/c...
File site_scons/site_tools/component_builders.py (right):

https://codereview.chromium.org/140653005/diff/200011/site_scons/site_tools/c...
site_scons/site_tools/component_builders.py:502: env.ComponentProgramAlias(a)
Can you explain these changes in the commit message?

https://codereview.chromium.org/140653005/diff/200011/src/untrusted/irt/DEPS
File src/untrusted/irt/DEPS (right):

https://codereview.chromium.org/140653005/diff/200011/src/untrusted/irt/DEPS#...
src/untrusted/irt/DEPS:2: # Need validator for tls_edit.
Having src/untrusted/irt depend on src/trusted/validator_ragel does not seem
right.

I'd say tls_edit.c should go somewhere in src/trusted, src/tools or
src/tools/validator_tools -- probably the last one of these.

https://codereview.chromium.org/140653005/diff/200011/src/untrusted/irt/nacl....
File src/untrusted/irt/nacl.scons (right):

https://codereview.chromium.org/140653005/diff/200011/src/untrusted/irt/nacl....
src/untrusted/irt/nacl.scons:105: tls_edit_library =
tls_edit_env.Install('${STAGING_DIR}', tls_edit_exe)
Why "_library"?  That's a little confusing since this is an executable.

https://codereview.chromium.org/140653005/diff/200011/src/untrusted/irt/nacl....
src/untrusted/irt/nacl.scons:111: )
Nit: closing bracket goes on previous line in normal Python style

https://codereview.chromium.org/140653005/diff/200011/src/untrusted/irt/tls_e...
File src/untrusted/irt/tls_edit.c (right):

https://codereview.chromium.org/140653005/diff/200011/src/untrusted/irt/tls_e...
src/untrusted/irt/tls_edit.c:191: default:
Can you make tls_edit a no-op on MIPS so that this doesn't break the MIPS build?

https://codereview.chromium.org/140653005/diff/200011/src/untrusted/irt/tls_e...
src/untrusted/irt/tls_edit.c:198: * TODO: Until we dep roll native_client
changes, no instructions
Nit: TODOs should have a name, i.e. "TODO(dyen):".

https://codereview.chromium.org/140653005/diff/200011/src/untrusted/irt/tls_e...
src/untrusted/irt/tls_edit.c:199: * should be changed. No changes to make should
return FALSE eventually.
FWIW, that's not necessarily true if you happen to have a trivial IRT
implementation (e.g. for testing) that happens not to use TLS at all.

https://codereview.chromium.org/140653005/diff/200011/tests/irt_compatibility...
File tests/irt_compatibility/nacl.scons (right):

https://codereview.chromium.org/140653005/diff/200011/tests/irt_compatibility...
tests/irt_compatibility/nacl.scons:29: irt_comp_test_library = env.Command(
How about putting an ApplyTlsEdit() command in SConstruct instead of
GetTlsEdit(), since calling env.Command() is a shared pattern?

[David Yen, 2014-02-03 23:07:10]

All of Mark's suggestions make sense to me, I applied them in the last patch.

The only thing I didn't change is the location of tls_edit source files, perhaps
that warrants a bit more discussion?

[Roland McGrath, 2014-02-03 23:11:07]

On 2014/02/03 23:07:10, David Yen wrote:
> All of Mark's suggestions make sense to me, I applied them in the last patch.
> 
> The only thing I didn't change is the location of tls_edit source files,
perhaps
> that warrants a bit more discussion?

I think having tls_edit.c in irt/ makes sense because that's where it's used.
But I don't have a strong opinion, and Mark probably does.
Certainly the source file should be in the same directory with the scons file
that builds the program.

[David Yen, 2014-02-03 23:52:10]

I moved tls_edit from src/untrusted/irt to src/tools/tls_edit (And removed
irt/DEPS in the process). I figured it wasn't really a validator so it didn't
make sense to me to put it into validator_tools.

The change was pretty significant so if everyone could give it another look over
that would be great.

[Roland McGrath, 2014-02-04 17:49:31]

https://codereview.chromium.org/140653005/diff/4490004/src/untrusted/irt/tls_...
File src/untrusted/irt/tls_edit.c (right):

https://codereview.chromium.org/140653005/diff/4490004/src/untrusted/irt/tls_...
src/untrusted/irt/tls_edit.c:187: printf("%s: MIP ELF detected, no instructions
changed\n",
typo: MIPS

[Mark Seaborn, 2014-02-04 18:09:10]

LGTM

https://codereview.chromium.org/140653005/diff/4860001/SConstruct
File SConstruct (right):

https://codereview.chromium.org/140653005/diff/4860001/SConstruct#newcode3728
SConstruct:3728: # When building nacl, any intermediate build tool that is used
during the
Nit: capitalise as "NaCl"

https://codereview.chromium.org/140653005/diff/4860001/src/tools/tls_edit/tls...
File src/tools/tls_edit/tls_edit.c (right):

https://codereview.chromium.org/140653005/diff/4860001/src/tools/tls_edit/tls...
src/tools/tls_edit/tls_edit.c:85: * The Instruction is already offset to the
right location.
Lower case "instruction"

https://codereview.chromium.org/140653005/diff/4860001/src/tools/tls_edit/tls...
src/tools/tls_edit/tls_edit.c:130: {
Nit: putting in a {} block just to declare a variable isn't the usual NaCl
style.  I'd put "size_t read_bytes;" at the start of the function.

https://codereview.chromium.org/140653005/diff/4860001/src/tools/tls_edit/tls...
src/tools/tls_edit/tls_edit.c:158: {
ditto

https://codereview.chromium.org/140653005/diff/4860001/src/tools/tls_edit/tls...
src/tools/tls_edit/tls_edit.c:264: static Bool Process64BitFile(const char
*infile) {
It's not very nice to duplicate this function between 32-bit and 64-bit.  If you
compile this as C++ instead, you can use templates to remove the duplication.

https://codereview.chromium.org/140653005/diff/4860001/src/tools/tls_edit/tls...
src/tools/tls_edit/tls_edit.c:350: ((Elf32_Ehdr*) g_contents)->e_ident[EI_CLASS]
== ELFCLASS32) {
Nit: NaCl spacing style is "type *"

https://codereview.chromium.org/140653005/diff/4860001/src/tools/tls_edit/tls...
src/tools/tls_edit/tls_edit.c:357: ((Elf64_Ehdr*) g_contents)->e_ident[EI_CLASS]
== ELFCLASS64) {
ditto

https://codereview.chromium.org/140653005/diff/4860001/src/untrusted/irt/irt.gyp
File src/untrusted/irt/irt.gyp (right):

https://codereview.chromium.org/140653005/diff/4860001/src/untrusted/irt/irt....
src/untrusted/irt/irt.gyp:122: 'action_name': 'tls_edit_irt_x64',
"x86-64" rather than "x64".  We don't use the term "x64" in NaCl.  Same for the
filename below.

https://codereview.chromium.org/140653005/diff/4860001/src/untrusted/irt/irt....
src/untrusted/irt/irt.gyp:130: '<(PRODUCT_DIR)/irt_core_newlib_x64.nexe',
"irt_core" shouldn't have "newlib" in the name because we always build it with
newlib.  (Same for the other two cases.)

If this is outputting to the wrong filename, I don't see how this would work,
since there is a test that uses the Gyp-built IRT.

https://codereview.chromium.org/140653005/diff/4860001/src/untrusted/irt/irt....
src/untrusted/irt/irt.gyp:135: 'action_name': 'tls_edit_irt_x32',
"x86-32" (not the same as "x32"!)

[David Yen, 2014-02-04 18:28:07]

On 2014/02/04 18:09:10, Mark Seaborn wrote:

I've fixed all the issues except for the one below:

>
https://codereview.chromium.org/140653005/diff/4860001/src/untrusted/irt/irt.gyp
> File src/untrusted/irt/irt.gyp (right):
> 
>
https://codereview.chromium.org/140653005/diff/4860001/src/untrusted/irt/irt....
> src/untrusted/irt/irt.gyp:122: 'action_name': 'tls_edit_irt_x64',
> "x86-64" rather than "x64".  We don't use the term "x64" in NaCl.  Same for
the
> filename below.
> 
>
https://codereview.chromium.org/140653005/diff/4860001/src/untrusted/irt/irt....
> src/untrusted/irt/irt.gyp:130: '<(PRODUCT_DIR)/irt_core_newlib_x64.nexe',
> "irt_core" shouldn't have "newlib" in the name because we always build it with
> newlib.  (Same for the other two cases.)
> 
> If this is outputting to the wrong filename, I don't see how this would work,
> since there is a test that uses the Gyp-built IRT.

As your comment suggests, this is the standard filename. See untrusted.gypi:
(321): 'out_newlib64%': '<(PRODUCT_DIR)/>(nexe_target)_newlib_x64.nexe',
(411): 'out_newlib32%': '<(PRODUCT_DIR)/>(nexe_target)_newlib_x32.nexe',

[David Yen, 2014-02-04 19:29:47]

Committed patchset #32 manually as r12698 (presubmit successful).

[Mark Seaborn, 2014-02-05 19:22:27]

After this change, I get the following when running the scons build on Windows:

link /nologo /safeseh /DEBUG -manifest
/OUT:scons-out\nacl_irt-x86-64\obj\src\tools\tls_edit\tls_edit.exe
/LIBPATH:scons-out\dbg-win-x86-32\lib rdfa_validator.lib platform.lib gio.lib
ws2_32.lib advapi32.lib
/PDB:scons-out\nacl_irt-x86-64\obj\src\tools\tls_edit\tls_edit.pdb /DEBUG
scons-out\nacl_irt-x86-64\obj\src\tools\tls_edit\tls_edit.obj
LINK : fatal error LNK1246: '/SAFESEH' not compatible with 'x64' target machine;
link without '/SAFESEH'

In my setup, I run scons with PATH set to point to the 64-bit version of 'cl'
(from /cygdrive/c/Program Files (x86)/Microsoft Visual Studio
10.0/VC/Bin/amd64).  I expect that is the cause.

[David Yen, 2014-02-05 23:50:26]

On 2014/02/05 19:22:27, Mark Seaborn wrote:
> After this change, I get the following when running the scons build on
Windows:
> 
> link /nologo /safeseh /DEBUG -manifest
> /OUT:scons-out\nacl_irt-x86-64\obj\src\tools\tls_edit\tls_edit.exe
> /LIBPATH:scons-out\dbg-win-x86-32\lib rdfa_validator.lib platform.lib gio.lib
> ws2_32.lib advapi32.lib
> /PDB:scons-out\nacl_irt-x86-64\obj\src\tools\tls_edit\tls_edit.pdb /DEBUG
> scons-out\nacl_irt-x86-64\obj\src\tools\tls_edit\tls_edit.obj
> LINK : fatal error LNK1246: '/SAFESEH' not compatible with 'x64' target
machine;
> link without '/SAFESEH'
> 
> In my setup, I run scons with PATH set to point to the 64-bit version of 'cl'
> (from /cygdrive/c/Program Files (x86)/Microsoft Visual Studio
> 10.0/VC/Bin/amd64).  I expect that is the cause.

I looked into this a bit with Brad, it looks like it has something to do with
not figuring out the right target platform for your build environment. Is this
on your windows machine at work? Lets take a look on your machine when you're
back.