Add options for base dir and redirecting files to IRT open resource and use it.

Add options for base dir and redirecting files to IRT open resource and use it.

These options are set by env vars: NACL_IRT_OPEN_RESOURCE_BASE and
NACL_IRT_OPEN_RESOURCE_REMAP. The redirection is used to map from
libpnacl_irt_shim.a to libpnacl_irt_shim_dummy.a.

Use these options with the PNaCl sandboxed translator, and
roll PNaCl translator ld DEPS. DEPS roll brings in:
https://codereview.chromium.org/770833002/

Also ensure that SPLIT_MODULE is always set in nativeld.py
so that the code can assume that value is valid.
Change the code so that when SPLIT_MODULE=1, the inputs
are partitioned into llc_output/linker_input files and
other linker input files, just like when SPLIT_MODULE > 1.
The consistent partitioning helps check that all native
libraries can use the same base_dir with open_resource.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=3914
R=mseaborn@chromium.org

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

[jvoung (off chromium), 2014-12-01 19:16:32]

jvoung@chromium.org changed reviewers:
+ dschuff@chromium.org, mseaborn@chromium.org

[Mark Seaborn, 2014-12-01 20:54:17]

https://codereview.chromium.org/731423004/diff/40001/pnacl/driver/nativeld.py
File pnacl/driver/nativeld.py (right):

https://codereview.chromium.org/731423004/diff/40001/pnacl/driver/nativeld.py...
pnacl/driver/nativeld.py:189: first_mainfile = all_inputs.index(main_input)
Is this part just general cleanup, on the grounds that the "if" case can handle
what the "else" case was handling?

This would be worth explaining in the commit message.

https://codereview.chromium.org/731423004/diff/40001/pnacl/driver/pnacl-trans...
File pnacl/driver/pnacl-translate.py (right):

https://codereview.chromium.org/731423004/diff/40001/pnacl/driver/pnacl-trans...
pnacl/driver/pnacl-translate.py:360: # Do not set -streaming-bitcode for
sandboxed mode, because it is already
Nit: shouldn't this go before the "if"?  It's a bit incongruous after the "if".

https://codereview.chromium.org/731423004/diff/40001/src/untrusted/irt/irt_co...
File src/untrusted/irt/irt_core_resource.c (right):

https://codereview.chromium.org/731423004/diff/40001/src/untrusted/irt/irt_co...
src/untrusted/irt/irt_core_resource.c:15: static int
nacl_irt_open_resource_remapped(const char *pathname, int *newfd,
Can you add a comment explaining what this is for?  i.e. Just for testing the
sandboxed translator (specifically the linker) outside of Chromium.

Also say in a sentence what the code does, since it's hard to read the C string
manipulation code.

https://codereview.chromium.org/731423004/diff/40001/src/untrusted/irt/irt_co...
src/untrusted/irt/irt_core_resource.c:19: char merged_path[2048] = "\0";
You can declare this and the others closer to where they're used.  (C99 features
are usable here.)

The initialiser here will memset() the full 2k buffer, which is unnecessary.

https://codereview.chromium.org/731423004/diff/40001/src/untrusted/irt/irt_co...
src/untrusted/irt/irt_core_resource.c:26: if (file_remap) {
Add "!= NULL"

https://codereview.chromium.org/731423004/diff/40001/src/untrusted/irt/irt_co...
src/untrusted/irt/irt_core_resource.c:28: if (delim_ptr) {
Add "!= NULL"

https://codereview.chromium.org/731423004/diff/40001/src/untrusted/irt/irt_co...
src/untrusted/irt/irt_core_resource.c:43: errno = ENAMETOOLONG;
Suggestion: Would it be easier to use VLAs, malloc(), or C++'s std::string
instead of checking for overflows?

https://codereview.chromium.org/731423004/diff/40001/src/untrusted/irt/irt_co...
src/untrusted/irt/irt_core_resource.c:55: if (!base_dir) {
NaCl style is "if (base_dir == NULL)" (no implicit conversion to bool)

[jvoung (off chromium), 2014-12-01 22:00:10]

https://codereview.chromium.org/731423004/diff/40001/pnacl/driver/nativeld.py
File pnacl/driver/nativeld.py (right):

https://codereview.chromium.org/731423004/diff/40001/pnacl/driver/nativeld.py...
pnacl/driver/nativeld.py:189: first_mainfile = all_inputs.index(main_input)
On 2014/12/01 20:54:16, Mark Seaborn wrote:
> Is this part just general cleanup, on the grounds that the "if" case can
handle
> what the "else" case was handling?
> 
> This would be worth explaining in the commit message.


I want the "else" case to be the same as the "if" case, so that the llc files
don't get mixed up with the libraries, when num modules == 1.

Otherwise, the GetNativeLibsDirname() call below will end up seeing some files
in the native lib directory, and a file (the llc output file) in a different
directory, and that breaks the check that all native libs use the same base
directory when setting the env var.

https://codereview.chromium.org/731423004/diff/40001/pnacl/driver/pnacl-trans...
File pnacl/driver/pnacl-translate.py (right):

https://codereview.chromium.org/731423004/diff/40001/pnacl/driver/pnacl-trans...
pnacl/driver/pnacl-translate.py:360: # Do not set -streaming-bitcode for
sandboxed mode, because it is already
On 2014/12/01 20:54:17, Mark Seaborn wrote:
> Nit: shouldn't this go before the "if"?  It's a bit incongruous after the
"if".

Done.

https://codereview.chromium.org/731423004/diff/40001/src/untrusted/irt/irt_co...
File src/untrusted/irt/irt_core_resource.c (right):

https://codereview.chromium.org/731423004/diff/40001/src/untrusted/irt/irt_co...
src/untrusted/irt/irt_core_resource.c:15: static int
nacl_irt_open_resource_remapped(const char *pathname, int *newfd,
On 2014/12/01 20:54:17, Mark Seaborn wrote:
> Can you add a comment explaining what this is for?  i.e. Just for testing the
> sandboxed translator (specifically the linker) outside of Chromium.
> 
> Also say in a sentence what the code does, since it's hard to read the C
string
> manipulation code.

Done.

https://codereview.chromium.org/731423004/diff/40001/src/untrusted/irt/irt_co...
src/untrusted/irt/irt_core_resource.c:19: char merged_path[2048] = "\0";
On 2014/12/01 20:54:17, Mark Seaborn wrote:
> You can declare this and the others closer to where they're used.  (C99
features
> are usable here.)
> 
> The initialiser here will memset() the full 2k buffer, which is unnecessary.

Done.

https://codereview.chromium.org/731423004/diff/40001/src/untrusted/irt/irt_co...
src/untrusted/irt/irt_core_resource.c:26: if (file_remap) {
On 2014/12/01 20:54:17, Mark Seaborn wrote:
> Add "!= NULL"

Done.

https://codereview.chromium.org/731423004/diff/40001/src/untrusted/irt/irt_co...
src/untrusted/irt/irt_core_resource.c:28: if (delim_ptr) {
On 2014/12/01 20:54:17, Mark Seaborn wrote:
> Add "!= NULL"

Done.

https://codereview.chromium.org/731423004/diff/40001/src/untrusted/irt/irt_co...
src/untrusted/irt/irt_core_resource.c:43: errno = ENAMETOOLONG;
On 2014/12/01 20:54:17, Mark Seaborn wrote:
> Suggestion: Would it be easier to use VLAs, malloc(), or C++'s std::string
> instead of checking for overflows?

Okay, I'll just use malloc.

re: c++'s std::string, SCons has a hack that uses pnacl-clang to link the IRT
core instead of pnacl-clang++, to avoid the libc++ plus pthread dependency.
Perhaps we can revert that hack and just live with the pthread dependency
though.

https://codereview.chromium.org/731423004/diff/40001/src/untrusted/irt/irt_co...
src/untrusted/irt/irt_core_resource.c:55: if (!base_dir) {
On 2014/12/01 20:54:17, Mark Seaborn wrote:
> NaCl style is "if (base_dir == NULL)" (no implicit conversion to bool)

Done.

[Mark Seaborn, 2014-12-02 00:42:08]

LGTM, thanks

https://codereview.chromium.org/731423004/diff/40001/src/untrusted/irt/irt_co...
File src/untrusted/irt/irt_core_resource.c (right):

https://codereview.chromium.org/731423004/diff/40001/src/untrusted/irt/irt_co...
src/untrusted/irt/irt_core_resource.c:43: errno = ENAMETOOLONG;
On 2014/12/01 22:00:10, jvoung wrote:
> On 2014/12/01 20:54:17, Mark Seaborn wrote:
> > Suggestion: Would it be easier to use VLAs, malloc(), or C++'s std::string
> > instead of checking for overflows?
> 
> Okay, I'll just use malloc.
> 
> re: c++'s std::string, SCons has a hack that uses pnacl-clang to link the IRT
> core instead of pnacl-clang++, to avoid the libc++ plus pthread dependency.
> Perhaps we can revert that hack and just live with the pthread dependency
> though.

Using malloc() is a good compromise, I think. :-)

https://codereview.chromium.org/731423004/diff/160001/src/untrusted/irt/irt_c...
File src/untrusted/irt/irt_core_resource.c (right):

https://codereview.chromium.org/731423004/diff/160001/src/untrusted/irt/irt_c...
src/untrusted/irt/irt_core_resource.c:17: * An open resource implementation used
for testing PNaCl's sandboxed
Nit: "open resource" -> "open_resource()"

https://codereview.chromium.org/731423004/diff/160001/src/untrusted/irt/irt_c...
src/untrusted/irt/irt_core_resource.c:18: * translator (the linker), outside of
chromium.
Nit: capitalise as "Chromium"

https://codereview.chromium.org/731423004/diff/160001/src/untrusted/irt/irt_c...
src/untrusted/irt/irt_core_resource.c:51: char *merged_path = (char *)malloc(
Nit: add space after "(char *)"

https://codereview.chromium.org/731423004/diff/160001/src/untrusted/irt/irt_c...
src/untrusted/irt/irt_core_resource.c:57: merged_path[0] = '\0';
Use strcpy() instead of this + strcat()?

[jvoung (off chromium), 2014-12-02 16:24:08]

https://codereview.chromium.org/731423004/diff/160001/src/untrusted/irt/irt_c...
File src/untrusted/irt/irt_core_resource.c (right):

https://codereview.chromium.org/731423004/diff/160001/src/untrusted/irt/irt_c...
src/untrusted/irt/irt_core_resource.c:17: * An open resource implementation used
for testing PNaCl's sandboxed
On 2014/12/02 00:42:07, Mark Seaborn wrote:
> Nit: "open resource" -> "open_resource()"

Done.

https://codereview.chromium.org/731423004/diff/160001/src/untrusted/irt/irt_c...
src/untrusted/irt/irt_core_resource.c:18: * translator (the linker), outside of
chromium.
On 2014/12/02 00:42:07, Mark Seaborn wrote:
> Nit: capitalise as "Chromium"

Done.

https://codereview.chromium.org/731423004/diff/160001/src/untrusted/irt/irt_c...
src/untrusted/irt/irt_core_resource.c:51: char *merged_path = (char *)malloc(
On 2014/12/02 00:42:07, Mark Seaborn wrote:
> Nit: add space after "(char *)"

Done.

https://codereview.chromium.org/731423004/diff/160001/src/untrusted/irt/irt_c...
src/untrusted/irt/irt_core_resource.c:57: merged_path[0] = '\0';
On 2014/12/02 00:42:07, Mark Seaborn wrote:
> Use strcpy() instead of this + strcat()?

Done.

https://codereview.chromium.org/731423004/diff/220001/pnacl/driver/pnacl-tran...
File pnacl/driver/pnacl-translate.py (right):

https://codereview.chromium.org/731423004/diff/220001/pnacl/driver/pnacl-tran...
pnacl/driver/pnacl-translate.py:359: if not env.getbool('SANDBOXED') and
env.getbool('STREAM_BITCODE'):
Had to revert this bit to stay within the "else:".

Otherwise -streaming-bitcode seems to also break some of the EH tests:
Alias must point to a definition
void (%"class.std::bad_alloc"*)* @_ZNSt9bad_allocC1Ev
Alias must point to a definition
void (%"class.std::bad_alloc"*)* @_ZNSt9bad_allocD1Ev
Alias must point to a definition
void (%"class.std::bad_array_new_length"*)* @_ZNSt20bad_array_new_lengthC1Ev
Alias must point to a definition
void (%"class.std::bad_array_new_length"*)* @_ZNSt20bad_array_new_lengthD1Ev
Alias must point to a definition
void (%"class.std::bad_array_new_length"*)* @_ZNSt20bad_array_new_lengthD2Ev
Alias must point to a definition
void (%"class.std::bad_array_length"*)* @_ZNSt16bad_array_lengthC1Ev

https://codereview.chromium.org/731423004/diff/220001/src/untrusted/irt/irt_c...
File src/untrusted/irt/irt_core_resource.c (right):

https://codereview.chromium.org/731423004/diff/220001/src/untrusted/irt/irt_c...
src/untrusted/irt/irt_core_resource.c:39: if (strncmp(pathname, file_remap,
remap_from_len) == 0 &&
Had to revert this little bit to what I had in an earlier patch set. Forgot that
file_remap contains the whole "from:to", not just the "from".

[Mark Seaborn, 2014-12-02 16:31:29]

LGTM

[jvoung (off chromium), 2014-12-02 16:40:17]

Committed patchset #12 (id:220001) manually as 14154 (presubmit successful).