Reserve 1GB at the base address of linux nacl_helper for Native Client.

Reserve 1GB at the base address of linux nacl_helper for Native Client.
Use PathService instead of command line flag to obtain path for helper. Adds a nacl_helper_bootstrap executable that implements the actual space reservation.

BUG=nativeclient:480
TEST=manual for now

Committed then reverted. See continuation on CL 7670011.

[Brad Chen, 2011-08-10 18:13:28]

On 2011/08/10 00:03:35, Brad Chen wrote:

The try-bots are green. Windows is a flake.

[Brad Chen, 2011-08-12 21:38:45]

Adding Roland as he knows the linker stuff really well.

[Brad Chen, 2011-08-15 19:52:58]

On 2011/08/12 21:38:45, Brad Chen wrote:
> Adding Roland as he knows the linker stuff really well.

Any feedback on this CL? I'm hoping to get it in in the next day or two.

[Roland McGrath, 2011-08-15 20:15:51]

I can't speak to the Chrome parts, but the linking rules and the basic
AS-reserving helper LGTM modulo tiny nits below.

http://codereview.chromium.org/7599011/diff/10003/chrome/nacl.gypi
File chrome/nacl.gypi (right):

http://codereview.chromium.org/7599011/diff/10003/chrome/nacl.gypi#newcode191
chrome/nacl.gypi:191: '-Wl,-Ttext-segment,10000,-z,max-page-size=0x1000',
Needs a comment that max-page-size=0x1000 is for the benefit of x86_64, where
that's not the default (in the BFD linker).

http://codereview.chromium.org/7599011/diff/10003/chrome/nacl/nacl_helper_boo...
File chrome/nacl/nacl_helper_bootstrap_linux.c (right):

http://codereview.chromium.org/7599011/diff/10003/chrome/nacl/nacl_helper_boo...
chrome/nacl/nacl_helper_bootstrap_linux.c:14: #define ONEGIG 0x40000000
I use (1 << 30).

http://codereview.chromium.org/7599011/diff/10003/chrome/nacl/nacl_helper_boo...
chrome/nacl/nacl_helper_bootstrap_linux.c:22: assert(0);
It's better to use abort(), in case -DNDEBUG is used (which renders assert a
no-op).

http://codereview.chromium.org/7599011/diff/10003/chrome/nacl/nacl_helper_lin...
File chrome/nacl/nacl_helper_linux.cc (right):

http://codereview.chromium.org/7599011/diff/10003/chrome/nacl/nacl_helper_lin...
chrome/nacl/nacl_helper_linux.cc:121: const char* nacl_helper_get_1G_address() {
Might as well just be const void* (both the variable and the function return
type).

http://codereview.chromium.org/7599011/diff/10003/chrome/nacl/nacl_helper_lin...
chrome/nacl/nacl_helper_linux.cc:188: // This main() is used for testing, and to
fool the linker into
main should go away since we're doing -shared linking now.

[Brad Chen, 2011-08-15 23:02:28]

http://codereview.chromium.org/7599011/diff/10003/chrome/nacl.gypi
File chrome/nacl.gypi (right):

http://codereview.chromium.org/7599011/diff/10003/chrome/nacl.gypi#newcode191
chrome/nacl.gypi:191: '-Wl,-Ttext-segment,10000,-z,max-page-size=0x1000',
On 2011/08/15 20:15:51, Roland McGrath wrote:
> Needs a comment that max-page-size=0x1000 is for the benefit of x86_64, where
> that's not the default (in the BFD linker).

Done.

http://codereview.chromium.org/7599011/diff/10003/chrome/nacl/nacl_helper_boo...
File chrome/nacl/nacl_helper_bootstrap_linux.c (right):

http://codereview.chromium.org/7599011/diff/10003/chrome/nacl/nacl_helper_boo...
chrome/nacl/nacl_helper_bootstrap_linux.c:14: #define ONEGIG 0x40000000
Seems at least as clear. Changed as per your suggestion.

On 2011/08/15 20:15:51, Roland McGrath wrote:
> I use (1 << 30).

http://codereview.chromium.org/7599011/diff/10003/chrome/nacl/nacl_helper_boo...
chrome/nacl/nacl_helper_bootstrap_linux.c:22: assert(0);
On 2011/08/15 20:15:51, Roland McGrath wrote:
> It's better to use abort(), in case -DNDEBUG is used (which renders assert a
> no-op).

Done.

http://codereview.chromium.org/7599011/diff/10003/chrome/nacl/nacl_helper_lin...
File chrome/nacl/nacl_helper_linux.cc (right):

http://codereview.chromium.org/7599011/diff/10003/chrome/nacl/nacl_helper_lin...
chrome/nacl/nacl_helper_linux.cc:121: const char* nacl_helper_get_1G_address() {
On 2011/08/15 20:15:51, Roland McGrath wrote:
> Might as well just be const void* (both the variable and the function return
> type).

Done.

http://codereview.chromium.org/7599011/diff/10003/chrome/nacl/nacl_helper_lin...
chrome/nacl/nacl_helper_linux.cc:188: // This main() is used for testing, and to
fool the linker into
On 2011/08/15 20:15:51, Roland McGrath wrote:
> main should go away since we're doing -shared linking now.

Done.

[agl, 2011-08-16 16:44:39]

http://codereview.chromium.org/7599011/diff/11010/chrome/common/chrome_paths.cc
File chrome/common/chrome_paths.cc (right):

http://codereview.chromium.org/7599011/diff/11010/chrome/common/chrome_paths....
chrome/common/chrome_paths.cc:55: #if !defined(OS_MACOSX) && defined(OS_POSIX)
ditto

http://codereview.chromium.org/7599011/diff/11010/chrome/common/chrome_paths.h
File chrome/common/chrome_paths.h (right):

http://codereview.chromium.org/7599011/diff/11010/chrome/common/chrome_paths....
chrome/common/chrome_paths.h:72: #if !defined(OS_MACOSX) && defined(OS_POSIX)
These clauses are typically written the other way around.

http://codereview.chromium.org/7599011/diff/11010/chrome/nacl/nacl_fork_deleg...
File chrome/nacl/nacl_fork_delegate_linux.cc (right):

http://codereview.chromium.org/7599011/diff/11010/chrome/nacl/nacl_fork_deleg...
chrome/nacl/nacl_fork_delegate_linux.cc:60: if (setenv("LD_BIND_NOW", "1", 1) !=
0) {
This is going to pollute the environment for all children of the main process,
no? |options| provides a member for setting the environment of just the child
process.

http://codereview.chromium.org/7599011/diff/11010/chrome/nacl/nacl_helper_boo...
File chrome/nacl/nacl_helper_bootstrap_linux.c (right):

http://codereview.chromium.org/7599011/diff/11010/chrome/nacl/nacl_helper_boo...
chrome/nacl/nacl_helper_bootstrap_linux.c:6: * of the address space, then
invokes nacl_helper_main. Note that,
|nacl_helper_main| doesn't match up with the function below: |nacl_helper_init|.

http://codereview.chromium.org/7599011/diff/11010/chrome/nacl/nacl_helper_boo...
chrome/nacl/nacl_helper_bootstrap_linux.c:15: char nacl_reserved_space[ONEGIG];
You pass in linker arguments to make the text segment start at 64KB, but this
will live in the BSS. I assume it does what you want anyway?

[Brad Chen, 2011-08-16 18:05:16]

PTAL

http://codereview.chromium.org/7599011/diff/11010/chrome/common/chrome_paths.cc
File chrome/common/chrome_paths.cc (right):

http://codereview.chromium.org/7599011/diff/11010/chrome/common/chrome_paths....
chrome/common/chrome_paths.cc:55: #if !defined(OS_MACOSX) && defined(OS_POSIX)
On 2011/08/16 16:44:39, agl wrote:
> ditto

Done.

http://codereview.chromium.org/7599011/diff/11010/chrome/common/chrome_paths.h
File chrome/common/chrome_paths.h (right):

http://codereview.chromium.org/7599011/diff/11010/chrome/common/chrome_paths....
chrome/common/chrome_paths.h:72: #if !defined(OS_MACOSX) && defined(OS_POSIX)
On 2011/08/16 16:44:39, agl wrote:
> These clauses are typically written the other way around.

Done.

http://codereview.chromium.org/7599011/diff/11010/chrome/nacl/nacl_fork_deleg...
File chrome/nacl/nacl_fork_delegate_linux.cc (right):

http://codereview.chromium.org/7599011/diff/11010/chrome/nacl/nacl_fork_deleg...
chrome/nacl/nacl_fork_delegate_linux.cc:60: if (setenv("LD_BIND_NOW", "1", 1) !=
0) {
On 2011/08/16 16:44:39, agl wrote:
> This is going to pollute the environment for all children of the main process,
> no? |options| provides a member for setting the environment of just the child
> process.

Done.

I agree this is much better done with options to LaunchProcess.

http://codereview.chromium.org/7599011/diff/11010/chrome/nacl/nacl_helper_boo...
File chrome/nacl/nacl_helper_bootstrap_linux.c (right):

http://codereview.chromium.org/7599011/diff/11010/chrome/nacl/nacl_helper_boo...
chrome/nacl/nacl_helper_bootstrap_linux.c:6: * of the address space, then
invokes nacl_helper_main. Note that,
On 2011/08/16 16:44:39, agl wrote:
> |nacl_helper_main| doesn't match up with the function below:
|nacl_helper_init|.

Done.

http://codereview.chromium.org/7599011/diff/11010/chrome/nacl/nacl_helper_boo...
chrome/nacl/nacl_helper_bootstrap_linux.c:15: char nacl_reserved_space[ONEGIG];
The kernel is quite inflexible about order of sections, so text comes first,
with RO-data and bss following immediately after modulo alignment. I have tested
this code with an appropriately-modified NaCl system and it works as intended.

On 2011/08/16 16:44:39, agl wrote:
> You pass in linker arguments to make the text segment start at 64KB, but this
> will live in the BSS. I assume it does what you want anyway?

[agl, 2011-08-16 18:30:10]

LGTM

[bradn, 2011-08-16 22:43:53]

http://codereview.chromium.org/7599011/diff/20002/chrome/nacl.gypi
File chrome/nacl.gypi (right):

http://codereview.chromium.org/7599011/diff/20002/chrome/nacl.gypi#newcode195
chrome/nacl.gypi:195: './out/Debug/nacl_helper.so',
Here you've assumed Debug, you probably want:
'out/<(CONFIGURATION_NAME)/nacl_helper.so',
Actually, more correctly you want:
'<(PRODUCT_DIR)/nacl_helper.so',