Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1341873002:
Enabling 64-bit mojo shell to launch 32-bit child to handle nonsfi content. (Closed)
|
Created:
5 years, 3 months ago by Sean Klein Modified:
5 years, 3 months ago CC:
mojo-reviews_chromium.org, gregsimon, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://github.com/domokit/mojo.git@master Target Ref:
refs/heads/master Project:
mojo Visibility:
Public. |
DescriptionEnabling 64-bit mojo shell to launch 32-bit child to handle nonsfi content.
This patch allows the nonsfi content handler to be used from the mojo
shell in two ways. First, it can be built for a purely 32-bit system
(--target-cpu=x86), and run without "--enable-multiprocess". Alternatively,
the nonsfi content handler can be run from a 64-bit version of the mojo
shell, as long as "--enable-multiprocess" is used. This will fork/exec a
32-bit child whenever the nonsfi content handler is detected.
BUG=https://github.com/domokit/mojo/issues/396
R=mseaborn@chromium.org
Committed: https://chromium.googlesource.com/external/mojo/+/a7ffbe0f340df9adc1b3f948b8a5c83593308701
Patch Set 1 : #
Total comments: 32
Patch Set 2 : #
Total comments: 13
Patch Set 3 : #
Total comments: 12
Patch Set 4 : Anonymous namespaces and modified target names #
Messages
Total messages: 14 (4 generated)
Patchset #1 (id:1) has been deleted
smklein@chromium.org changed reviewers: + mseaborn@chromium.org, phosek@chromium.org
https://codereview.chromium.org/1341873002/diff/20001/shell/context.cc File shell/context.cc (right): https://codereview.chromium.org/1341873002/diff/20001/shell/context.cc#newcod... shell/context.cc:261: shell_path.DirName().AppendASCII("clang_x86/mojo_shell_child"); I'm not entirely sure if I should be dependent on the clang_x86 toolchain here. I could potentially create another copy rule to move "clang_x86/mojo_shell_child" to "mojo_shell_child_x86", and call it regardless of the toolchain that built it. (That being said, this current version does technically work) https://codereview.chromium.org/1341873002/diff/20001/shell/out_of_process_na... File shell/out_of_process_native_runner.cc (right): https://codereview.chromium.org/1341873002/diff/20001/shell/out_of_process_na... shell/out_of_process_native_runner.cc:45: require_32_bit = true; I am fully aware this is kind of hacky -- mojo people suggested reading ELF headers to identify if 32-bit child shell is required. That being said, this version does currently work. I would be happy to write the ELF-reading code, but I would rather not start from scratch, since reusing good code is probably better than rolling my own. Any pointers on which elf util I should be using?
https://codereview.chromium.org/1341873002/diff/20001/services/nacl/BUILD.gn File services/nacl/BUILD.gn (right): https://codereview.chromium.org/1341873002/diff/20001/services/nacl/BUILD.gn#... services/nacl/BUILD.gn:49: # Copy to the root build directory so our content handler prefixed line How about: "so that the '#!' prefix line for invoking our content handler..." (easier to parse) https://codereview.chromium.org/1341873002/diff/20001/services/nacl/BUILD.gn#... services/nacl/BUILD.gn:72: deps += [ ":nacl_content_handler_nonsfi_copy(//build/toolchain/linux:clang_x86)" ] See other comment -- comment to warn about assuming clang here? https://codereview.chromium.org/1341873002/diff/20001/shell/BUILD.gn File shell/BUILD.gn (right): https://codereview.chromium.org/1341873002/diff/20001/shell/BUILD.gn#newcode114 shell/BUILD.gn:114: ":mojo_shell_child(//build/toolchain/linux:clang_x86)", Nit: Shouldn't this come with the same warning as you added in https://codereview.chromium.org/1327913004? https://codereview.chromium.org/1341873002/diff/20001/shell/child_process_hos... File shell/child_process_host_unittest.cc (right): https://codereview.chromium.org/1341873002/diff/20001/shell/child_process_hos... shell/child_process_host_unittest.cc:53: child_process_host.Start(require_32_bit); Alternative style is: child_process_host.Start(false /* require_32_bit */); https://codereview.chromium.org/1341873002/diff/20001/shell/context.cc File shell/context.cc (right): https://codereview.chromium.org/1341873002/diff/20001/shell/context.cc#newcod... shell/context.cc:261: shell_path.DirName().AppendASCII("clang_x86/mojo_shell_child"); On 2015/09/14 18:10:33, smklein1 wrote: > I'm not entirely sure if I should be dependent on the clang_x86 toolchain here. > I could potentially create another copy rule to move > "clang_x86/mojo_shell_child" > to > "mojo_shell_child_x86", > and call it regardless of the toolchain that built it. > > > (That being said, this current version does technically work) I think this depends how mojo_shell gets packaged. Whether this is OK is up to the owners of the code. https://codereview.chromium.org/1341873002/diff/20001/shell/out_of_process_na... File shell/out_of_process_native_runner.cc (right): https://codereview.chromium.org/1341873002/diff/20001/shell/out_of_process_na... shell/out_of_process_native_runner.cc:45: require_32_bit = true; On 2015/09/14 18:10:34, smklein1 wrote: > I am fully aware this is kind of hacky -- mojo people suggested reading ELF > headers to identify if 32-bit child shell is required. That being said, this > version does currently work. I think it's up to an owner of this code whether this hack is acceptable. If you're adding a hack it should at least have a comment to explain what it's for. :-) > I would be happy to write the ELF-reading code, but I would rather not start > from scratch, since reusing good code is probably better than rolling my own. > Any pointers on which elf util I should be using? Reading the header is trivial enough that it's not worth adding a dependency. Have a look at /usr/include/elf.h. The first 16 bytes of an ELF file are the e_ident header from Elf32_Ehdr/Elf64_Ehdr. You could look at the field e_ident[EI_CLASS] to see whether the file is ELFCLASS32 or ELFCLASS64. You could start here: http://sco.com/developers/devspecs/gabi41.pdf -- See Section 4-4, "ELF Header". However, this is an old version of the ELF docs which predates ELFCLASS64.
https://codereview.chromium.org/1341873002/diff/20001/mojo/nacl/BUILD.gn File mojo/nacl/BUILD.gn (right): https://codereview.chromium.org/1341873002/diff/20001/mojo/nacl/BUILD.gn#newc... mojo/nacl/BUILD.gn:107: "//services/nacl:nacl_content_handler_nonsfi_x86", What if we're building Mojo on ARM (e.g. Android)? https://codereview.chromium.org/1341873002/diff/20001/services/nacl/BUILD.gn File services/nacl/BUILD.gn (right): https://codereview.chromium.org/1341873002/diff/20001/services/nacl/BUILD.gn#... services/nacl/BUILD.gn:30: # Nonsfi nacl can only be executed by a 32-bit process, so our Nit: "Non-SFI NaCl" instead of "Nonsfi nacl". https://codereview.chromium.org/1341873002/diff/20001/services/nacl/BUILD.gn#... services/nacl/BUILD.gn:51: if ("${root_out_dir}" != "${root_build_dir}") { Do you really need this? To check that you're not using a default toolchain you should use `current_toolchain != default_toolchain`. https://codereview.chromium.org/1341873002/diff/20001/services/nacl/BUILD.gn#... services/nacl/BUILD.gn:69: group("nacl_content_handler_nonsfi_x86") { I'd suggest naming the group `nacl_content_handler_nonsfi` since you'll likely use it to handle ARM (32-bit vs 64-bit) as well. https://codereview.chromium.org/1341873002/diff/20001/services/nacl/BUILD.gn#... services/nacl/BUILD.gn:75: [ ":nacl_content_handler_nonsfi(//build/toolchain/linux:clang_x86)" ] What if the default toolchain is glibc_x86? Then the `target_cpu == "x86"` but the nacl_content_handler_nonsfi binary will end up in `$root_out_dir` since you're not using the default toolchain and you still need to copy it to `$root_build_dir`. https://codereview.chromium.org/1341873002/diff/20001/shell/BUILD.gn File shell/BUILD.gn (right): https://codereview.chromium.org/1341873002/diff/20001/shell/BUILD.gn#newcode109 shell/BUILD.gn:109: group("mojo_shell_child_x86") { Ditto here. https://codereview.chromium.org/1341873002/diff/20001/shell/child_process_host.h File shell/child_process_host.h (right): https://codereview.chromium.org/1341873002/diff/20001/shell/child_process_hos... shell/child_process_host.h:43: void Start(bool require_32_bit); What if Mojo is built as 32-bit? Wouldn't it be better to define an enum for the target architecture (unless there is already one) and use it here? https://codereview.chromium.org/1341873002/diff/20001/shell/context.cc File shell/context.cc (right): https://codereview.chromium.org/1341873002/diff/20001/shell/context.cc#newcod... shell/context.cc:261: shell_path.DirName().AppendASCII("clang_x86/mojo_shell_child"); The path is an artifact of the build system (GN in this case) and I'd argue against hardcoding it (we don't want to have a different code path for each toolchain); instead I'd use a copy rule. https://codereview.chromium.org/1341873002/diff/20001/shell/context.h File shell/context.h (right): https://codereview.chromium.org/1341873002/diff/20001/shell/context.h#newcode92 shell/context.h:92: base::FilePath mojo_shell_child_path_32_bit_; Do you really need a separate variable for the 32-bit version? Couldn't you just append a suffix to `mojo_shell_child_path_` based on the target architecture?
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
https://codereview.chromium.org/1341873002/diff/20001/mojo/nacl/BUILD.gn File mojo/nacl/BUILD.gn (right): https://codereview.chromium.org/1341873002/diff/20001/mojo/nacl/BUILD.gn#newc... mojo/nacl/BUILD.gn:107: "//services/nacl:nacl_content_handler_nonsfi_x86", On 2015/09/14 19:13:51, Petr Hosek wrote: > What if we're building Mojo on ARM (e.g. Android)? Removed "x86" from suffix. Currently, ARM is not being considered when building Non-SFI NaCl -- I will include support (along with tests, hopefully) in a future CL. https://codereview.chromium.org/1341873002/diff/20001/services/nacl/BUILD.gn File services/nacl/BUILD.gn (right): https://codereview.chromium.org/1341873002/diff/20001/services/nacl/BUILD.gn#... services/nacl/BUILD.gn:30: # Nonsfi nacl can only be executed by a 32-bit process, so our On 2015/09/14 19:13:51, Petr Hosek wrote: > Nit: "Non-SFI NaCl" instead of "Nonsfi nacl". Done. https://codereview.chromium.org/1341873002/diff/20001/services/nacl/BUILD.gn#... services/nacl/BUILD.gn:49: # Copy to the root build directory so our content handler prefixed line On 2015/09/14 18:38:58, Mark Seaborn wrote: > How about: "so that the '#!' prefix line for invoking our content handler..." > (easier to parse) Done. https://codereview.chromium.org/1341873002/diff/20001/services/nacl/BUILD.gn#... services/nacl/BUILD.gn:51: if ("${root_out_dir}" != "${root_build_dir}") { On 2015/09/14 19:13:51, Petr Hosek wrote: > Do you really need this? To check that you're not using a default toolchain you > should use `current_toolchain != default_toolchain`. I updated things to use a different naming convention. This conditional is no longer here. Since you suggested renaming the group to "nacl_content_handler_nonsfi", I renamed the native application to "nacl_content_handler_nonsfi_x86" (since it is always built as 32 bit). The copy rule is now always used, on both 32 and 64 bit systems, to copy ".../nacl_content_handler_nonsfi_x86.mojo" to ".../nacl_content_handler_nonsfi.mojo". https://codereview.chromium.org/1341873002/diff/20001/services/nacl/BUILD.gn#... services/nacl/BUILD.gn:69: group("nacl_content_handler_nonsfi_x86") { On 2015/09/14 19:13:51, Petr Hosek wrote: > I'd suggest naming the group `nacl_content_handler_nonsfi` since you'll likely > use it to handle ARM (32-bit vs 64-bit) as well. Done. https://codereview.chromium.org/1341873002/diff/20001/services/nacl/BUILD.gn#... services/nacl/BUILD.gn:72: deps += [ ":nacl_content_handler_nonsfi_copy(//build/toolchain/linux:clang_x86)" ] On 2015/09/14 18:38:58, Mark Seaborn wrote: > See other comment -- comment to warn about assuming clang here? Done. https://codereview.chromium.org/1341873002/diff/20001/services/nacl/BUILD.gn#... services/nacl/BUILD.gn:75: [ ":nacl_content_handler_nonsfi(//build/toolchain/linux:clang_x86)" ] On 2015/09/14 19:13:51, Petr Hosek wrote: > What if the default toolchain is glibc_x86? Then the `target_cpu == "x86"` but > the nacl_content_handler_nonsfi binary will end up in `$root_out_dir` since > you're not using the default toolchain and you still need to copy it to > `$root_build_dir`. Although this is untested, the new dependency on the copy rule (regardless of x86 vs x64) should cause the binary to be copied to the root_out_dir regardless of default toolchain. I think. https://codereview.chromium.org/1341873002/diff/20001/shell/BUILD.gn File shell/BUILD.gn (right): https://codereview.chromium.org/1341873002/diff/20001/shell/BUILD.gn#newcode109 shell/BUILD.gn:109: group("mojo_shell_child_x86") { On 2015/09/14 19:13:51, Petr Hosek wrote: > Ditto here. Naming "mojo_shell_child_32_bit", since "mojo_shell_child" is already taken and may imply the 64 bit version. https://codereview.chromium.org/1341873002/diff/20001/shell/BUILD.gn#newcode114 shell/BUILD.gn:114: ":mojo_shell_child(//build/toolchain/linux:clang_x86)", On 2015/09/14 18:38:58, Mark Seaborn wrote: > Nit: Shouldn't this come with the same warning as you added in > https://codereview.chromium.org/1327913004 Done. https://codereview.chromium.org/1341873002/diff/20001/shell/child_process_host.h File shell/child_process_host.h (right): https://codereview.chromium.org/1341873002/diff/20001/shell/child_process_hos... shell/child_process_host.h:43: void Start(bool require_32_bit); On 2015/09/14 19:13:52, Petr Hosek wrote: > What if Mojo is built as 32-bit? Wouldn't it be better to define an enum for the > target architecture (unless there is already one) and use it here? An enum may eventually be useful, but at the moment, the only functionality is a request to use a pre-built 32-bit shell for the nonsfi content handler. This value should be independent of the built architecture -- though it might be "unnecessary" if the rest of mojo is built as 32 bit, it will still work. https://codereview.chromium.org/1341873002/diff/20001/shell/child_process_hos... File shell/child_process_host_unittest.cc (right): https://codereview.chromium.org/1341873002/diff/20001/shell/child_process_hos... shell/child_process_host_unittest.cc:53: child_process_host.Start(require_32_bit); On 2015/09/14 18:38:58, Mark Seaborn wrote: > Alternative style is: > child_process_host.Start(false /* require_32_bit */); Done. https://codereview.chromium.org/1341873002/diff/20001/shell/context.cc File shell/context.cc (right): https://codereview.chromium.org/1341873002/diff/20001/shell/context.cc#newcod... shell/context.cc:261: shell_path.DirName().AppendASCII("clang_x86/mojo_shell_child"); On 2015/09/14 19:13:52, Petr Hosek wrote: > The path is an artifact of the build system (GN in this case) and I'd argue > against hardcoding it (we don't want to have a different code path for each > toolchain); instead I'd use a copy rule. Using a copy rule in the gn files -- now there are two binaries: "mojo_shell_child" and "mojo_shell_child_32". The 32-bit version does not indicate which build system it originated from now. https://codereview.chromium.org/1341873002/diff/20001/shell/context.h File shell/context.h (right): https://codereview.chromium.org/1341873002/diff/20001/shell/context.h#newcode92 shell/context.h:92: base::FilePath mojo_shell_child_path_32_bit_; On 2015/09/14 19:13:52, Petr Hosek wrote: > Do you really need a separate variable for the 32-bit version? Couldn't you just > append a suffix to `mojo_shell_child_path_` based on the target architecture? Done. https://codereview.chromium.org/1341873002/diff/20001/shell/out_of_process_na... File shell/out_of_process_native_runner.cc (right): https://codereview.chromium.org/1341873002/diff/20001/shell/out_of_process_na... shell/out_of_process_native_runner.cc:45: require_32_bit = true; On 2015/09/14 18:38:59, Mark Seaborn wrote: > On 2015/09/14 18:10:34, smklein1 wrote: > > I am fully aware this is kind of hacky -- mojo people suggested reading ELF > > headers to identify if 32-bit child shell is required. That being said, this > > version does currently work. > > I think it's up to an owner of this code whether this hack is acceptable. > > If you're adding a hack it should at least have a comment to explain what it's > for. :-) > > > I would be happy to write the ELF-reading code, but I would rather not start > > from scratch, since reusing good code is probably better than rolling my own. > > Any pointers on which elf util I should be using? > > Reading the header is trivial enough that it's not worth adding a dependency. > > Have a look at /usr/include/elf.h. > > The first 16 bytes of an ELF file are the e_ident header from > Elf32_Ehdr/Elf64_Ehdr. You could look at the field e_ident[EI_CLASS] to see > whether the file is ELFCLASS32 or ELFCLASS64. > > You could start here: > http://sco.com/developers/devspecs/gabi41.pdf > -- See Section 4-4, "ELF Header". > However, this is an old version of the ELF docs which predates ELFCLASS64. Modified to check ELF header of content handler. Alternatively, I could be checking the ELF header of the content itself, but this is a bit trickier (and not necessarily and ELF file). My error handling isn't great at this point, but I'm not sure what else can be done (it's a void function, but is an assertion the best I can do in case of error?). Also, should I be checking for the magic ELF header here?
https://codereview.chromium.org/1341873002/diff/20001/shell/out_of_process_na... File shell/out_of_process_native_runner.cc (right): https://codereview.chromium.org/1341873002/diff/20001/shell/out_of_process_na... shell/out_of_process_native_runner.cc:45: require_32_bit = true; On 2015/09/15 18:37:50, smklein1 wrote: > Also, should I be checking for the magic ELF header here? Yes, checking for "\x7fELF" would be good, to ensure that when you read EI_CLASS it's meaningful. https://codereview.chromium.org/1341873002/diff/80001/shell/child_process_hos... File shell/child_process_host.cc (right): https://codereview.chromium.org/1341873002/diff/80001/shell/child_process_hos... shell/child_process_host.cc:52: context_->mojo_shell_child_path().InsertBeforeExtensionASCII("_32"); It seems like this should be inside an "#if defined(ARCH_CPU_X86_64)". Adding this suffix is only wanted when mojo_shell is built for x86-64. https://codereview.chromium.org/1341873002/diff/80001/shell/out_of_process_na... File shell/out_of_process_native_runner.cc (right): https://codereview.chromium.org/1341873002/diff/80001/shell/out_of_process_na... shell/out_of_process_native_runner.cc:14: #include "elf.h" Should be <elf.h> since this is a system header https://codereview.chromium.org/1341873002/diff/80001/shell/out_of_process_na... shell/out_of_process_native_runner.cc:46: char data[EI_NIDENT]; This new code could also be conditionalised on "#if defined(ARCH_CPU_X86_64)". https://codereview.chromium.org/1341873002/diff/80001/shell/out_of_process_na... shell/out_of_process_native_runner.cc:47: if (EI_NIDENT != ReadFile(app_path, data, EI_NIDENT)) How about ReadFile(app_path, data, sizeof(data)), to better ensure the size is in sync? https://codereview.chromium.org/1341873002/diff/80001/shell/out_of_process_na... shell/out_of_process_native_runner.cc:48: assert(false); Chromium style is to avoid assert() and use CHECK() instead. However, if the file is too small, I'm not sure you want to abort here. Maybe it would be better to assume require_32_bit = false and just let the process startup fail later. https://codereview.chromium.org/1341873002/diff/80001/shell/out_of_process_na... shell/out_of_process_native_runner.cc:50: require_32_bit = true; How about splitting this new code into a Require32Bit(filename) function?
https://codereview.chromium.org/1341873002/diff/80001/shell/child_process_hos... File shell/child_process_host.cc (right): https://codereview.chromium.org/1341873002/diff/80001/shell/child_process_hos... shell/child_process_host.cc:52: context_->mojo_shell_child_path().InsertBeforeExtensionASCII("_32"); On 2015/09/15 19:42:58, Mark Seaborn wrote: > It seems like this should be inside an "#if defined(ARCH_CPU_X86_64)". Adding > this suffix is only wanted when mojo_shell is built for x86-64. Using ARCH_CPU_64_BITS to work with ARM too. https://codereview.chromium.org/1341873002/diff/80001/shell/out_of_process_na... File shell/out_of_process_native_runner.cc (right): https://codereview.chromium.org/1341873002/diff/80001/shell/out_of_process_na... shell/out_of_process_native_runner.cc:14: #include "elf.h" On 2015/09/15 19:42:58, Mark Seaborn wrote: > Should be <elf.h> since this is a system header Done. https://codereview.chromium.org/1341873002/diff/80001/shell/out_of_process_na... shell/out_of_process_native_runner.cc:46: char data[EI_NIDENT]; On 2015/09/15 19:42:58, Mark Seaborn wrote: > This new code could also be conditionalised on "#if defined(ARCH_CPU_X86_64)". It seems a bit misleading to use a conditional here and return a unique value for 32 bit architectures just to ignore it later. I will include the shortcut for 32 bit systems regardless. https://codereview.chromium.org/1341873002/diff/80001/shell/out_of_process_na... shell/out_of_process_native_runner.cc:47: if (EI_NIDENT != ReadFile(app_path, data, EI_NIDENT)) On 2015/09/15 19:42:58, Mark Seaborn wrote: > How about ReadFile(app_path, data, sizeof(data)), to better ensure the size is > in sync? Done. https://codereview.chromium.org/1341873002/diff/80001/shell/out_of_process_na... shell/out_of_process_native_runner.cc:48: assert(false); On 2015/09/15 19:42:58, Mark Seaborn wrote: > Chromium style is to avoid assert() and use CHECK() instead. > > However, if the file is too small, I'm not sure you want to abort here. Maybe > it would be better to assume require_32_bit = false and just let the process > startup fail later. This seems misleading to me (I am not a fan of silent errors), but I have updated it to this version. https://codereview.chromium.org/1341873002/diff/80001/shell/out_of_process_na... shell/out_of_process_native_runner.cc:50: require_32_bit = true; On 2015/09/15 19:42:58, Mark Seaborn wrote: > How about splitting this new code into a Require32Bit(filename) function? Done.
LGTM https://codereview.chromium.org/1341873002/diff/80001/shell/out_of_process_na... File shell/out_of_process_native_runner.cc (right): https://codereview.chromium.org/1341873002/diff/80001/shell/out_of_process_na... shell/out_of_process_native_runner.cc:48: assert(false); On 2015/09/15 21:35:40, smklein1 wrote: > On 2015/09/15 19:42:58, Mark Seaborn wrote: > > Chromium style is to avoid assert() and use CHECK() instead. > > > > However, if the file is too small, I'm not sure you want to abort here. Maybe > > it would be better to assume require_32_bit = false and just let the process > > startup fail later. > > This seems misleading to me (I am not a fan of silent errors), but I have > updated it to this version. Maybe: size_t bytes_read = ReadFile(...); DCHECK_EQ(bytes_read, sizeof(data)); if (bytes_read != sizeof(data)) return false; https://codereview.chromium.org/1341873002/diff/100001/shell/out_of_process_n... File shell/out_of_process_native_runner.cc (right): https://codereview.chromium.org/1341873002/diff/100001/shell/out_of_process_n... shell/out_of_process_native_runner.cc:36: bool Require32Bit(const base::FilePath& app_path) { This could go in an anon namespace. https://codereview.chromium.org/1341873002/diff/100001/shell/out_of_process_n... shell/out_of_process_native_runner.cc:37: #if defined(ARCH_CPU_32_BITS) You could use "if (sizeof(void *) == 4)". This way, both branches get compile-tested. if() is often preferable to #if: http://lackingrhoticity.blogspot.com/2015/01/conditionalising-c-ifdef-vs-if.html If you do use ARCH_*, though, you should make sure to #include "build/build_config.h". Same in other file too. https://codereview.chromium.org/1341873002/diff/100001/shell/out_of_process_n... shell/out_of_process_native_runner.cc:43: if (EI_NIDENT != ReadFile(app_path, data, sizeof(data))) Nit: Also "EI_NIDENT" -> "sizeof(data)" (as before) https://codereview.chromium.org/1341873002/diff/100001/shell/out_of_process_n... shell/out_of_process_native_runner.cc:49: if (data[EI_CLASS] == ELFCLASS32) Can simplify to: return data[EI_CLASS] == ELFCLASS32;
https://codereview.chromium.org/1341873002/diff/100001/services/nacl/BUILD.gn File services/nacl/BUILD.gn (right): https://codereview.chromium.org/1341873002/diff/100001/services/nacl/BUILD.gn... services/nacl/BUILD.gn:32: mojo_native_application("nacl_content_handler_nonsfi_x86") { I'd probably name it as "nacl_content_handler_nonsfi_mojo"; then you won't have to rename this target in the future (i.e. when you add support for ARM). https://codereview.chromium.org/1341873002/diff/100001/services/nacl/BUILD.gn... services/nacl/BUILD.gn:53: "${root_out_dir}/nacl_content_handler_nonsfi_x86.mojo", You can use "nacl_content_handler_nonsfi_${current_cpu}.mojo" for the filename.
https://codereview.chromium.org/1341873002/diff/100001/services/nacl/BUILD.gn File services/nacl/BUILD.gn (right): https://codereview.chromium.org/1341873002/diff/100001/services/nacl/BUILD.gn... services/nacl/BUILD.gn:32: mojo_native_application("nacl_content_handler_nonsfi_x86") { On 2015/09/15 22:40:27, Petr Hosek wrote: > I'd probably name it as "nacl_content_handler_nonsfi_mojo"; then you won't have > to rename this target in the future (i.e. when you add support for ARM). Calling it "nacl_content_handler_nonsfi_32_bit" instead. The mojo suffix doesn't mean much, and though it is not x86-specific, it IS 32-bit specific. https://codereview.chromium.org/1341873002/diff/100001/services/nacl/BUILD.gn... services/nacl/BUILD.gn:53: "${root_out_dir}/nacl_content_handler_nonsfi_x86.mojo", On 2015/09/15 22:40:27, Petr Hosek wrote: > You can use "nacl_content_handler_nonsfi_${current_cpu}.mojo" for the filename. With the name change above, this is not necessary. https://codereview.chromium.org/1341873002/diff/100001/shell/out_of_process_n... File shell/out_of_process_native_runner.cc (right): https://codereview.chromium.org/1341873002/diff/100001/shell/out_of_process_n... shell/out_of_process_native_runner.cc:36: bool Require32Bit(const base::FilePath& app_path) { On 2015/09/15 22:25:43, Mark Seaborn wrote: > This could go in an anon namespace. Done. https://codereview.chromium.org/1341873002/diff/100001/shell/out_of_process_n... shell/out_of_process_native_runner.cc:37: #if defined(ARCH_CPU_32_BITS) On 2015/09/15 22:25:43, Mark Seaborn wrote: > You could use "if (sizeof(void *) == 4)". This way, both branches get > compile-tested. > > if() is often preferable to #if: > http://lackingrhoticity.blogspot.com/2015/01/conditionalising-c-ifdef-vs-if.html > > If you do use ARCH_*, though, you should make sure to #include > "build/build_config.h". > > Same in other file too. Using "sizeof(void *) == 4", along with a comment. https://codereview.chromium.org/1341873002/diff/100001/shell/out_of_process_n... shell/out_of_process_native_runner.cc:43: if (EI_NIDENT != ReadFile(app_path, data, sizeof(data))) On 2015/09/15 22:25:43, Mark Seaborn wrote: > Nit: Also "EI_NIDENT" -> "sizeof(data)" (as before) Done. https://codereview.chromium.org/1341873002/diff/100001/shell/out_of_process_n... shell/out_of_process_native_runner.cc:49: if (data[EI_CLASS] == ELFCLASS32) On 2015/09/15 22:25:43, Mark Seaborn wrote: > Can simplify to: > return data[EI_CLASS] == ELFCLASS32; Done.
Message was sent while issue was closed.
Committed patchset #4 (id:120001) manually as a7ffbe0f340df9adc1b3f948b8a5c83593308701 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
