Allow sel_ldr to continue without the IRT, when nexe doesn't have segment gap

Allow sel_ldr to continue without the IRT, when nexe doesn't
have a segment gap.  This allows the specially-built PNaCl
translator nexes to be run without plumbing through flags
from Chrome.  Once we clean up the various code-paths for
NACL_STANDALONE, etc., it may be easier to make and
activate such a flag.

BUG= http://code.google.com/p/nativeclient/issues/detail?id=2877
TEST= run a nexe that doesn't have a segment gap, but
supply the IRT anyway (as chrome would).

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

[jvoung (off chromium), 2012-06-26 22:14:57]

ah sorry, hold on, need to look at the "ff_" tests.

[Mark Seaborn, 2012-06-26 22:19:56]

I would be happier if you plumbed through an option to turn off IRT loading when
loading the translator.  The current change is reversing
https://chromiumcodereview.appspot.com/9307024 which improved error reporting. 
It's obscure if some code paths just ignore an error.

You could change sel_main_chrome.c not to call NaClLoadIrt() if args->irt_fd is
-1.  For sel_ldr, -B is already optional.

Cheers,
Mark

[jvoung - send to chromium..., 2012-06-26 23:29:58]

On 2012/06/26 22:19:56, Mark Seaborn wrote:
> I would be happier if you plumbed through an option to turn off IRT loading
when
> loading the translator.  The current change is reversing
> https://chromiumcodereview.appspot.com/9307024 which improved error reporting.

> It's obscure if some code paths just ignore an error.
> 
> You could change sel_main_chrome.c not to call NaClLoadIrt() if args->irt_fd
is
> -1.  For sel_ldr, -B is already optional.
> 
> Cheers,
> Mark


Is that much better to accept "-1"?

I'm not super familiar with the code, but are you instead suggesting a new
sel_ldr_launcher_chrome/standalone "Start()" which will be used by these helper
nexes?  It looks like something will need to be set much earlier.

* It also looks like nacl_listener.cc on the chrome side will need to be changed
to set the fd to -1, instead of just looking through the "handles" vector.

* There's also the NACL_STANDALONE case, which is currently always sending "-B"
whenever the env var "NACL_IRT_LIBRARY" is set, so toggling that would also need
to be earlier for these helper-nexes, and I'm not sure there is anything earlier
than "Start()" in that case.

[jvoung (off chromium), 2012-06-27 21:53:52]

Okay, changed to check for absence of a segment gap before skipping the load.

[Mark Seaborn, 2012-06-28 22:05:11]

http://codereview.chromium.org/10681005/diff/23003/src/trusted/service_runtim...
File src/trusted/service_runtime/nacl_text.c (right):

http://codereview.chromium.org/10681005/diff/23003/src/trusted/service_runtim...
src/trusted/service_runtime/nacl_text.c:982: return nap->dynamic_text_start !=
nap->dynamic_text_end;
I would suggest checking whether nap->text_shm is NULL instead, since that's
what the code does elsewhere.  That's simple enough that you can just inline it.

http://codereview.chromium.org/10681005/diff/23003/src/trusted/service_runtim...
File src/trusted/service_runtime/sel_main.c (right):

http://codereview.chromium.org/10681005/diff/23003/src/trusted/service_runtim...
src/trusted/service_runtime/sel_main.c:695: fprintf(
Can you use NaClLog() instead of fprintf(), please?  Also, can you add a similar
TODO(mseaborn) here for omitting "-B" when the IRT is not wanted.

http://codereview.chromium.org/10681005/diff/23003/src/trusted/service_runtim...
File src/trusted/service_runtime/sel_main_chrome.c (right):

http://codereview.chromium.org/10681005/diff/23003/src/trusted/service_runtim...
src/trusted/service_runtime/sel_main_chrome.c:285: /* We check if there is a
segment gap before trying to load the IRT.  This
The comment style is
/*
 * blah
 * blah
 */

Can you combine this with the comment on the previous line?

http://codereview.chromium.org/10681005/diff/23003/src/trusted/service_runtim...
src/trusted/service_runtime/sel_main_chrome.c:287: * We should plumb through a
flag from Chrome in such cases, instead of
Can you make this: "TODO(mseaborn): Plumb through a flag from Chromium for such
cases..." as a reminder to me.

http://codereview.chromium.org/10681005/diff/23003/src/trusted/service_runtim...
src/trusted/service_runtime/sel_main_chrome.c:295: "NaCl app doesn't have a
segment gap, not loading IRT.\n"
'executable' rather than 'app'?  "Main executable has no segment gap; skipping
loading IRT library..."

[jvoung (off chromium), 2012-06-28 22:41:55]

http://codereview.chromium.org/10681005/diff/23003/src/trusted/service_runtim...
File src/trusted/service_runtime/nacl_text.c (right):

http://codereview.chromium.org/10681005/diff/23003/src/trusted/service_runtim...
src/trusted/service_runtime/nacl_text.c:982: return nap->dynamic_text_start !=
nap->dynamic_text_end;
On 2012/06/28 22:05:11, Mark Seaborn wrote:
> I would suggest checking whether nap->text_shm is NULL instead, since that's
> what the code does elsewhere.  That's simple enough that you can just inline
it.

Done.

http://codereview.chromium.org/10681005/diff/23003/src/trusted/service_runtim...
File src/trusted/service_runtime/sel_main.c (right):

http://codereview.chromium.org/10681005/diff/23003/src/trusted/service_runtim...
src/trusted/service_runtime/sel_main.c:695: fprintf(
On 2012/06/28 22:05:11, Mark Seaborn wrote:
> Can you use NaClLog() instead of fprintf(), please?  Also, can you add a
similar
> TODO(mseaborn) here for omitting "-B" when the IRT is not wanted.

Done.  The other code around here used fprintf for some reason, so I stuck with
that originally...

http://codereview.chromium.org/10681005/diff/23003/src/trusted/service_runtim...
File src/trusted/service_runtime/sel_main_chrome.c (right):

http://codereview.chromium.org/10681005/diff/23003/src/trusted/service_runtim...
src/trusted/service_runtime/sel_main_chrome.c:285: /* We check if there is a
segment gap before trying to load the IRT.  This
On 2012/06/28 22:05:11, Mark Seaborn wrote:
> The comment style is
> /*
>  * blah
>  * blah
>  */
> 
> Can you combine this with the comment on the previous line?

Done.

http://codereview.chromium.org/10681005/diff/23003/src/trusted/service_runtim...
src/trusted/service_runtime/sel_main_chrome.c:287: * We should plumb through a
flag from Chrome in such cases, instead of
On 2012/06/28 22:05:11, Mark Seaborn wrote:
> Can you make this: "TODO(mseaborn): Plumb through a flag from Chromium for
such
> cases..." as a reminder to me.

Done.

http://codereview.chromium.org/10681005/diff/23003/src/trusted/service_runtim...
src/trusted/service_runtime/sel_main_chrome.c:295: "NaCl app doesn't have a
segment gap, not loading IRT.\n"
On 2012/06/28 22:05:11, Mark Seaborn wrote:
> 'executable' rather than 'app'?  "Main executable has no segment gap; skipping
> loading IRT library..."

Done.

[Mark Seaborn, 2012-06-28 22:50:55]

LGTM, thanks

http://codereview.chromium.org/10681005/diff/19010/src/trusted/service_runtim...
File src/trusted/service_runtime/sel_main.c (right):

http://codereview.chromium.org/10681005/diff/19010/src/trusted/service_runtim...
src/trusted/service_runtime/sel_main.c:697: * of a segment gap, to provide
better diagnostics when the linker
maybe: "when the nexe does not follow NaCl's stable ABI"

http://codereview.chromium.org/10681005/diff/19010/src/trusted/service_runtim...
src/trusted/service_runtime/sel_main.c:701: "Main executable has no segment gap;
not loading IRT library.\n"
Can you keep the warning all on one line, please?  (Make sure to put a space
instead of '\n'.)

http://codereview.chromium.org/10681005/diff/19010/src/trusted/service_runtim...
File src/trusted/service_runtime/sel_main_chrome.c (right):

http://codereview.chromium.org/10681005/diff/19010/src/trusted/service_runtim...
src/trusted/service_runtime/sel_main_chrome.c:289: * diagnostics when the linker
did not create a segment gap!
ditto: "when the nexe does not follow NaCl's stable ABI"

http://codereview.chromium.org/10681005/diff/19010/src/trusted/service_runtim...
src/trusted/service_runtime/sel_main_chrome.c:296: "Main executable has no
segment gap; skipping loading IRT library.\n"
ditto about multi-line

[jvoung - send to chromium..., 2012-06-28 23:02:50]

Thanks, cq'ing

http://codereview.chromium.org/10681005/diff/19010/src/trusted/service_runtim...
File src/trusted/service_runtime/sel_main.c (right):

http://codereview.chromium.org/10681005/diff/19010/src/trusted/service_runtim...
src/trusted/service_runtime/sel_main.c:697: * of a segment gap, to provide
better diagnostics when the linker
On 2012/06/28 22:50:55, Mark Seaborn wrote:
> maybe: "when the nexe does not follow NaCl's stable ABI"

Done.

http://codereview.chromium.org/10681005/diff/19010/src/trusted/service_runtim...
src/trusted/service_runtime/sel_main.c:701: "Main executable has no segment gap;
not loading IRT library.\n"
On 2012/06/28 22:50:55, Mark Seaborn wrote:
> Can you keep the warning all on one line, please?  (Make sure to put a space
> instead of '\n'.)

Done.

http://codereview.chromium.org/10681005/diff/19010/src/trusted/service_runtim...
File src/trusted/service_runtime/sel_main_chrome.c (right):

http://codereview.chromium.org/10681005/diff/19010/src/trusted/service_runtim...
src/trusted/service_runtime/sel_main_chrome.c:289: * diagnostics when the linker
did not create a segment gap!
On 2012/06/28 22:50:55, Mark Seaborn wrote:
> ditto: "when the nexe does not follow NaCl's stable ABI"

Done.

http://codereview.chromium.org/10681005/diff/19010/src/trusted/service_runtim...
src/trusted/service_runtime/sel_main_chrome.c:296: "Main executable has no
segment gap; skipping loading IRT library.\n"
On 2012/06/28 22:50:55, Mark Seaborn wrote:
> ditto about multi-line

Done.

[commit-bot: I haz the power, 2012-06-28 23:26:58]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/jvoung@chromium.org/10681005/35006

[commit-bot: I haz the power, 2012-06-29 00:03:48]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/jvoung@chromium.org/10681005/35006

[commit-bot: I haz the power, 2012-06-29 00:16:38]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is nacl-lucid64-bare-shared, revision is 9053, job
name
was 10681005-35006 (retry) (retry) (retry) (retry).