Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1382713002:
Creating a pexe content handler to translate and run pexes. (Closed)
|
Created:
5 years, 2 months ago by Sean Klein Modified:
5 years, 1 month ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, darin (slow to review), gregsimon, mojo-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://github.com/domokit/mojo.git@master Target Ref:
refs/heads/master Project:
mojo Visibility:
Public. |
DescriptionCreating a pexe content handler to translate and run pexes.
-Creates translator and linker applications, which launch the pexe
translator (pnacl-llc.nexe) and pexe loader (ld.nexe), passing a message
pipe handle.
-Updates IRT to include callbacks for pexe translator and pexe loader.
-Creates a content handler application, which contacts the translator
and loader services, communicating with them via message pipes. Uses a mojom
interface to pass a message pipe to each application.
-The translation process places a PEXE FILE into a temporary file, compiles
that pexe into a temporary OBJECT FILE, and finally links that object file
into a finalized temporary NEXE FILE. All file names are passed via message
pipes, and all files are unlinked after they are opened by the final service
which uses them.
-Adds some .pexe.mojo files to the nacl_nonsfi_apptests.
Test by running:
> ./mojo/tools/mojob.py build
> ./out/Debug/mojo_shell --enable-multiprocess ./out/Debug/shell_apptests.pexe.mojo
BUG=https://github.com/domokit/mojo/issues/396
R=mseaborn@chromium.org, phosek@chromium.org
Committed: https://chromium.googlesource.com/external/mojo/+/2ba2b8d5259f250e4f7933de76645cf5113db6c0
Patch Set 1 : Works with modified nexes doing IRT. Uses environmental variable hacks. #Patch Set 2 : Pexe --> Nexe Translation complete without callbacks/environmental variables #Patch Set 3 : Tests Added #
Total comments: 48
Patch Set 4 : #Patch Set 5 : Added Mojom interface to communicate with translation nexes #
Total comments: 44
Patch Set 6 : Response to Code reviews #
Total comments: 26
Patch Set 7 : Using PathService instead of URL hacking #
Total comments: 8
Patch Set 8 : #
Total comments: 4
Patch Set 9 : #Patch Set 10 : #
Total comments: 7
Messages
Total messages: 45 (24 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== Creating a pexe content handler to translate and run pexes. WARNING: This CL is currently laughably broken -- BUT -- running "./mojo/tools/mojob.py build; ./out/Debug/mojo_shell --enable-multiprocess ./out/Debug/shell_apptests.pexe.mojo" will get somewhere. It'll get to an abort, because of the line "Not an ELF file: no ELF header", because the content handler is currently more or less a blatant copy of the nonsfi-nexe version (it should not be), but right before we call "NaClLoadElfFile", we are inside a content handler with access to an unprefixed pexe, prime for translation. BUG=https://github.com/domokit/mojo/issues/396 ========== to ========== Creating a pexe content handler to translate and run pexes. -Creates translator and linker applications, which launch the pexe translator (pnacl-llc.nexe) and pexe loader (ld.nexe), passing a message pipe handle. -Updates IRT to include callbacks for pexe translator and pexe loader. -Creates a content handler application, which contacts the translator and loader services, communicating with them via message pipes. Uses a mojom interface to pass a message pipe to each application. -The translation process places a PEXE FILE into a temporary file, compiles that pexe into a temporary OBJECT FILE, and finally links that object file into a finalized temporary NEXE FILE. All file names are passed via message pipes, and all files are unlinked after they are opened by the final service which uses them. Test by running: > ./mojo/tools/mojob.py build > ./out/Debug/mojo_shell --enable-multiprocess ./out/Debug/shell_apptests.pexe.mojo BUG=https://github.com/domokit/mojo/issues/396 ==========
Description was changed from ========== Creating a pexe content handler to translate and run pexes. -Creates translator and linker applications, which launch the pexe translator (pnacl-llc.nexe) and pexe loader (ld.nexe), passing a message pipe handle. -Updates IRT to include callbacks for pexe translator and pexe loader. -Creates a content handler application, which contacts the translator and loader services, communicating with them via message pipes. Uses a mojom interface to pass a message pipe to each application. -The translation process places a PEXE FILE into a temporary file, compiles that pexe into a temporary OBJECT FILE, and finally links that object file into a finalized temporary NEXE FILE. All file names are passed via message pipes, and all files are unlinked after they are opened by the final service which uses them. Test by running: > ./mojo/tools/mojob.py build > ./out/Debug/mojo_shell --enable-multiprocess ./out/Debug/shell_apptests.pexe.mojo BUG=https://github.com/domokit/mojo/issues/396 ========== to ========== Creating a pexe content handler to translate and run pexes. -Creates translator and linker applications, which launch the pexe translator (pnacl-llc.nexe) and pexe loader (ld.nexe), passing a message pipe handle. -Updates IRT to include callbacks for pexe translator and pexe loader. -Creates a content handler application, which contacts the translator and loader services, communicating with them via message pipes. Uses a mojom interface to pass a message pipe to each application. -The translation process places a PEXE FILE into a temporary file, compiles that pexe into a temporary OBJECT FILE, and finally links that object file into a finalized temporary NEXE FILE. All file names are passed via message pipes, and all files are unlinked after they are opened by the final service which uses them. Test by running: > ./mojo/tools/mojob.py build > ./out/Debug/mojo_shell --enable-multiprocess ./out/Debug/shell_apptests.pexe.mojo Issues/Questions to resolve before submitting: -Tests? -In "content_handler_main_nonsfi_pexe.cc", I'm not calling "loop.Run()". Is that an issue? I feel like I do not fully understand the base::MessageLoop. -I create a new "ScopedMessagePipeHandle" pair for communication between the content handler and the linker/compiler, but once the handle reaches the nexe, it is no longer scoped. Should I be closing these handles? -I'm repeating the pattern of "Read + Wait on a handle in a do while loop" in a few places. Is there a built-in function for this? BUG=https://github.com/domokit/mojo/issues/396 ==========
Patchset #3 (id:120001) has been deleted
Patchset #1 (id:80001) has been deleted
Description was changed from ========== Creating a pexe content handler to translate and run pexes. -Creates translator and linker applications, which launch the pexe translator (pnacl-llc.nexe) and pexe loader (ld.nexe), passing a message pipe handle. -Updates IRT to include callbacks for pexe translator and pexe loader. -Creates a content handler application, which contacts the translator and loader services, communicating with them via message pipes. Uses a mojom interface to pass a message pipe to each application. -The translation process places a PEXE FILE into a temporary file, compiles that pexe into a temporary OBJECT FILE, and finally links that object file into a finalized temporary NEXE FILE. All file names are passed via message pipes, and all files are unlinked after they are opened by the final service which uses them. Test by running: > ./mojo/tools/mojob.py build > ./out/Debug/mojo_shell --enable-multiprocess ./out/Debug/shell_apptests.pexe.mojo Issues/Questions to resolve before submitting: -Tests? -In "content_handler_main_nonsfi_pexe.cc", I'm not calling "loop.Run()". Is that an issue? I feel like I do not fully understand the base::MessageLoop. -I create a new "ScopedMessagePipeHandle" pair for communication between the content handler and the linker/compiler, but once the handle reaches the nexe, it is no longer scoped. Should I be closing these handles? -I'm repeating the pattern of "Read + Wait on a handle in a do while loop" in a few places. Is there a built-in function for this? BUG=https://github.com/domokit/mojo/issues/396 ========== to ========== Creating a pexe content handler to translate and run pexes. -Creates translator and linker applications, which launch the pexe translator (pnacl-llc.nexe) and pexe loader (ld.nexe), passing a message pipe handle. -Updates IRT to include callbacks for pexe translator and pexe loader. -Creates a content handler application, which contacts the translator and loader services, communicating with them via message pipes. Uses a mojom interface to pass a message pipe to each application. -The translation process places a PEXE FILE into a temporary file, compiles that pexe into a temporary OBJECT FILE, and finally links that object file into a finalized temporary NEXE FILE. All file names are passed via message pipes, and all files are unlinked after they are opened by the final service which uses them. -Copies most SFI NaCl tests into the "pexe translate + run as Non-SFI" version. Test by running: > ./mojo/tools/mojob.py build > ./out/Debug/mojo_shell --enable-multiprocess ./out/Debug/shell_apptests.pexe.mojo Issues/Questions to resolve before submitting: -In "content_handler_main_nonsfi_pexe.cc", I'm not calling "loop.Run()". Is that an issue? I feel like I do not fully understand the base::MessageLoop. -I create a new "ScopedMessagePipeHandle" pair for communication between the content handler and the linker/compiler, but once the handle reaches the nexe, it is no longer scoped. Should I be closing these handles? -I'm repeating the pattern of "Read + Wait on a handle in a do while loop" in a few places. Is there a built-in function for this? BUG=https://github.com/domokit/mojo/issues/396 ==========
Description was changed from ========== Creating a pexe content handler to translate and run pexes. -Creates translator and linker applications, which launch the pexe translator (pnacl-llc.nexe) and pexe loader (ld.nexe), passing a message pipe handle. -Updates IRT to include callbacks for pexe translator and pexe loader. -Creates a content handler application, which contacts the translator and loader services, communicating with them via message pipes. Uses a mojom interface to pass a message pipe to each application. -The translation process places a PEXE FILE into a temporary file, compiles that pexe into a temporary OBJECT FILE, and finally links that object file into a finalized temporary NEXE FILE. All file names are passed via message pipes, and all files are unlinked after they are opened by the final service which uses them. -Copies most SFI NaCl tests into the "pexe translate + run as Non-SFI" version. Test by running: > ./mojo/tools/mojob.py build > ./out/Debug/mojo_shell --enable-multiprocess ./out/Debug/shell_apptests.pexe.mojo Issues/Questions to resolve before submitting: -In "content_handler_main_nonsfi_pexe.cc", I'm not calling "loop.Run()". Is that an issue? I feel like I do not fully understand the base::MessageLoop. -I create a new "ScopedMessagePipeHandle" pair for communication between the content handler and the linker/compiler, but once the handle reaches the nexe, it is no longer scoped. Should I be closing these handles? -I'm repeating the pattern of "Read + Wait on a handle in a do while loop" in a few places. Is there a built-in function for this? BUG=https://github.com/domokit/mojo/issues/396 ========== to ========== Creating a pexe content handler to translate and run pexes. -Creates translator and linker applications, which launch the pexe translator (pnacl-llc.nexe) and pexe loader (ld.nexe), passing a message pipe handle. -Updates IRT to include callbacks for pexe translator and pexe loader. -Creates a content handler application, which contacts the translator and loader services, communicating with them via message pipes. Uses a mojom interface to pass a message pipe to each application. -The translation process places a PEXE FILE into a temporary file, compiles that pexe into a temporary OBJECT FILE, and finally links that object file into a finalized temporary NEXE FILE. All file names are passed via message pipes, and all files are unlinked after they are opened by the final service which uses them. -Adds some .pexe.mojo files to the nacl_nonsfi_apptests. Test by running: > ./mojo/tools/mojob.py build > ./out/Debug/mojo_shell --enable-multiprocess ./out/Debug/shell_apptests.pexe.mojo Issues/Questions to resolve before submitting: -In "content_handler_main_nonsfi_pexe.cc", I'm not calling "loop.Run()". Is that an issue? I feel like I do not fully understand the base::MessageLoop. -I create a new "ScopedMessagePipeHandle" pair for communication between the content handler and the linker/compiler, but once the handle reaches the nexe, it is no longer scoped. Should I be closing these handles? -I'm repeating the pattern of "Read + Wait on a handle in a do while loop" in a few places. Is there a built-in function for this? BUG=https://github.com/domokit/mojo/issues/396 ==========
Description was changed from ========== Creating a pexe content handler to translate and run pexes. -Creates translator and linker applications, which launch the pexe translator (pnacl-llc.nexe) and pexe loader (ld.nexe), passing a message pipe handle. -Updates IRT to include callbacks for pexe translator and pexe loader. -Creates a content handler application, which contacts the translator and loader services, communicating with them via message pipes. Uses a mojom interface to pass a message pipe to each application. -The translation process places a PEXE FILE into a temporary file, compiles that pexe into a temporary OBJECT FILE, and finally links that object file into a finalized temporary NEXE FILE. All file names are passed via message pipes, and all files are unlinked after they are opened by the final service which uses them. -Adds some .pexe.mojo files to the nacl_nonsfi_apptests. Test by running: > ./mojo/tools/mojob.py build > ./out/Debug/mojo_shell --enable-multiprocess ./out/Debug/shell_apptests.pexe.mojo Issues/Questions to resolve before submitting: -In "content_handler_main_nonsfi_pexe.cc", I'm not calling "loop.Run()". Is that an issue? I feel like I do not fully understand the base::MessageLoop. -I create a new "ScopedMessagePipeHandle" pair for communication between the content handler and the linker/compiler, but once the handle reaches the nexe, it is no longer scoped. Should I be closing these handles? -I'm repeating the pattern of "Read + Wait on a handle in a do while loop" in a few places. Is there a built-in function for this? BUG=https://github.com/domokit/mojo/issues/396 ========== to ========== Creating a pexe content handler to translate and run pexes. -Creates translator and linker applications, which launch the pexe translator (pnacl-llc.nexe) and pexe loader (ld.nexe), passing a message pipe handle. -Updates IRT to include callbacks for pexe translator and pexe loader. -Creates a content handler application, which contacts the translator and loader services, communicating with them via message pipes. Uses a mojom interface to pass a message pipe to each application. -The translation process places a PEXE FILE into a temporary file, compiles that pexe into a temporary OBJECT FILE, and finally links that object file into a finalized temporary NEXE FILE. All file names are passed via message pipes, and all files are unlinked after they are opened by the final service which uses them. -Adds some .pexe.mojo files to the nacl_nonsfi_apptests. Test by running: > ./mojo/tools/mojob.py build > ./out/Debug/mojo_shell --enable-multiprocess ./out/Debug/shell_apptests.pexe.mojo Issues/Questions to resolve before submitting: -In "content_handler_main_nonsfi_pexe.cc", I'm not calling "loop.Run()". Is that an issue? I feel like I do not fully understand the base::MessageLoop. -I create a new "ScopedMessagePipeHandle" pair for communication between the content handler and the linker/compiler, but once the handle reaches the nexe, it is no longer scoped. Should I be closing these handles? -I'm repeating the pattern of "Read + Wait on a handle in a do while loop" in a few places. Is there a built-in function for this? -I'm currently using an "ld.nexe" and "pnacl-llc.nexe" from a local directory on my machine called "sean_out", since these updated nexes are not committed/rolled into deps yet. These paths will be resolved before this change is landed! BUG=https://github.com/domokit/mojo/issues/396 ==========
Description was changed from ========== Creating a pexe content handler to translate and run pexes. -Creates translator and linker applications, which launch the pexe translator (pnacl-llc.nexe) and pexe loader (ld.nexe), passing a message pipe handle. -Updates IRT to include callbacks for pexe translator and pexe loader. -Creates a content handler application, which contacts the translator and loader services, communicating with them via message pipes. Uses a mojom interface to pass a message pipe to each application. -The translation process places a PEXE FILE into a temporary file, compiles that pexe into a temporary OBJECT FILE, and finally links that object file into a finalized temporary NEXE FILE. All file names are passed via message pipes, and all files are unlinked after they are opened by the final service which uses them. -Adds some .pexe.mojo files to the nacl_nonsfi_apptests. Test by running: > ./mojo/tools/mojob.py build > ./out/Debug/mojo_shell --enable-multiprocess ./out/Debug/shell_apptests.pexe.mojo Issues/Questions to resolve before submitting: -In "content_handler_main_nonsfi_pexe.cc", I'm not calling "loop.Run()". Is that an issue? I feel like I do not fully understand the base::MessageLoop. -I create a new "ScopedMessagePipeHandle" pair for communication between the content handler and the linker/compiler, but once the handle reaches the nexe, it is no longer scoped. Should I be closing these handles? -I'm repeating the pattern of "Read + Wait on a handle in a do while loop" in a few places. Is there a built-in function for this? -I'm currently using an "ld.nexe" and "pnacl-llc.nexe" from a local directory on my machine called "sean_out", since these updated nexes are not committed/rolled into deps yet. These paths will be resolved before this change is landed! BUG=https://github.com/domokit/mojo/issues/396 ========== to ========== Creating a pexe content handler to translate and run pexes. -Creates translator and linker applications, which launch the pexe translator (pnacl-llc.nexe) and pexe loader (ld.nexe), passing a message pipe handle. -Updates IRT to include callbacks for pexe translator and pexe loader. -Creates a content handler application, which contacts the translator and loader services, communicating with them via message pipes. Uses a mojom interface to pass a message pipe to each application. -The translation process places a PEXE FILE into a temporary file, compiles that pexe into a temporary OBJECT FILE, and finally links that object file into a finalized temporary NEXE FILE. All file names are passed via message pipes, and all files are unlinked after they are opened by the final service which uses them. -Adds some .pexe.mojo files to the nacl_nonsfi_apptests. Test by running: > ./mojo/tools/mojob.py build > ./out/Debug/mojo_shell --enable-multiprocess ./out/Debug/shell_apptests.pexe.mojo Issues/Questions to resolve before submitting: -In "content_handler_main_nonsfi_pexe.cc", I'm not calling "loop.Run()". Is that an issue? I feel like I do not fully understand the base::MessageLoop. -I create a new "ScopedMessagePipeHandle" pair for communication between the content handler and the linker/compiler, but once the handle reaches the nexe, it is no longer scoped. Should I be closing these handles? -I'm repeating the pattern of "Read + Wait on a handle in a do while loop" in a few places. Is there a built-in function for this? -I'm currently using an "ld.nexe" and "pnacl-llc.nexe" from a local directory on my machine called "sean_out", since these updated nexes are not committed/rolled into deps yet. These paths will be resolved before this change is landed! BUG=https://github.com/domokit/mojo/issues/396 ==========
smklein@chromium.org changed reviewers: + mseaborn@chromium.org, phosek@chromium.org
Tests don't work on bots yet because "ld.nexe" and "pnacl-llc.nexe" are not available. I'll send the CL to either james or vtl for readability after getting the initial issued worked out.
Description was changed from ========== Creating a pexe content handler to translate and run pexes. -Creates translator and linker applications, which launch the pexe translator (pnacl-llc.nexe) and pexe loader (ld.nexe), passing a message pipe handle. -Updates IRT to include callbacks for pexe translator and pexe loader. -Creates a content handler application, which contacts the translator and loader services, communicating with them via message pipes. Uses a mojom interface to pass a message pipe to each application. -The translation process places a PEXE FILE into a temporary file, compiles that pexe into a temporary OBJECT FILE, and finally links that object file into a finalized temporary NEXE FILE. All file names are passed via message pipes, and all files are unlinked after they are opened by the final service which uses them. -Adds some .pexe.mojo files to the nacl_nonsfi_apptests. Test by running: > ./mojo/tools/mojob.py build > ./out/Debug/mojo_shell --enable-multiprocess ./out/Debug/shell_apptests.pexe.mojo Issues/Questions to resolve before submitting: -In "content_handler_main_nonsfi_pexe.cc", I'm not calling "loop.Run()". Is that an issue? I feel like I do not fully understand the base::MessageLoop. -I create a new "ScopedMessagePipeHandle" pair for communication between the content handler and the linker/compiler, but once the handle reaches the nexe, it is no longer scoped. Should I be closing these handles? -I'm repeating the pattern of "Read + Wait on a handle in a do while loop" in a few places. Is there a built-in function for this? -I'm currently using an "ld.nexe" and "pnacl-llc.nexe" from a local directory on my machine called "sean_out", since these updated nexes are not committed/rolled into deps yet. These paths will be resolved before this change is landed! BUG=https://github.com/domokit/mojo/issues/396 ========== to ========== Creating a pexe content handler to translate and run pexes. -Creates translator and linker applications, which launch the pexe translator (pnacl-llc.nexe) and pexe loader (ld.nexe), passing a message pipe handle. -Updates IRT to include callbacks for pexe translator and pexe loader. -Creates a content handler application, which contacts the translator and loader services, communicating with them via message pipes. Uses a mojom interface to pass a message pipe to each application. -The translation process places a PEXE FILE into a temporary file, compiles that pexe into a temporary OBJECT FILE, and finally links that object file into a finalized temporary NEXE FILE. All file names are passed via message pipes, and all files are unlinked after they are opened by the final service which uses them. -Adds some .pexe.mojo files to the nacl_nonsfi_apptests. Test by running: > ./mojo/tools/mojob.py build > ./out/Debug/mojo_shell --enable-multiprocess ./out/Debug/shell_apptests.pexe.mojo Issues/Questions to resolve before submitting: -In "content_handler_main_nonsfi_pexe.cc", I'm not calling "loop.Run()". Is that an issue? I feel like I do not fully understand the base::MessageLoop. -I create a new "ScopedMessagePipeHandle" pair for communication between the content handler and the linker/compiler, but once the handle reaches the nexe, it is no longer scoped. Should I be closing these handles? -I'm repeating the pattern of "Read + Wait on a handle in a do while loop" in a few places. Is there a built-in function for this? -I'm unsure how this translation process will react to translating multiple pexes at the same time (this behavior is thus far untested -- what's the best way to go about doing these tests?). -I'm currently using an "ld.nexe" and "pnacl-llc.nexe" from a local directory on my machine called "sean_out", since these updated nexes are not committed/rolled into deps yet. These paths will be resolved before this change is landed! BUG=https://github.com/domokit/mojo/issues/396 ==========
At phosek's suggestion, I will move the changes to "data_pipe_utils" into a separate CL.
https://codereview.chromium.org/1382713002/diff/160001/DEPS File DEPS (right): https://codereview.chromium.org/1382713002/diff/160001/DEPS#newcode33 DEPS:33: 'nacl_revision': '9c69f1fcb35b34f136da7ea7c91ceb743b9d2aaa', It's better to do rolls as a separate change, if possible. https://codereview.chromium.org/1382713002/diff/160001/mojo/nacl/nonsfi/irt_m... File mojo/nacl/nonsfi/irt_mojo_nonsfi.cc (right): https://codereview.chromium.org/1382713002/diff/160001/mojo/nacl/nonsfi/irt_m... mojo/nacl/nonsfi/irt_mojo_nonsfi.cc:38: static const struct nacl_irt_interface kIrtInterfaces[] = { Nit: 'static' not needed: this is in an anon namespace. https://codereview.chromium.org/1382713002/diff/160001/mojo/nacl/nonsfi/irt_m... mojo/nacl/nonsfi/irt_mojo_nonsfi.cc:39: /* Interface to call Mojo functions */ Nit: Use C++-style comments https://codereview.chromium.org/1382713002/diff/160001/mojo/nacl/nonsfi/irt_m... File mojo/nacl/nonsfi/irt_mojo_nonsfi.h (right): https://codereview.chromium.org/1382713002/diff/160001/mojo/nacl/nonsfi/irt_m... mojo/nacl/nonsfi/irt_mojo_nonsfi.h:11: nacl_irt_private_pnacl_translator_compile; How about putting these in the namespace? https://codereview.chromium.org/1382713002/diff/160001/mojo/nacl/nonsfi/irt_m... mojo/nacl/nonsfi/irt_mojo_nonsfi.h:23: MojoResult _MojoGetInitialHandle(MojoHandle* handle); If you're moving this to a header, let's remove the "_" prefix, which doesn't conform to the style guide. https://codereview.chromium.org/1382713002/diff/160001/mojo/nacl/nonsfi/irt_p... File mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc (right): https://codereview.chromium.org/1382713002/diff/160001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:24: result = MojoReadMessage(handle, I don't think you should be using raw Mojo IPC. Can you use Mojo's message marshalling layer? Also, there should be a big TODO somewhere to pass FDs across IPC instead of passing filenames. :-) https://codereview.chromium.org/1382713002/diff/160001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:43: pexe_file_name[path_size] = '\0'; Couldn't path_size be sizeof(pexe_file_name)? In that case, this would overrun the buffer. https://codereview.chromium.org/1382713002/diff/160001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:51: int obj_file_fds[] = { open(obj_file_name.value().c_str(), O_RDWR, O_TRUNC) }; This looks a bit quirky. :-) You could just do: int obj_file_fd = open(...); and use &obj_file_fd as the array pointer below. However, why are you using just one object file? This will basically turn off multi-threaded translation for pnacl-llc, which needs one object file per thread. You could add a TODO to use multiple object files, but maybe it wouldn't be hard to implement that for this first version? https://codereview.chromium.org/1382713002/diff/160001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:56: // TODO(smklein): Is there a less arbitrary number to choose? What is Chromium using for the thread count? This shouldn't be more than the number of CPUs. https://codereview.chromium.org/1382713002/diff/160001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:68: char buf[0x100000]; That's a lot to stack-allocate. Can you heap-allocate it instead? https://codereview.chromium.org/1382713002/diff/160001/mojo/nacl/nonsfi/irt_r... File mojo/nacl/nonsfi/irt_resource_open.cc (right): https://codereview.chromium.org/1382713002/diff/160001/mojo/nacl/nonsfi/irt_r... mojo/nacl/nonsfi/irt_resource_open.cc:11: int nacl_irt_open_resource(const char *pathname, int *newfd) { This can go in an anon namespace and follow Chromium naming style. Same in irt_pnacl_translator_link/compile.cc too. https://codereview.chromium.org/1382713002/diff/160001/mojo/nacl/nonsfi/irt_r... mojo/nacl/nonsfi/irt_resource_open.cc:13: char nonsfi_toolchain[] = "native_client/toolchain/linux_x86" This can use 'const' https://codereview.chromium.org/1382713002/diff/160001/mojo/nacl/nonsfi/irt_r... mojo/nacl/nonsfi/irt_resource_open.cc:17: strcat(path, pathname); Can you use C++ strings instead? This overruns the buffer if the caller passes a long string, which the caller is allowed to do. https://codereview.chromium.org/1382713002/diff/160001/mojo/nacl/nonsfi/irt_r... mojo/nacl/nonsfi/irt_resource_open.cc:22: return -rv; Should return errno instead. https://codereview.chromium.org/1382713002/diff/160001/services/nacl/pexe_lin... File services/nacl/pexe_linker_app.cc (right): https://codereview.chromium.org/1382713002/diff/160001/services/nacl/pexe_lin... services/nacl/pexe_linker_app.cc:25: int nexe_fd = open("sean_out/ld.nexe", O_RDONLY); Needs fixing. :-)
Some more comments... https://codereview.chromium.org/1382713002/diff/160001/mojo/nacl/nonsfi/irt_m... File mojo/nacl/nonsfi/irt_mojo_nonsfi.cc (right): https://codereview.chromium.org/1382713002/diff/160001/mojo/nacl/nonsfi/irt_m... mojo/nacl/nonsfi/irt_mojo_nonsfi.cc:48: nullptr }, Can you conditionalise the 3 new interfaces so that they are only made visible to the PNaCl translator nexes, please? (This 4th field in 'struct nacl_irt_interface' is for conditionalising interfaces.) https://codereview.chromium.org/1382713002/diff/160001/services/nacl/BUILD.gn File services/nacl/BUILD.gn (right): https://codereview.chromium.org/1382713002/diff/160001/services/nacl/BUILD.gn... services/nacl/BUILD.gn:133: mojom("pexe_bindings") { How about "pexe" -> "pnacl_translator"? https://codereview.chromium.org/1382713002/diff/160001/services/nacl/content_... File services/nacl/content_handler_main_nonsfi_pexe.cc (right): https://codereview.chromium.org/1382713002/diff/160001/services/nacl/content_... services/nacl/content_handler_main_nonsfi_pexe.cc:61: app->ConnectToService("mojo:pexe_translator_app", &translator_); I'm curious whether this could just be done in RunApplication()? https://codereview.chromium.org/1382713002/diff/160001/services/nacl/pexe_tra... File services/nacl/pexe_translator_app.cc (right): https://codereview.chromium.org/1382713002/diff/160001/services/nacl/pexe_tra... services/nacl/pexe_translator_app.cc:4: Nit: Can you name this using similar naming to the IRT interface, perhaps? i.e. "pnacl_translator_compile/link" or (shorter) "pnacl_compile/link"? https://codereview.chromium.org/1382713002/diff/160001/services/nacl/pexe_tra... services/nacl/pexe_translator_app.cc:22: void PexeTranslate(ScopedMessagePipeHandle handle) override { Nit: fix indentation -- this is indented by 3 https://codereview.chromium.org/1382713002/diff/160001/services/nacl/pexe_tra... services/nacl/pexe_translator_app.cc:27: LOG(FATAL) << "Could not open compiler nexe"; It would be nice to report the filename in these error messages. https://codereview.chromium.org/1382713002/diff/160001/services/nacl/pexe_tra... services/nacl/pexe_translator_app.cc:28: ::nacl::MojoLaunchNexeNonsfi(nexe_fd, handle.Pass().get().value()); I think you can do handle.release().value() instead of handle.Pass().get().value(). https://codereview.chromium.org/1382713002/diff/160001/services/nacl/pexe_tra... services/nacl/pexe_translator_app.cc:32: class StrongBindingPexeTranslatorImpl : public PexeTranslatorImpl { How come this is a separate class from PexeTranslatorImpl? Can't the two classes be combined? https://codereview.chromium.org/1382713002/diff/160001/services/nacl/pexe_tra... services/nacl/pexe_translator_app.cc:42: class MultiPexeTranslator : public mojo::ApplicationDelegate, What does the "Multi" part mean? If we try to translate multiple pexes, will the system create separate translator processes for that, or try to reuse one process?
Description was changed from ========== Creating a pexe content handler to translate and run pexes. -Creates translator and linker applications, which launch the pexe translator (pnacl-llc.nexe) and pexe loader (ld.nexe), passing a message pipe handle. -Updates IRT to include callbacks for pexe translator and pexe loader. -Creates a content handler application, which contacts the translator and loader services, communicating with them via message pipes. Uses a mojom interface to pass a message pipe to each application. -The translation process places a PEXE FILE into a temporary file, compiles that pexe into a temporary OBJECT FILE, and finally links that object file into a finalized temporary NEXE FILE. All file names are passed via message pipes, and all files are unlinked after they are opened by the final service which uses them. -Adds some .pexe.mojo files to the nacl_nonsfi_apptests. Test by running: > ./mojo/tools/mojob.py build > ./out/Debug/mojo_shell --enable-multiprocess ./out/Debug/shell_apptests.pexe.mojo Issues/Questions to resolve before submitting: -In "content_handler_main_nonsfi_pexe.cc", I'm not calling "loop.Run()". Is that an issue? I feel like I do not fully understand the base::MessageLoop. -I create a new "ScopedMessagePipeHandle" pair for communication between the content handler and the linker/compiler, but once the handle reaches the nexe, it is no longer scoped. Should I be closing these handles? -I'm repeating the pattern of "Read + Wait on a handle in a do while loop" in a few places. Is there a built-in function for this? -I'm unsure how this translation process will react to translating multiple pexes at the same time (this behavior is thus far untested -- what's the best way to go about doing these tests?). -I'm currently using an "ld.nexe" and "pnacl-llc.nexe" from a local directory on my machine called "sean_out", since these updated nexes are not committed/rolled into deps yet. These paths will be resolved before this change is landed! BUG=https://github.com/domokit/mojo/issues/396 ========== to ========== Creating a pexe content handler to translate and run pexes. -Creates translator and linker applications, which launch the pexe translator (pnacl-llc.nexe) and pexe loader (ld.nexe), passing a message pipe handle. -Updates IRT to include callbacks for pexe translator and pexe loader. -Creates a content handler application, which contacts the translator and loader services, communicating with them via message pipes. Uses a mojom interface to pass a message pipe to each application. -The translation process places a PEXE FILE into a temporary file, compiles that pexe into a temporary OBJECT FILE, and finally links that object file into a finalized temporary NEXE FILE. All file names are passed via message pipes, and all files are unlinked after they are opened by the final service which uses them. -Adds some .pexe.mojo files to the nacl_nonsfi_apptests. Test by running: > ./mojo/tools/mojob.py build > ./out/Debug/mojo_shell --enable-multiprocess ./out/Debug/shell_apptests.pexe.mojo Issues/Questions to resolve before submitting: -In "content_handler_main_nonsfi_pexe.cc", I'm not calling "loop.Run()". Is that an issue? I feel like I do not fully understand the base::MessageLoop. -I create a new "ScopedMessagePipeHandle" pair for communication between the content handler and the linker/compiler, but once the handle reaches the nexe, it is no longer scoped. Should I be closing these handles? -I'm repeating the pattern of "Read + Wait on a handle in a do while loop" in a few places. Is there a built-in function for this? -I'm unsure how this translation process will react to translating multiple pexes at the same time (this behavior is thus far untested -- what's the best way to go about doing these tests?). BUG=https://github.com/domokit/mojo/issues/396 ==========
https://chromiumcodereview.appspot.com/1382713002/diff/160001/DEPS File DEPS (right): https://chromiumcodereview.appspot.com/1382713002/diff/160001/DEPS#newcode33 DEPS:33: 'nacl_revision': '9c69f1fcb35b34f136da7ea7c91ceb743b9d2aaa', On 2015/10/20 21:32:40, Mark Seaborn wrote: > It's better to do rolls as a separate change, if possible. Done (Petr already rolled them for me) https://chromiumcodereview.appspot.com/1382713002/diff/160001/mojo/nacl/nonsf... File mojo/nacl/nonsfi/irt_mojo_nonsfi.cc (right): https://chromiumcodereview.appspot.com/1382713002/diff/160001/mojo/nacl/nonsf... mojo/nacl/nonsfi/irt_mojo_nonsfi.cc:38: static const struct nacl_irt_interface kIrtInterfaces[] = { On 2015/10/20 21:32:40, Mark Seaborn wrote: > Nit: 'static' not needed: this is in an anon namespace. Done. https://chromiumcodereview.appspot.com/1382713002/diff/160001/mojo/nacl/nonsf... mojo/nacl/nonsfi/irt_mojo_nonsfi.cc:39: /* Interface to call Mojo functions */ On 2015/10/20 21:32:40, Mark Seaborn wrote: > Nit: Use C++-style comments Done. https://chromiumcodereview.appspot.com/1382713002/diff/160001/mojo/nacl/nonsf... mojo/nacl/nonsfi/irt_mojo_nonsfi.cc:48: nullptr }, On 2015/10/20 22:28:58, Mark Seaborn wrote: > Can you conditionalise the 3 new interfaces so that they are only made visible > to the PNaCl translator nexes, please? (This 4th field in 'struct > nacl_irt_interface' is for conditionalising interfaces.) Done. https://chromiumcodereview.appspot.com/1382713002/diff/160001/mojo/nacl/nonsf... File mojo/nacl/nonsfi/irt_mojo_nonsfi.h (right): https://chromiumcodereview.appspot.com/1382713002/diff/160001/mojo/nacl/nonsf... mojo/nacl/nonsfi/irt_mojo_nonsfi.h:11: nacl_irt_private_pnacl_translator_compile; On 2015/10/20 21:32:40, Mark Seaborn wrote: > How about putting these in the namespace? Done. https://chromiumcodereview.appspot.com/1382713002/diff/160001/mojo/nacl/nonsf... mojo/nacl/nonsfi/irt_mojo_nonsfi.h:23: MojoResult _MojoGetInitialHandle(MojoHandle* handle); On 2015/10/20 21:32:40, Mark Seaborn wrote: > If you're moving this to a header, let's remove the "_" prefix, which doesn't > conform to the style guide. Done. https://chromiumcodereview.appspot.com/1382713002/diff/160001/mojo/nacl/nonsf... File mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc (right): https://chromiumcodereview.appspot.com/1382713002/diff/160001/mojo/nacl/nonsf... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:24: result = MojoReadMessage(handle, On 2015/10/20 21:32:40, Mark Seaborn wrote: > I don't think you should be using raw Mojo IPC. Can you use Mojo's message > marshalling layer? > > Also, there should be a big TODO somewhere to pass FDs across IPC instead of > passing filenames. :-) I think this is somewhere that vtl should get involved in the discussion. 1) I'm not entirely sure how to use the mojom layer for sending marshalled strings when my only access to the world is through a raw "MojoHandle". 2) We discussed the concept of sending FDs through IPC -- It seems something that Mojo devs are strongly opposed to. https://chromiumcodereview.appspot.com/1382713002/diff/160001/mojo/nacl/nonsf... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:43: pexe_file_name[path_size] = '\0'; On 2015/10/20 21:32:40, Mark Seaborn wrote: > Couldn't path_size be sizeof(pexe_file_name)? In that case, this would overrun > the buffer. Using "PATH_MAX + 1" to allow room for null terminator https://chromiumcodereview.appspot.com/1382713002/diff/160001/mojo/nacl/nonsf... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:51: int obj_file_fds[] = { open(obj_file_name.value().c_str(), O_RDWR, O_TRUNC) }; On 2015/10/20 21:32:40, Mark Seaborn wrote: > This looks a bit quirky. :-) > > You could just do: > int obj_file_fd = open(...); > > and use &obj_file_fd as the array pointer below. > > However, why are you using just one object file? This will basically turn off > multi-threaded translation for pnacl-llc, which needs one object file per > thread. > > You could add a TODO to use multiple object files, but maybe it wouldn't be hard > to implement that for this first version? The "quirky" version was there so the type of obj_file_fds would be the same once multiple object files were added. Updated to the single "obj_file_fd" version. TODO included for multiple object files -- I will definitely get to that, since we care about compilation speed, but it will involve changes across the mojom interfaces, so I am going to defer it to a later change (as it is an optimization). https://chromiumcodereview.appspot.com/1382713002/diff/160001/mojo/nacl/nonsf... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:56: // TODO(smklein): Is there a less arbitrary number to choose? On 2015/10/20 21:32:40, Mark Seaborn wrote: > What is Chromium using for the thread count? > > This shouldn't be more than the number of CPUs. It's passed in as an argument, so I'm not entirely sure: https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/nacl_irt/irt... But I know it is in the range of 1 to 16 threads. https://chromiumcodereview.appspot.com/1382713002/diff/160001/mojo/nacl/nonsf... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:68: char buf[0x100000]; On 2015/10/20 21:32:40, Mark Seaborn wrote: > That's a lot to stack-allocate. Can you heap-allocate it instead? Done with new / delete[]. https://chromiumcodereview.appspot.com/1382713002/diff/160001/mojo/nacl/nonsf... File mojo/nacl/nonsfi/irt_resource_open.cc (right): https://chromiumcodereview.appspot.com/1382713002/diff/160001/mojo/nacl/nonsf... mojo/nacl/nonsfi/irt_resource_open.cc:11: int nacl_irt_open_resource(const char *pathname, int *newfd) { On 2015/10/20 21:32:40, Mark Seaborn wrote: > This can go in an anon namespace and follow Chromium naming style. Same in > irt_pnacl_translator_link/compile.cc too. Done. https://chromiumcodereview.appspot.com/1382713002/diff/160001/mojo/nacl/nonsf... mojo/nacl/nonsfi/irt_resource_open.cc:13: char nonsfi_toolchain[] = "native_client/toolchain/linux_x86" On 2015/10/20 21:32:40, Mark Seaborn wrote: > This can use 'const' Not done, since changed to string (which has file appended). https://chromiumcodereview.appspot.com/1382713002/diff/160001/mojo/nacl/nonsf... mojo/nacl/nonsfi/irt_resource_open.cc:17: strcat(path, pathname); On 2015/10/20 21:32:40, Mark Seaborn wrote: > Can you use C++ strings instead? This overruns the buffer if the caller passes > a long string, which the caller is allowed to do. Done. https://chromiumcodereview.appspot.com/1382713002/diff/160001/mojo/nacl/nonsf... mojo/nacl/nonsfi/irt_resource_open.cc:22: return -rv; On 2015/10/20 21:32:40, Mark Seaborn wrote: > Should return errno instead. Done. https://chromiumcodereview.appspot.com/1382713002/diff/160001/services/nacl/B... File services/nacl/BUILD.gn (right): https://chromiumcodereview.appspot.com/1382713002/diff/160001/services/nacl/B... services/nacl/BUILD.gn:133: mojom("pexe_bindings") { On 2015/10/20 22:28:58, Mark Seaborn wrote: > How about "pexe" -> "pnacl_translator"? Done. https://chromiumcodereview.appspot.com/1382713002/diff/160001/services/nacl/c... File services/nacl/content_handler_main_nonsfi_pexe.cc (right): https://chromiumcodereview.appspot.com/1382713002/diff/160001/services/nacl/c... services/nacl/content_handler_main_nonsfi_pexe.cc:61: app->ConnectToService("mojo:pexe_translator_app", &translator_); On 2015/10/20 22:28:58, Mark Seaborn wrote: > I'm curious whether this could just be done in RunApplication()? I'm not entirely sure -- the ApplicationImpl pointer is no longer available in RunApplication, unless it can somehow be acquired from the InterfaceRequest<mojo::Application>. https://chromiumcodereview.appspot.com/1382713002/diff/160001/services/nacl/p... File services/nacl/pexe_linker_app.cc (right): https://chromiumcodereview.appspot.com/1382713002/diff/160001/services/nacl/p... services/nacl/pexe_linker_app.cc:25: int nexe_fd = open("sean_out/ld.nexe", O_RDONLY); On 2015/10/20 21:32:40, Mark Seaborn wrote: > Needs fixing. :-) No worries, the bots wouldn't let me get past this one. In my issue description: -I'm currently using an "ld.nexe" and "pnacl-llc.nexe" from a local directory on my machine called "sean_out", since these updated nexes are not committed/rolled into deps yet. These paths will be resolved before this change is landed! https://chromiumcodereview.appspot.com/1382713002/diff/160001/services/nacl/p... File services/nacl/pexe_translator_app.cc (right): https://chromiumcodereview.appspot.com/1382713002/diff/160001/services/nacl/p... services/nacl/pexe_translator_app.cc:4: On 2015/10/20 22:28:58, Mark Seaborn wrote: > Nit: Can you name this using similar naming to the IRT interface, perhaps? > > i.e. "pnacl_translator_compile/link" or (shorter) "pnacl_compile/link"? Done. https://chromiumcodereview.appspot.com/1382713002/diff/160001/services/nacl/p... services/nacl/pexe_translator_app.cc:22: void PexeTranslate(ScopedMessagePipeHandle handle) override { On 2015/10/20 22:28:58, Mark Seaborn wrote: > Nit: fix indentation -- this is indented by 3 Done. https://chromiumcodereview.appspot.com/1382713002/diff/160001/services/nacl/p... services/nacl/pexe_translator_app.cc:27: LOG(FATAL) << "Could not open compiler nexe"; On 2015/10/20 22:28:58, Mark Seaborn wrote: > It would be nice to report the filename in these error messages. Done. https://chromiumcodereview.appspot.com/1382713002/diff/160001/services/nacl/p... services/nacl/pexe_translator_app.cc:28: ::nacl::MojoLaunchNexeNonsfi(nexe_fd, handle.Pass().get().value()); On 2015/10/20 22:28:58, Mark Seaborn wrote: > I think you can do handle.release().value() instead of > handle.Pass().get().value(). Done. https://chromiumcodereview.appspot.com/1382713002/diff/160001/services/nacl/p... services/nacl/pexe_translator_app.cc:32: class StrongBindingPexeTranslatorImpl : public PexeTranslatorImpl { On 2015/10/20 22:28:58, Mark Seaborn wrote: > How come this is a separate class from PexeTranslatorImpl? Can't the two > classes be combined? This is based on the echo server's model here: https://github.com/domokit/mojo/blob/master/examples/echo/echo_server.cc I figured they should be separated for the distinction of StrongBinding. https://chromiumcodereview.appspot.com/1382713002/diff/160001/services/nacl/p... services/nacl/pexe_translator_app.cc:42: class MultiPexeTranslator : public mojo::ApplicationDelegate, On 2015/10/20 22:28:58, Mark Seaborn wrote: > What does the "Multi" part mean? If we try to translate multiple pexes, will > the system create separate translator processes for that, or try to reuse one > process? As I mentioned earlier, I followed a similar structure to the echo server (https://github.com/domokit/mojo/blob/master/examples/echo/echo_server.cc), and although it is called "Multi", I think the entire process will be taken over once the nexe is launched. Also, as I mentioned in the description: -I'm unsure how this translation process will react to translating multiple pexes at the same time (this behavior is thus far untested -- what's the best way to go about doing these tests?). I would like to test this behavior, but I do not know how to do so currently with the Mojo shell. Ideally, each service request to the pexe_translator_app will get an entirely new process responding.
Description was changed from ========== Creating a pexe content handler to translate and run pexes. -Creates translator and linker applications, which launch the pexe translator (pnacl-llc.nexe) and pexe loader (ld.nexe), passing a message pipe handle. -Updates IRT to include callbacks for pexe translator and pexe loader. -Creates a content handler application, which contacts the translator and loader services, communicating with them via message pipes. Uses a mojom interface to pass a message pipe to each application. -The translation process places a PEXE FILE into a temporary file, compiles that pexe into a temporary OBJECT FILE, and finally links that object file into a finalized temporary NEXE FILE. All file names are passed via message pipes, and all files are unlinked after they are opened by the final service which uses them. -Adds some .pexe.mojo files to the nacl_nonsfi_apptests. Test by running: > ./mojo/tools/mojob.py build > ./out/Debug/mojo_shell --enable-multiprocess ./out/Debug/shell_apptests.pexe.mojo Issues/Questions to resolve before submitting: -In "content_handler_main_nonsfi_pexe.cc", I'm not calling "loop.Run()". Is that an issue? I feel like I do not fully understand the base::MessageLoop. -I create a new "ScopedMessagePipeHandle" pair for communication between the content handler and the linker/compiler, but once the handle reaches the nexe, it is no longer scoped. Should I be closing these handles? -I'm repeating the pattern of "Read + Wait on a handle in a do while loop" in a few places. Is there a built-in function for this? -I'm unsure how this translation process will react to translating multiple pexes at the same time (this behavior is thus far untested -- what's the best way to go about doing these tests?). BUG=https://github.com/domokit/mojo/issues/396 ========== to ========== Creating a pexe content handler to translate and run pexes. -Creates translator and linker applications, which launch the pexe translator (pnacl-llc.nexe) and pexe loader (ld.nexe), passing a message pipe handle. -Updates IRT to include callbacks for pexe translator and pexe loader. -Creates a content handler application, which contacts the translator and loader services, communicating with them via message pipes. Uses a mojom interface to pass a message pipe to each application. -The translation process places a PEXE FILE into a temporary file, compiles that pexe into a temporary OBJECT FILE, and finally links that object file into a finalized temporary NEXE FILE. All file names are passed via message pipes, and all files are unlinked after they are opened by the final service which uses them. -Adds some .pexe.mojo files to the nacl_nonsfi_apptests. Test by running: > ./mojo/tools/mojob.py build > ./out/Debug/mojo_shell --enable-multiprocess ./out/Debug/shell_apptests.pexe.mojo Issues/Questions to resolve before submitting: -I'm unsure how this translation process will react to translating multiple pexes at the same time (this behavior is thus far untested -- what's the best way to go about doing these tests?). BUG=https://github.com/domokit/mojo/issues/396 ==========
Patchset #5 (id:200001) has been deleted
Patchset #5 (id:220001) has been deleted
This patch expands the mojom interface and binds the MojoHandle (received by the nexes) to a PexeCompiler / PexeLinker interface, which can receive calls from the content handler. This approach avoids: 1) Sending raw byte streams 2) Saving global variables 3) Adding a dependency for "irt_mojo_nonsfi" on anything other than mojom.h files.
https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_m... File mojo/nacl/nonsfi/irt_mojo_nonsfi.cc (left): https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_m... mojo/nacl/nonsfi/irt_mojo_nonsfi.cc:64: nullptr}, Could you reformat all items to be consistent? https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_m... File mojo/nacl/nonsfi/irt_mojo_nonsfi.h (right): https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_m... mojo/nacl/nonsfi/irt_mojo_nonsfi.h:26: // Mechanism to contorl when nexes get access to the PNaCl translation Nit: "contorl" -> "control" https://codereview.chromium.org/1382713002/diff/240001/services/nacl/BUILD.gn File services/nacl/BUILD.gn (right): https://codereview.chromium.org/1382713002/diff/240001/services/nacl/BUILD.gn... services/nacl/BUILD.gn:54: mojo_native_application("nacl_content_handler_nonsfi_pexe_32_bit") { Nit: GN convention is to avoid duplicating the directory name in the name of the target, so this target should be named "content_handler_nonsfi_pexe_32_bit" and the same for all other targets in this file. https://codereview.chromium.org/1382713002/diff/240001/services/nacl/BUILD.gn... services/nacl/BUILD.gn:124: "${root_build_dir}/{{source_name_part}}.mojo", You can use {{source_file_part}} here and omit the ".mojo" bit since you're not changing the extension. https://codereview.chromium.org/1382713002/diff/240001/services/nacl/pnacl_co... File services/nacl/pnacl_compile.cc (right): https://codereview.chromium.org/1382713002/diff/240001/services/nacl/pnacl_co... services/nacl/pnacl_compile.cc:24: "/translator/x86-32-nonsfi/bin/pnacl-llc.nexe"; Hardcoding this path makes it difficult to test or run this elsewhere since you'll have to replicate the entire directory structure. We should probably copy pnacl-llc.nexe to build directory using a GN rule; we might even pass in the path as macro at compile time (although that might not be needed). https://codereview.chromium.org/1382713002/diff/240001/services/nacl/pnacl_li... File services/nacl/pnacl_link.cc (right): https://codereview.chromium.org/1382713002/diff/240001/services/nacl/pnacl_li... services/nacl/pnacl_link.cc:24: "/translator/x86-32-nonsfi/bin/ld.nexe"; ditto
https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_m... File mojo/nacl/nonsfi/irt_mojo_nonsfi.cc (right): https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_m... mojo/nacl/nonsfi/irt_mojo_nonsfi.cc:57: int not_pnacl_filter(void) { Naming nit: NotPNaClFilter(). Also can use "()" instead of "(void)" in C++ (where they're equivalent). https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_m... mojo/nacl/nonsfi/irt_mojo_nonsfi.cc:58: if (g_running_translator) { Just "return g_running_translator"? https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_p... File mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc (right): https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:43: char *args[] = { relocation_model, force_tls, bitcode_format }; irt_dev.h says this array should be NULL-terminated. https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:50: char* buf = new char[buf_size]; I believe you can use "scoped_ptr<char[]> buf(new char[buf_size])". https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:57: for(;;) { Add space after "for" https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_p... File mojo/nacl/nonsfi/irt_pnacl_translator_link.cc (right): https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_link.cc:21: int (*func)(int nexe_fd, const int* obj_file_fds, It might be worth typedef'ing this in this file to avoid repetition. https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_link.cc:39: int obj_file_fds[] = { open(object_file_name.get().c_str(), O_RDONLY) }; Can you use "int obj_file_fds = open(...)" here too? https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_r... File mojo/nacl/nonsfi/irt_resource_open.cc (right): https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_r... mojo/nacl/nonsfi/irt_resource_open.cc:13: int IrtOpenResource(const char *filename, int *newfd) { Nit: use "* " spacing https://codereview.chromium.org/1382713002/diff/240001/mojo/tools/data/nacl_n... File mojo/tools/data/nacl_nonsfi_apptests (right): https://codereview.chromium.org/1382713002/diff/240001/mojo/tools/data/nacl_n... mojo/tools/data/nacl_nonsfi_apptests:6: # Multiprocess mode is required for the nonsfi nacl tests: the content This command now applies to all the tests here, right? You could move it to the start of the list. https://codereview.chromium.org/1382713002/diff/240001/mojo/tools/data/nacl_n... mojo/tools/data/nacl_nonsfi_apptests:39: # TODO(smklein) Include "view_manager_service_apptests" once it isn't as slow. Nit: indent this https://codereview.chromium.org/1382713002/diff/240001/services/nacl/BUILD.gn File services/nacl/BUILD.gn (right): https://codereview.chromium.org/1382713002/diff/240001/services/nacl/BUILD.gn... services/nacl/BUILD.gn:66: "//mojo/public/platform/native:mgl_thunks", These are being repeated across all of these mojo_native_applications. Shouldn't these be dependencies of "//mojo/nacl/nonsfi:irt_mojo_nonsfi" instead? https://codereview.chromium.org/1382713002/diff/240001/services/nacl/content_... File services/nacl/content_handler_main_nonsfi_pexe.cc (right): https://codereview.chromium.org/1382713002/diff/240001/services/nacl/content_... services/nacl/content_handler_main_nonsfi_pexe.cc:22: // RunnableImpl and MakeRunnable lifted from Is this different from base::Bind() (from base/bind.h)? https://codereview.chromium.org/1382713002/diff/240001/services/nacl/content_... services/nacl/content_handler_main_nonsfi_pexe.cc:45: class CompilerUI { Does "UI" mean "user interface" here? https://codereview.chromium.org/1382713002/diff/240001/services/nacl/content_... services/nacl/content_handler_main_nonsfi_pexe.cc:55: compiler_->PexeCompile(pexe_file_path, callback_); Hmm, isn't invoking a method synchronously a common enough thing to do that it should be less awkward than this? There are some examples in: mojo/services/files/c/lib/file_fd_impl.cc e.g. mojo::files::Error error = mojo::files::Error::INTERNAL; file_->Close(Capture(&error)); if (!file_.WaitForIncomingResponse()) return errno_setter.Set(ESTALE); Could you use Capture() for the callback? It is defined in mojo/services/files/c/lib/template_util.h and it uses a C++ lambda. https://codereview.chromium.org/1382713002/diff/240001/services/nacl/pnacl_co... File services/nacl/pnacl_compile.cc (right): https://codereview.chromium.org/1382713002/diff/240001/services/nacl/pnacl_co... services/nacl/pnacl_compile.cc:23: char path[] = "native_client/toolchain/linux_x86/pnacl_translator" You want "static const char kPath[]". "char path[]" will make a copy of this on the stack. https://codereview.chromium.org/1382713002/diff/240001/services/nacl/pnacl_co... File services/nacl/pnacl_compile.mojom (right): https://codereview.chromium.org/1382713002/diff/240001/services/nacl/pnacl_co... services/nacl/pnacl_compile.mojom:12: PexeCompile(string? pexe_file_name) => (string? object_file_name); Does "?" mean "optional" (i.e. may be null)? (I've not found any docs for the mojom interface language yet. Got any pointers?)
Patchset #6 (id:260001) has been deleted
Patchset #6 (id:280001) has been deleted
Patchset #6 (id:300001) has been deleted
https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_m... File mojo/nacl/nonsfi/irt_mojo_nonsfi.cc (left): https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_m... mojo/nacl/nonsfi/irt_mojo_nonsfi.cc:64: nullptr}, On 2015/10/27 15:07:52, Petr Hosek wrote: > Could you reformat all items to be consistent? Done. Also, added comments for GPU-related IRT interfaces (let me know if something is inaccurate; I'm trying to avoid making kIrtInterfaces a blob of undocumented structs) https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_m... File mojo/nacl/nonsfi/irt_mojo_nonsfi.cc (right): https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_m... mojo/nacl/nonsfi/irt_mojo_nonsfi.cc:57: int not_pnacl_filter(void) { On 2015/10/27 17:30:20, Mark Seaborn wrote: > Naming nit: NotPNaClFilter(). > > Also can use "()" instead of "(void)" in C++ (where they're equivalent). Done. https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_m... mojo/nacl/nonsfi/irt_mojo_nonsfi.cc:58: if (g_running_translator) { On 2015/10/27 17:30:20, Mark Seaborn wrote: > Just "return g_running_translator"? Done. https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_m... File mojo/nacl/nonsfi/irt_mojo_nonsfi.h (right): https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_m... mojo/nacl/nonsfi/irt_mojo_nonsfi.h:26: // Mechanism to contorl when nexes get access to the PNaCl translation On 2015/10/27 15:07:52, Petr Hosek wrote: > Nit: "contorl" -> "control" Done. https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_p... File mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc (right): https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:43: char *args[] = { relocation_model, force_tls, bitcode_format }; On 2015/10/27 17:30:20, Mark Seaborn wrote: > irt_dev.h says this array should be NULL-terminated. Added sentinel https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:50: char* buf = new char[buf_size]; On 2015/10/27 17:30:20, Mark Seaborn wrote: > I believe you can use "scoped_ptr<char[]> buf(new char[buf_size])". Done -- also, usage of "buf" changed to "&buf[0]", and "delete[] buf" removed. https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:57: for(;;) { On 2015/10/27 17:30:20, Mark Seaborn wrote: > Add space after "for" Done. https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_p... File mojo/nacl/nonsfi/irt_pnacl_translator_link.cc (right): https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_link.cc:21: int (*func)(int nexe_fd, const int* obj_file_fds, On 2015/10/27 17:30:20, Mark Seaborn wrote: > It might be worth typedef'ing this in this file to avoid repetition. Done. https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_link.cc:39: int obj_file_fds[] = { open(object_file_name.get().c_str(), O_RDONLY) }; On 2015/10/27 17:30:20, Mark Seaborn wrote: > Can you use "int obj_file_fds = open(...)" here too? Done. https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_r... File mojo/nacl/nonsfi/irt_resource_open.cc (right): https://codereview.chromium.org/1382713002/diff/240001/mojo/nacl/nonsfi/irt_r... mojo/nacl/nonsfi/irt_resource_open.cc:13: int IrtOpenResource(const char *filename, int *newfd) { On 2015/10/27 17:30:20, Mark Seaborn wrote: > Nit: use "* " spacing Done. https://codereview.chromium.org/1382713002/diff/240001/mojo/tools/data/nacl_n... File mojo/tools/data/nacl_nonsfi_apptests (right): https://codereview.chromium.org/1382713002/diff/240001/mojo/tools/data/nacl_n... mojo/tools/data/nacl_nonsfi_apptests:6: # Multiprocess mode is required for the nonsfi nacl tests: the content On 2015/10/27 17:30:20, Mark Seaborn wrote: > This command now applies to all the tests here, right? You could move it to the > start of the list. Done. https://codereview.chromium.org/1382713002/diff/240001/mojo/tools/data/nacl_n... mojo/tools/data/nacl_nonsfi_apptests:39: # TODO(smklein) Include "view_manager_service_apptests" once it isn't as slow. On 2015/10/27 17:30:20, Mark Seaborn wrote: > Nit: indent this Done. https://codereview.chromium.org/1382713002/diff/240001/services/nacl/BUILD.gn File services/nacl/BUILD.gn (right): https://codereview.chromium.org/1382713002/diff/240001/services/nacl/BUILD.gn... services/nacl/BUILD.gn:54: mojo_native_application("nacl_content_handler_nonsfi_pexe_32_bit") { On 2015/10/27 15:07:52, Petr Hosek wrote: > Nit: GN convention is to avoid duplicating the directory name in the name of the > target, so this target should be named "content_handler_nonsfi_pexe_32_bit" and > the same for all other targets in this file. Done. https://codereview.chromium.org/1382713002/diff/240001/services/nacl/BUILD.gn... services/nacl/BUILD.gn:66: "//mojo/public/platform/native:mgl_thunks", On 2015/10/27 17:30:20, Mark Seaborn wrote: > These are being repeated across all of these mojo_native_applications. > Shouldn't these be dependencies of "//mojo/nacl/nonsfi:irt_mojo_nonsfi" instead? Done. https://codereview.chromium.org/1382713002/diff/240001/services/nacl/BUILD.gn... services/nacl/BUILD.gn:124: "${root_build_dir}/{{source_name_part}}.mojo", On 2015/10/27 15:07:52, Petr Hosek wrote: > You can use {{source_file_part}} here and omit the ".mojo" bit since you're not > changing the extension. Done. https://codereview.chromium.org/1382713002/diff/240001/services/nacl/content_... File services/nacl/content_handler_main_nonsfi_pexe.cc (right): https://codereview.chromium.org/1382713002/diff/240001/services/nacl/content_... services/nacl/content_handler_main_nonsfi_pexe.cc:22: // RunnableImpl and MakeRunnable lifted from On 2015/10/27 17:30:21, Mark Seaborn wrote: > Is this different from base::Bind() (from base/bind.h)? Nope -- Removed. Now I know what base::Bind() does! https://codereview.chromium.org/1382713002/diff/240001/services/nacl/content_... services/nacl/content_handler_main_nonsfi_pexe.cc:45: class CompilerUI { On 2015/10/27 17:30:21, Mark Seaborn wrote: > Does "UI" mean "user interface" here? Yeah, I used this naming scheme (https://cs.corp.google.com/#github/domokit/mojo/mojo/public/cpp/bindings/test...). I can change it to be "CompilerUserInterface" or "CompilerInterface" if you'd prefer. https://codereview.chromium.org/1382713002/diff/240001/services/nacl/content_... services/nacl/content_handler_main_nonsfi_pexe.cc:55: compiler_->PexeCompile(pexe_file_path, callback_); On 2015/10/27 17:30:20, Mark Seaborn wrote: > Hmm, isn't invoking a method synchronously a common enough thing to do that it > should be less awkward than this? > > There are some examples in: > mojo/services/files/c/lib/file_fd_impl.cc > > e.g. > mojo::files::Error error = mojo::files::Error::INTERNAL; > file_->Close(Capture(&error)); > if (!file_.WaitForIncomingResponse()) > return errno_setter.Set(ESTALE); > > Could you use Capture() for the callback? It is defined in > mojo/services/files/c/lib/template_util.h and it uses a C++ lambda. Done -- this also removed the need for base::Bind. https://codereview.chromium.org/1382713002/diff/240001/services/nacl/pnacl_co... File services/nacl/pnacl_compile.cc (right): https://codereview.chromium.org/1382713002/diff/240001/services/nacl/pnacl_co... services/nacl/pnacl_compile.cc:23: char path[] = "native_client/toolchain/linux_x86/pnacl_translator" On 2015/10/27 17:30:21, Mark Seaborn wrote: > You want "static const char kPath[]". "char path[]" will make a copy of this on > the stack. Done. https://codereview.chromium.org/1382713002/diff/240001/services/nacl/pnacl_co... services/nacl/pnacl_compile.cc:24: "/translator/x86-32-nonsfi/bin/pnacl-llc.nexe"; On 2015/10/27 15:07:52, Petr Hosek wrote: > Hardcoding this path makes it difficult to test or run this elsewhere since > you'll have to replicate the entire directory structure. We should probably copy > pnacl-llc.nexe to build directory using a GN rule; we might even pass in the > path as macro at compile time (although that might not be needed). I totally agree -- Updated irt_mojo_nonsfi, pnacl_compile, pnacl_link, and irt_resource_compile to deal with this issue. https://codereview.chromium.org/1382713002/diff/240001/services/nacl/pnacl_co... File services/nacl/pnacl_compile.mojom (right): https://codereview.chromium.org/1382713002/diff/240001/services/nacl/pnacl_co... services/nacl/pnacl_compile.mojom:12: PexeCompile(string? pexe_file_name) => (string? object_file_name); On 2015/10/27 17:30:21, Mark Seaborn wrote: > Does "?" mean "optional" (i.e. may be null)? > > (I've not found any docs for the mojom interface language yet. Got any > pointers?) It means nullable, apparently. I'll remove it, since we don't want to be passing null strings. Search for "NullableReferenceType" in here: https://docs.google.com/document/d/1r7yCseBktlDEN9CKp_JWD0ZYxMi4GCsLXMvSN5sI0... https://codereview.chromium.org/1382713002/diff/240001/services/nacl/pnacl_li... File services/nacl/pnacl_link.cc (right): https://codereview.chromium.org/1382713002/diff/240001/services/nacl/pnacl_li... services/nacl/pnacl_link.cc:24: "/translator/x86-32-nonsfi/bin/ld.nexe"; On 2015/10/27 15:07:52, Petr Hosek wrote: > ditto Done.
As a follow up -- services/nacl/pnacl_compile.cc and services/nacl/pnacl_link.cc should definitely be combined into one service, since they are almost identical. However, I first need the ability to request that the mojo shell launch this service as an "anti-singleton", such that each connection to the service gives a new process. vtl said this is doable, but it does not exist yet.
LGTM https://codereview.chromium.org/1382713002/diff/320001/mojo/nacl/nonsfi/irt_m... File mojo/nacl/nonsfi/irt_mojo_nonsfi.cc (right): https://codereview.chromium.org/1382713002/diff/320001/mojo/nacl/nonsfi/irt_m... mojo/nacl/nonsfi/irt_mojo_nonsfi.cc:17: std::string g_base_path = ""; This is a non-POD initialiser. Chromium has a policy against those because of their startup-time cost. You could fix this by using base/lazy_instance.h. See: http://neugierig.org/software/chromium/notes/2011/08/static-initializers.html https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/p6h3HC8Wro4 https://codereview.chromium.org/1382713002/diff/320001/mojo/nacl/nonsfi/irt_m... mojo/nacl/nonsfi/irt_mojo_nonsfi.cc:117: void MojoSetBasePath(std::string base_path) { Can be "const std::string&" to avoid one copy. https://codereview.chromium.org/1382713002/diff/320001/mojo/nacl/nonsfi/irt_p... File mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc (right): https://codereview.chromium.org/1382713002/diff/320001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:40: char relocation_model[] = "-relocation-model=pic"; How about some comments on these options? For this one: "Non-SFI mode requires PIC" https://codereview.chromium.org/1382713002/diff/320001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:41: char force_tls[] = "-force-tls-non-pic"; This is copied from pnacl-translate.py but I suspect we don't need it any more. You could add a comment to that effect or remove it. https://codereview.chromium.org/1382713002/diff/320001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:43: char sentinel[] = "\0"; The sentinel is supposed to be NULL, not an empty string. (Also, "" would be equivalent to "\0" as a C string.) https://codereview.chromium.org/1382713002/diff/320001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:44: char *args[] = { relocation_model, force_tls, bitcode_format, sentinel }; Spacing: "char*" https://codereview.chromium.org/1382713002/diff/320001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:59: size_t num_bytes_from_pexe = fread(&buf[0], 1, buf_size, buf.get() is probably more idiomatic for scoped_ptr. Same below. https://codereview.chromium.org/1382713002/diff/320001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:69: You could replace the original "delete[] buf;" with buf.reset() to deallocate earlier. https://codereview.chromium.org/1382713002/diff/320001/services/nacl/BUILD.gn File services/nacl/BUILD.gn (right): https://codereview.chromium.org/1382713002/diff/320001/services/nacl/BUILD.gn... services/nacl/BUILD.gn:133: print(rebase_path("//")) Is this debugging code you forgot to take out? https://codereview.chromium.org/1382713002/diff/320001/services/nacl/content_... File services/nacl/content_handler_main_nonsfi_pexe.cc (right): https://codereview.chromium.org/1382713002/diff/320001/services/nacl/content_... services/nacl/content_handler_main_nonsfi_pexe.cc:32: compiler_->PexeCompile(pexe_file_path, mojio::Capture(&output_)); output_ can now be a local variable instead of a class member. https://codereview.chromium.org/1382713002/diff/320001/services/nacl/content_... services/nacl/content_handler_main_nonsfi_pexe.cc:34: LOG(FATAL) << "Could not wait for pexe compiler"; Sounds impatient. :-) Maybe "Waiting for pexe compiler failed"? https://codereview.chromium.org/1382713002/diff/320001/services/nacl/pnacl_li... File services/nacl/pnacl_link.cc (right): https://codereview.chromium.org/1382713002/diff/320001/services/nacl/pnacl_li... services/nacl/pnacl_link.cc:37: virtual std::string GetBasePath() { return ""; } This could presumably be "virtual ... = 0"? But I don't see why you need this method. If this is global anyway, why not do MojoSetBasePath() in Initialize() and MojoGetBasePath() in PexeLinkerStart()? Or at least make "std::string base_path_" a member of PexeLinkerImpl and pass base_path through all the constructors. https://codereview.chromium.org/1382713002/diff/320001/services/nacl/pnacl_li... services/nacl/pnacl_link.cc:75: std::string base_path; This could be private.
Patchset #7 (id:340001) has been deleted
https://codereview.chromium.org/1382713002/diff/320001/mojo/nacl/nonsfi/irt_m... File mojo/nacl/nonsfi/irt_mojo_nonsfi.cc (right): https://codereview.chromium.org/1382713002/diff/320001/mojo/nacl/nonsfi/irt_m... mojo/nacl/nonsfi/irt_mojo_nonsfi.cc:17: std::string g_base_path = ""; On 2015/10/28 18:31:43, Mark Seaborn wrote: > This is a non-POD initialiser. Chromium has a policy against those because of > their startup-time cost. > > You could fix this by using base/lazy_instance.h. > > See: > http://neugierig.org/software/chromium/notes/2011/08/static-initializers.html > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/p6h3HC8Wro4 This is interesting. Good to know! I'll be more careful about initializing static variables. In the process of adding LazyInitializer here, I ran into a runtime error: "[1028/123526:FATAL:at_exit.cc(53)] Check failed: false. Tried to RegisterCallback without an AtExitManager". This was actually something I ran into while trying to use base::PathService to determine the "out/Debug" path. Turns out, the fix is to add a "base::AtExitManager" variable on the stack, which I did in MojoMain. This fixed the PathService bug too! Long story short -- g_base_path and all related functions/uses have been removed from irt_mojo_nonsfi, and calls to PathService are being used instead. https://codereview.chromium.org/1382713002/diff/320001/mojo/nacl/nonsfi/irt_m... mojo/nacl/nonsfi/irt_mojo_nonsfi.cc:117: void MojoSetBasePath(std::string base_path) { On 2015/10/28 18:31:43, Mark Seaborn wrote: > Can be "const std::string&" to avoid one copy. Done, but this function is being removed anyway. https://codereview.chromium.org/1382713002/diff/320001/mojo/nacl/nonsfi/irt_p... File mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc (right): https://codereview.chromium.org/1382713002/diff/320001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:40: char relocation_model[] = "-relocation-model=pic"; On 2015/10/28 18:31:43, Mark Seaborn wrote: > How about some comments on these options? For this one: "Non-SFI mode requires > PIC" Done. https://codereview.chromium.org/1382713002/diff/320001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:41: char force_tls[] = "-force-tls-non-pic"; On 2015/10/28 18:31:43, Mark Seaborn wrote: > This is copied from pnacl-translate.py but I suspect we don't need it any more. > You could add a comment to that effect or remove it. Removed. https://codereview.chromium.org/1382713002/diff/320001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:43: char sentinel[] = "\0"; On 2015/10/28 18:31:43, Mark Seaborn wrote: > The sentinel is supposed to be NULL, not an empty string. > > (Also, "" would be equivalent to "\0" as a C string.) Updated to nullptr in args. https://codereview.chromium.org/1382713002/diff/320001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:44: char *args[] = { relocation_model, force_tls, bitcode_format, sentinel }; On 2015/10/28 18:31:43, Mark Seaborn wrote: > Spacing: "char*" Done. https://codereview.chromium.org/1382713002/diff/320001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:59: size_t num_bytes_from_pexe = fread(&buf[0], 1, buf_size, On 2015/10/28 18:31:43, Mark Seaborn wrote: > buf.get() is probably more idiomatic for scoped_ptr. Same below. Done. https://codereview.chromium.org/1382713002/diff/320001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:69: On 2015/10/28 18:31:43, Mark Seaborn wrote: > You could replace the original "delete[] buf;" with buf.reset() to deallocate > earlier. Done. https://codereview.chromium.org/1382713002/diff/320001/services/nacl/BUILD.gn File services/nacl/BUILD.gn (right): https://codereview.chromium.org/1382713002/diff/320001/services/nacl/BUILD.gn... services/nacl/BUILD.gn:133: print(rebase_path("//")) On 2015/10/28 18:31:43, Mark Seaborn wrote: > Is this debugging code you forgot to take out? Yup. Deleted. https://codereview.chromium.org/1382713002/diff/320001/services/nacl/content_... File services/nacl/content_handler_main_nonsfi_pexe.cc (right): https://codereview.chromium.org/1382713002/diff/320001/services/nacl/content_... services/nacl/content_handler_main_nonsfi_pexe.cc:32: compiler_->PexeCompile(pexe_file_path, mojio::Capture(&output_)); On 2015/10/28 18:31:43, Mark Seaborn wrote: > output_ can now be a local variable instead of a class member. Done. https://codereview.chromium.org/1382713002/diff/320001/services/nacl/content_... services/nacl/content_handler_main_nonsfi_pexe.cc:34: LOG(FATAL) << "Could not wait for pexe compiler"; On 2015/10/28 18:31:43, Mark Seaborn wrote: > Sounds impatient. :-) Maybe "Waiting for pexe compiler failed"? Done. https://codereview.chromium.org/1382713002/diff/320001/services/nacl/pnacl_li... File services/nacl/pnacl_link.cc (right): https://codereview.chromium.org/1382713002/diff/320001/services/nacl/pnacl_li... services/nacl/pnacl_link.cc:37: virtual std::string GetBasePath() { return ""; } On 2015/10/28 18:31:43, Mark Seaborn wrote: > This could presumably be "virtual ... = 0"? > > But I don't see why you need this method. If this is global anyway, why not do > MojoSetBasePath() in Initialize() and MojoGetBasePath() in PexeLinkerStart()? > > Or at least make "std::string base_path_" a member of PexeLinkerImpl and pass > base_path through all the constructors. Replaced with PathService, so this has been removed. https://codereview.chromium.org/1382713002/diff/320001/services/nacl/pnacl_li... services/nacl/pnacl_link.cc:75: std::string base_path; On 2015/10/28 18:31:43, Mark Seaborn wrote: > This could be private. Removed.
LGTM https://codereview.chromium.org/1382713002/diff/360001/mojo/nacl/nonsfi/irt_r... File mojo/nacl/nonsfi/irt_resource_open.cc (right): https://codereview.chromium.org/1382713002/diff/360001/mojo/nacl/nonsfi/irt_r... mojo/nacl/nonsfi/irt_resource_open.cc:16: PathService::Get(base::DIR_MODULE, &base_path); How about returning ENOENT if this fails? https://codereview.chromium.org/1382713002/diff/360001/mojo/nacl/nonsfi/irt_r... mojo/nacl/nonsfi/irt_resource_open.cc:18: path.append("/pnacl_translation_files/"); You could use FilePath()'s Append() method instead. That adds separators in a portable way. https://codereview.chromium.org/1382713002/diff/360001/mojo/nacl/nonsfi/irt_r... mojo/nacl/nonsfi/irt_resource_open.cc:25: return -errno; This shouldn't be negated. https://codereview.chromium.org/1382713002/diff/360001/services/nacl/pnacl_co... File services/nacl/pnacl_compile.cc (right): https://codereview.chromium.org/1382713002/diff/360001/services/nacl/pnacl_co... services/nacl/pnacl_compile.cc:26: PathService::Get(base::DIR_MODULE, &base_path); Ideally should check for failure here.
https://codereview.chromium.org/1382713002/diff/360001/mojo/nacl/nonsfi/irt_r... File mojo/nacl/nonsfi/irt_resource_open.cc (right): https://codereview.chromium.org/1382713002/diff/360001/mojo/nacl/nonsfi/irt_r... mojo/nacl/nonsfi/irt_resource_open.cc:16: PathService::Get(base::DIR_MODULE, &base_path); On 2015/10/28 21:53:01, Mark Seaborn wrote: > How about returning ENOENT if this fails? Done. https://codereview.chromium.org/1382713002/diff/360001/mojo/nacl/nonsfi/irt_r... mojo/nacl/nonsfi/irt_resource_open.cc:18: path.append("/pnacl_translation_files/"); On 2015/10/28 21:53:01, Mark Seaborn wrote: > You could use FilePath()'s Append() method instead. That adds separators in a > portable way. Done, and for consistency, updated pnacl_link and pnacl_compile too. https://codereview.chromium.org/1382713002/diff/360001/mojo/nacl/nonsfi/irt_r... mojo/nacl/nonsfi/irt_resource_open.cc:25: return -errno; On 2015/10/28 21:53:01, Mark Seaborn wrote: > This shouldn't be negated. Done. https://codereview.chromium.org/1382713002/diff/360001/services/nacl/pnacl_co... File services/nacl/pnacl_compile.cc (right): https://codereview.chromium.org/1382713002/diff/360001/services/nacl/pnacl_co... services/nacl/pnacl_compile.cc:26: PathService::Get(base::DIR_MODULE, &base_path); On 2015/10/28 21:53:01, Mark Seaborn wrote: > Ideally should check for failure here. Done.
lgtm https://codereview.chromium.org/1382713002/diff/380001/mojo/nacl/nonsfi/irt_p... File mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc (right): https://codereview.chromium.org/1382713002/diff/380001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:50: size_t buf_size = 0x100000; Shouldn't this be a global constant i.e. kBufferSize?
https://codereview.chromium.org/1382713002/diff/380001/mojo/nacl/nonsfi/irt_p... File mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc (right): https://codereview.chromium.org/1382713002/diff/380001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:50: size_t buf_size = 0x100000; On 2015/10/28 23:06:09, Petr Hosek wrote: > Shouldn't this be a global constant i.e. kBufferSize? Updated to "static const k_buffer_size"
Description was changed from ========== Creating a pexe content handler to translate and run pexes. -Creates translator and linker applications, which launch the pexe translator (pnacl-llc.nexe) and pexe loader (ld.nexe), passing a message pipe handle. -Updates IRT to include callbacks for pexe translator and pexe loader. -Creates a content handler application, which contacts the translator and loader services, communicating with them via message pipes. Uses a mojom interface to pass a message pipe to each application. -The translation process places a PEXE FILE into a temporary file, compiles that pexe into a temporary OBJECT FILE, and finally links that object file into a finalized temporary NEXE FILE. All file names are passed via message pipes, and all files are unlinked after they are opened by the final service which uses them. -Adds some .pexe.mojo files to the nacl_nonsfi_apptests. Test by running: > ./mojo/tools/mojob.py build > ./out/Debug/mojo_shell --enable-multiprocess ./out/Debug/shell_apptests.pexe.mojo Issues/Questions to resolve before submitting: -I'm unsure how this translation process will react to translating multiple pexes at the same time (this behavior is thus far untested -- what's the best way to go about doing these tests?). BUG=https://github.com/domokit/mojo/issues/396 ========== to ========== Creating a pexe content handler to translate and run pexes. -Creates translator and linker applications, which launch the pexe translator (pnacl-llc.nexe) and pexe loader (ld.nexe), passing a message pipe handle. -Updates IRT to include callbacks for pexe translator and pexe loader. -Creates a content handler application, which contacts the translator and loader services, communicating with them via message pipes. Uses a mojom interface to pass a message pipe to each application. -The translation process places a PEXE FILE into a temporary file, compiles that pexe into a temporary OBJECT FILE, and finally links that object file into a finalized temporary NEXE FILE. All file names are passed via message pipes, and all files are unlinked after they are opened by the final service which uses them. -Adds some .pexe.mojo files to the nacl_nonsfi_apptests. Test by running: > ./mojo/tools/mojob.py build > ./out/Debug/mojo_shell --enable-multiprocess ./out/Debug/shell_apptests.pexe.mojo BUG=https://github.com/domokit/mojo/issues/396 ==========
https://codereview.chromium.org/1382713002/diff/380001/mojo/nacl/nonsfi/irt_p... File mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc (right): https://codereview.chromium.org/1382713002/diff/380001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:50: size_t buf_size = 0x100000; On 2015/10/28 23:21:48, smklein1 wrote: > On 2015/10/28 23:06:09, Petr Hosek wrote: > > Shouldn't this be a global constant i.e. kBufferSize? > > Updated to "static const k_buffer_size" Chromium style for naming constants is kCamelCase.
https://codereview.chromium.org/1382713002/diff/380001/mojo/nacl/nonsfi/irt_p... File mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc (right): https://codereview.chromium.org/1382713002/diff/380001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:50: size_t buf_size = 0x100000; On 2015/10/28 23:24:00, Petr Hosek wrote: > On 2015/10/28 23:21:48, smklein1 wrote: > > On 2015/10/28 23:06:09, Petr Hosek wrote: > > > Shouldn't this be a global constant i.e. kBufferSize? > > > > Updated to "static const k_buffer_size" > > Chromium style for naming constants is kCamelCase. Ah. I was just trying to match the rest of the variables in mojo. Updated.
Message was sent while issue was closed.
Committed patchset #10 (id:420001) manually as 2ba2b8d5259f250e4f7933de76645cf5113db6c0 (presubmit successful).
Message was sent while issue was closed.
jamesr@chromium.org changed reviewers: + jamesr@chromium.org
Message was sent while issue was closed.
Sean asked me to take a look at things that might look a bit odd since we're seeing rare hangs. A few things noted below but nothing sticks out as definitely buggy. https://codereview.chromium.org/1382713002/diff/420001/mojo/nacl/nonsfi/irt_p... File mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc (right): https://codereview.chromium.org/1382713002/diff/420001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:58: for (;;) { this loop could take a long time (or forever) with no output. maybe a good place to add some logging? https://codereview.chromium.org/1382713002/diff/420001/services/nacl/content_... File services/nacl/content_handler_main_nonsfi_pexe.cc (right): https://codereview.chromium.org/1382713002/diff/420001/services/nacl/content_... services/nacl/content_handler_main_nonsfi_pexe.cc:89: base::MessageLoop loop(mojo::common::MessagePumpMojo::Create()); why do you need to create another message loop? mojo::ApplicationRunnerChromium creates a mojo message pump for you: https://github.com/domokit/mojo/blob/master/mojo/application/application_runn... https://codereview.chromium.org/1382713002/diff/420001/services/nacl/pnacl_li... File services/nacl/pnacl_link.cc (right): https://codereview.chromium.org/1382713002/diff/420001/services/nacl/pnacl_li... services/nacl/pnacl_link.cc:69: base::AtExitManager at_exit; what needs a base::AtExitManager? mojo::ApplicationRunnerChromium creates a base::AtExitManager for you but it's at a slightly different time than this code is doing. normally I'd expect a particular app to either use base and use mojo::ApplicationRunnerChromium or not use base concepts at all and use mojo::ApplicationRunner
Message was sent while issue was closed.
Thanks for helping me review this! All updates are made in a new CL: https://codereview.chromium.org/1429953003/ https://codereview.chromium.org/1382713002/diff/420001/mojo/nacl/nonsfi/irt_p... File mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc (right): https://codereview.chromium.org/1382713002/diff/420001/mojo/nacl/nonsfi/irt_p... mojo/nacl/nonsfi/irt_pnacl_translator_compile.cc:58: for (;;) { On 2015/11/02 23:00:01, jamesr wrote: > this loop could take a long time (or forever) with no output. maybe a good place > to add some logging? Done -- the presubmit will complain, though. https://codereview.chromium.org/1382713002/diff/420001/services/nacl/content_... File services/nacl/content_handler_main_nonsfi_pexe.cc (right): https://codereview.chromium.org/1382713002/diff/420001/services/nacl/content_... services/nacl/content_handler_main_nonsfi_pexe.cc:84: // Overridden from ContentHandlerFactory::ManagedDelegate: Fixed this incorrect comment regarding ManagedDelegate -- actually inheriting from ContentHandlerFactory::Delegate. https://codereview.chromium.org/1382713002/diff/420001/services/nacl/content_... services/nacl/content_handler_main_nonsfi_pexe.cc:89: base::MessageLoop loop(mojo::common::MessagePumpMojo::Create()); On 2015/11/02 23:00:02, jamesr wrote: > why do you need to create another message loop? mojo::ApplicationRunnerChromium > creates a mojo message pump for you: > > https://github.com/domokit/mojo/blob/master/mojo/application/application_runn... As discussed offline, the ContentHandlerFactory is running RunApplication on a different thread. Removing the message loop here causes the content handler to fail on the call to "compiler_init_->PexeCompilerStart(...)". https://codereview.chromium.org/1382713002/diff/420001/services/nacl/pnacl_li... File services/nacl/pnacl_link.cc (right): https://codereview.chromium.org/1382713002/diff/420001/services/nacl/pnacl_li... services/nacl/pnacl_link.cc:69: base::AtExitManager at_exit; On 2015/11/02 23:00:02, jamesr wrote: > what needs a base::AtExitManager? mojo::ApplicationRunnerChromium creates a > base::AtExitManager for you but it's at a slightly different time than this code > is doing. normally I'd expect a particular app to either use base and use > mojo::ApplicationRunnerChromium or not use base concepts at all and use > mojo::ApplicationRunner The base::AtExitManager is needed for the call to PathService on line 26. I will need to find a different mechanism to get the root mojo directory for Android anyway -- something akin to calling Context::ResolveShellFileURL(""), but from a Mojo application. Is this possible? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
