Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
100373005:
Initial implementation of Bare Metal Mode for NaCl. (Closed)
|
Created:
7 years ago by hidehiko Modified:
6 years, 10 months ago CC:
chromium-reviews, Takashi Toyoshima Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionInitial implementation of Bare Metal Mode for NaCl.
This CL introduces bare_metal_main, which contains the implementation for
Bare Metal Mode replacing nacl_secure_service, and ELF related utility.
Some codes are extracted from native_client/src/trusted/service_runtime
in order to separate Bare Metal related stuff completely from service runtime.
Some parts should be refactored and merged eventually, probably after we
convince ourselves that the Bare Metal is stable enough.
BUG=https://code.google.com/p/nativeclient/issues/detail?id=3734
TEST=Tried to load the elf binary with this loader, and made sure that the entry point is reached.
TBR=jln@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243060
Patch Set 1 : #
Total comments: 18
Patch Set 2 : #
Total comments: 12
Patch Set 3 : #
Total comments: 20
Patch Set 4 : Rebase #Patch Set 5 : #
Total comments: 33
Patch Set 6 : #
Total comments: 18
Patch Set 7 : Rebase #Patch Set 8 : #Patch Set 9 : Rebase #
Messages
Total messages: 19 (0 generated)
This CL is the result of movement from NaCl source tree to Chrome source tree, as Mark advised me on crrev.com/101643002. Also, I dropped the support except on linux, as Shinichiro advised me. Could you take another look? Thanks in advance, - hidehiko
Mostly high-level comments to start with... https://codereview.chromium.org/100373005/diff/20001/components/nacl.gyp File components/nacl.gyp (right): https://codereview.chromium.org/100373005/diff/20001/components/nacl.gyp#newc... components/nacl.gyp:8: 'enable_bare_metal_nacl': 0, If we're going to test Bare Metal Mode on the Chromium buildbots (which should happen in a later CL but not the first CL), then it can't require a build-time flag to enable it. It will require a run time flag instead. So, please remove this line. We'll at least be compile-testing on the Chromium bots in the first CL. :-) This means you should also run the CL through the trybots. https://codereview.chromium.org/100373005/diff/20001/components/nacl.gyp#newc... components/nacl.gyp:216: ['enable_bare_metal_nacl == 1', { So this should just be 'os_linux==1' instead. https://codereview.chromium.org/100373005/diff/20001/components/nacl.gyp#newc... components/nacl.gyp:220: 'nacl/loader/bare_metal/bare_metal_error_code.h', On naming: I was thinking about calling this "nonsfi" rather than "bare_metal" in the code. The term "bare metal" isn't really accurate because it means "without an operating system". It's OK as a project name, but for the codebase I think it would be more descriptive to use the term "non-SFI". What do you think? https://codereview.chromium.org/100373005/diff/20001/components/nacl/loader/b... File components/nacl/loader/bare_metal/bare_metal_main.c (right): https://codereview.chromium.org/100373005/diff/20001/components/nacl/loader/b... components/nacl/loader/bare_metal/bare_metal_main.c:1: /* Can you make these .cc files rather than .c files? This code should follow the Chromium style as much as possible, and virtually all the Chromium code is in C++ rather than C. This will allow reusing base/ where appropriate, and calling into the PPAPI proxy when we add that in a later CL. https://codereview.chromium.org/100373005/diff/20001/components/nacl/loader/b... components/nacl/loader/bare_metal/bare_metal_main.c:252: void NaClBareMetalMainStart(NaClHandle imc_bootstrap_handle) { Can you hook this up so that it's called by nacl_listener.cc, please? As I was saying in the nacl-eng thread, I would like for this to work end-to-end in this initial CL, at least to the extent of being able to ELF-load and call an executable that's loaded by Chromium from a manifest file. (The executable won't be able to call any IRT interfaces -- that can come later.) In this initial CL, we can use a command line flag to switch on Bare Metal Mode. You'll probably need to add an entry to components/nacl/common/nacl_switches.h. So it should be possible to test this manually by: * building a simple executable * creating an NMF file that refers to the executable * loading the NMF in the browser You can build a simple standalone, relocatable executable for manual testing like this: $ cat hellow_direct_syscall.c static int my_errno; #define SYS_ERRNO my_errno #include "linux_syscall_support.h" void _start() { static const char string[] = "Hello world!\n"; sys_write(1, string, sizeof(string) - 1); sys_exit_group(0); } $ gcc hellow_direct_syscall.c -Ithird_party/linux-syscall-support/lss/ -o hellow_direct_syscall -nostdlib -shared -fPIC $ ./hellow_direct_syscall Hello world! (Note that "-shared -fPIC" makes a relocatable, ET_DYN executable.) https://codereview.chromium.org/100373005/diff/20001/components/nacl/loader/b... File components/nacl/loader/bare_metal/bare_metal_main.h (right): https://codereview.chromium.org/100373005/diff/20001/components/nacl/loader/b... components/nacl/loader/bare_metal/bare_metal_main.h:2: * Copyright 2013 The Native Client Authors. All rights reserved. Nit: This should be "The Chromium Authors", as with all files in the Chromium tree. https://codereview.chromium.org/100373005/diff/20001/components/nacl/loader/b... File components/nacl/loader/bare_metal/elf_util.c (right): https://codereview.chromium.org/100373005/diff/20001/components/nacl/loader/b... components/nacl/loader/bare_metal/elf_util.c:126: if (ET_EXEC != hdr->e_type) { This should require ET_DYN -- see my other comment below... https://codereview.chromium.org/100373005/diff/20001/components/nacl/loader/b... components/nacl/loader/bare_metal/elf_util.c:451: mmap_flags |= MAP_FIXED; For the initial versions of Bare Metal Mode, this is not really going to work. We can't handle ET_EXEC (non-relocatable) executables because we won't have reserved any specific ranges of address space for them. If you pass MAP_FIXED here, this will clobber memory that might already be allocated. I think for the initial versions we should only accept ET_DYN (relocatable) executables, so you can drop MAP_FIXED. https://codereview.chromium.org/100373005/diff/20001/components/nacl/loader/b... File components/nacl/loader/bare_metal/elf_util.h (right): https://codereview.chromium.org/100373005/diff/20001/components/nacl/loader/b... components/nacl/loader/bare_metal/elf_util.h:15: * TODO(hidehiko): Unify with it after Bare Metal Mode gets stable. I should say now that I am not expecting such a unification to happen. :-) That's partly because NaCl and Bare Metal Mode will have subtly different requirements for how to ELF-load (e.g. whether to support ET_DYN, and what constraints to place on the PT_LOAD segments). But also, ELF loading is simple enough that it's just not worth having an awkward-to-maintain cross-repo dependency just for some trivial ELF header reading code. So I think it's better just to have a clean and minimal version in the Chromium repo than try to merge them.
Thank you for your review. PTAL? https://codereview.chromium.org/100373005/diff/20001/components/nacl.gyp File components/nacl.gyp (right): https://codereview.chromium.org/100373005/diff/20001/components/nacl.gyp#newc... components/nacl.gyp:8: 'enable_bare_metal_nacl': 0, On 2013/12/06 03:21:16, Mark Seaborn wrote: > If we're going to test Bare Metal Mode on the Chromium buildbots (which should > happen in a later CL but not the first CL), then it can't require a build-time > flag to enable it. It will require a run time flag instead. > > So, please remove this line. We'll at least be compile-testing on the Chromium > bots in the first CL. :-) This means you should also run the CL through the > trybots. Done. https://codereview.chromium.org/100373005/diff/20001/components/nacl.gyp#newc... components/nacl.gyp:216: ['enable_bare_metal_nacl == 1', { On 2013/12/06 03:21:16, Mark Seaborn wrote: > So this should just be 'os_linux==1' instead. Done. Note that OS == linux is checked by enclosing condition. We also need to restrict to ia32 arch for bare metal, now. https://codereview.chromium.org/100373005/diff/20001/components/nacl.gyp#newc... components/nacl.gyp:220: 'nacl/loader/bare_metal/bare_metal_error_code.h', On 2013/12/06 03:21:16, Mark Seaborn wrote: > On naming: I was thinking about calling this "nonsfi" rather than "bare_metal" > in the code. > > The term "bare metal" isn't really accurate because it means "without an > operating system". It's OK as a project name, but for the codebase I think it > would be more descriptive to use the term "non-SFI". What do you think? Indeed. Done. Renamed all Bare Metal to Non SFI. https://codereview.chromium.org/100373005/diff/20001/components/nacl/loader/b... File components/nacl/loader/bare_metal/bare_metal_main.c (right): https://codereview.chromium.org/100373005/diff/20001/components/nacl/loader/b... components/nacl/loader/bare_metal/bare_metal_main.c:1: /* On 2013/12/06 03:21:16, Mark Seaborn wrote: > Can you make these .cc files rather than .c files? This code should follow the > Chromium style as much as possible, and virtually all the Chromium code is in > C++ rather than C. > > This will allow reusing base/ where appropriate, and calling into the PPAPI > proxy when we add that in a later CL. I see. I fully rewrote the code with C++. Along the change, introduced namespace nonsfi. https://codereview.chromium.org/100373005/diff/20001/components/nacl/loader/b... components/nacl/loader/bare_metal/bare_metal_main.c:252: void NaClBareMetalMainStart(NaClHandle imc_bootstrap_handle) { On 2013/12/06 03:21:16, Mark Seaborn wrote: > Can you hook this up so that it's called by nacl_listener.cc, please? > > As I was saying in the nacl-eng thread, I would like for this to work end-to-end > in this initial CL, at least to the extent of being able to ELF-load and call an > executable that's loaded by Chromium from a manifest file. (The executable > won't be able to call any IRT interfaces -- that can come later.) > > In this initial CL, we can use a command line flag to switch on Bare Metal Mode. > You'll probably need to add an entry to components/nacl/common/nacl_switches.h. > > So it should be possible to test this manually by: > * building a simple executable > * creating an NMF file that refers to the executable > * loading the NMF in the browser > > You can build a simple standalone, relocatable executable for manual testing > like this: > > $ cat hellow_direct_syscall.c > > static int my_errno; > > #define SYS_ERRNO my_errno > #include "linux_syscall_support.h" > > void _start() { > static const char string[] = "Hello world!\n"; > sys_write(1, string, sizeof(string) - 1); > sys_exit_group(0); > } > > $ gcc hellow_direct_syscall.c -Ithird_party/linux-syscall-support/lss/ -o > hellow_direct_syscall -nostdlib -shared -fPIC > $ ./hellow_direct_syscall > Hello world! > > (Note that "-shared -fPIC" makes a relocatable, ET_DYN executable.) Done. Note that the flag is added to content_switched, as the process running nacl_listener is forked from Zygote process, so we need to propagate the flag to Zygote process, too. I'm not very sure it is the appropriate way or not, to be honest. If there is better way, please let me know. https://codereview.chromium.org/100373005/diff/20001/components/nacl/loader/b... File components/nacl/loader/bare_metal/bare_metal_main.h (right): https://codereview.chromium.org/100373005/diff/20001/components/nacl/loader/b... components/nacl/loader/bare_metal/bare_metal_main.h:2: * Copyright 2013 The Native Client Authors. All rights reserved. On 2013/12/06 03:21:16, Mark Seaborn wrote: > Nit: This should be "The Chromium Authors", as with all files in the Chromium > tree. Oops. Done. https://codereview.chromium.org/100373005/diff/20001/components/nacl/loader/b... File components/nacl/loader/bare_metal/elf_util.c (right): https://codereview.chromium.org/100373005/diff/20001/components/nacl/loader/b... components/nacl/loader/bare_metal/elf_util.c:126: if (ET_EXEC != hdr->e_type) { On 2013/12/06 03:21:16, Mark Seaborn wrote: > This should require ET_DYN -- see my other comment below... Done. https://codereview.chromium.org/100373005/diff/20001/components/nacl/loader/b... components/nacl/loader/bare_metal/elf_util.c:451: mmap_flags |= MAP_FIXED; On 2013/12/06 03:21:16, Mark Seaborn wrote: > For the initial versions of Bare Metal Mode, this is not really going to work. > We can't handle ET_EXEC (non-relocatable) executables because we won't have > reserved any specific ranges of address space for them. If you pass MAP_FIXED > here, this will clobber memory that might already be allocated. > > I think for the initial versions we should only accept ET_DYN (relocatable) > executables, so you can drop MAP_FIXED. Done. https://codereview.chromium.org/100373005/diff/20001/components/nacl/loader/b... File components/nacl/loader/bare_metal/elf_util.h (right): https://codereview.chromium.org/100373005/diff/20001/components/nacl/loader/b... components/nacl/loader/bare_metal/elf_util.h:15: * TODO(hidehiko): Unify with it after Bare Metal Mode gets stable. On 2013/12/06 03:21:16, Mark Seaborn wrote: > I should say now that I am not expecting such a unification to happen. :-) > That's partly because NaCl and Bare Metal Mode will have subtly different > requirements for how to ELF-load (e.g. whether to support ET_DYN, and what > constraints to place on the PT_LOAD segments). But also, ELF loading is simple > enough that it's just not worth having an awkward-to-maintain cross-repo > dependency just for some trivial ELF header reading code. So I think it's > better just to have a clean and minimal version in the Chromium repo than try to > merge them. I see. Done. Let's revisit here, when we move the code to nacl's repository after bare metal gets stabled enough.
Thanks for making the changes quickly. https://codereview.chromium.org/100373005/diff/40001/components/nacl.gyp File components/nacl.gyp (right): https://codereview.chromium.org/100373005/diff/40001/components/nacl.gyp#newc... components/nacl.gyp:215: ['target_arch == "ia32"', { Can we enable this for all architectures, please? For maintainability, we want to have this code tested on the Chromium trybots, which primarily test x86-64. (There is a "linux_rel_precise32" trybot, but the default trybot runs and the Commit Queue only cover x86-64.) Initially we can just use the native pointer size, so 64-bit on x86-64. We might to revisit that later. e.g. If we use the PNaCl toolchain to translate pexes to BMM executables for testing on the bots, this will probably need to use 32-bit pointers. But let's cross that bridge when we come to it. For now, this just means we should use uintptr_t in the ELF loader rather than uint32_t. https://codereview.chromium.org/100373005/diff/40001/components/nacl/loader/n... File components/nacl/loader/nonsfi/elf_util.cc (right): https://codereview.chromium.org/100373005/diff/40001/components/nacl/loader/n... components/nacl/loader/nonsfi/elf_util.cc:4: Nit: "elf_loader.cc" would be a more descriptive name for this file https://codereview.chromium.org/100373005/diff/40001/components/nacl/loader/n... components/nacl/loader/nonsfi/elf_util.cc:118: Elf32_Addr GetPageStart(Elf32_Addr addr) { So that we can test in x86-64 builds, all of the Elf32_Addrs can be uintptr_t instead... https://codereview.chromium.org/100373005/diff/40001/components/nacl/loader/n... components/nacl/loader/nonsfi/elf_util.cc:150: Elf32_Addr GetLoadSize(const Elf32_Phdr* phdrs, int phnum) { So that this works on x86-64, you can use "ElfW(Phdr)" instead of "Elf32_Phdr", if you #include <link.h> and <elf.h> instead of "native_client/src/include/elf.h". (I think that's a small dependency on glibc, but that's OK for now.) https://codereview.chromium.org/100373005/diff/40001/content/browser/zygote_h... File content/browser/zygote_host/zygote_host_impl_linux.cc (right): https://codereview.chromium.org/100373005/diff/40001/content/browser/zygote_h... content/browser/zygote_host/zygote_host_impl_linux.cc:116: switches::kDisableNaClSfi, The content/ directory is not supposed to have references to NaCl in it. Given that constraint, I'm not sure there is a way to forward this through to nacl_helper as a command line argument. Another way to forward this option through is to add it to 'struct NaClStartParams' (from components/nacl/common/nacl_types.h), which nacl_listener.cc receives. nacl_process_host.cc can set the new field from the command line option. (In a later CL, nacl_process_host.cc can receive this option from the plugin, so using NaClStartParams is the right direction to go in.) https://codereview.chromium.org/100373005/diff/40001/content/public/common/co... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/100373005/diff/40001/content/public/common/co... content/public/common/content_switches.cc:265: const char kDisableNaClSfi[] = "disable-nacl-sfi"; Can we call this something like kEnableNaClNonSfiMode = "enable-nacl-nonsfi-mode? This is more about enabling a feature than disabling something. (Also, it's already possible to disable the SFI protections by disabling the validator via an env var, and I wouldn't want this option to be confused with that.)
Thank you for your review. PTAL. https://codereview.chromium.org/100373005/diff/40001/components/nacl.gyp File components/nacl.gyp (right): https://codereview.chromium.org/100373005/diff/40001/components/nacl.gyp#newc... components/nacl.gyp:215: ['target_arch == "ia32"', { On 2013/12/06 21:42:37, Mark Seaborn wrote: > Can we enable this for all architectures, please? For maintainability, we want > to have this code tested on the Chromium trybots, which primarily test x86-64. > (There is a "linux_rel_precise32" trybot, but the default trybot runs and the > Commit Queue only cover x86-64.) > > Initially we can just use the native pointer size, so 64-bit on x86-64. We > might to revisit that later. e.g. If we use the PNaCl toolchain to translate > pexes to BMM executables for testing on the bots, this will probably need to use > 32-bit pointers. But let's cross that bridge when we come to it. For now, this > just means we should use uintptr_t in the ELF loader rather than uint32_t. I see. Done. https://codereview.chromium.org/100373005/diff/40001/components/nacl/loader/n... File components/nacl/loader/nonsfi/elf_util.cc (right): https://codereview.chromium.org/100373005/diff/40001/components/nacl/loader/n... components/nacl/loader/nonsfi/elf_util.cc:4: On 2013/12/06 21:42:37, Mark Seaborn wrote: > Nit: "elf_loader.cc" would be a more descriptive name for this file Done. https://codereview.chromium.org/100373005/diff/40001/components/nacl/loader/n... components/nacl/loader/nonsfi/elf_util.cc:118: Elf32_Addr GetPageStart(Elf32_Addr addr) { On 2013/12/06 21:42:37, Mark Seaborn wrote: > So that we can test in x86-64 builds, all of the Elf32_Addrs can be uintptr_t > instead... Replaced by ElfW(Addr) by following the manner you commented below. https://codereview.chromium.org/100373005/diff/40001/components/nacl/loader/n... components/nacl/loader/nonsfi/elf_util.cc:150: Elf32_Addr GetLoadSize(const Elf32_Phdr* phdrs, int phnum) { On 2013/12/06 21:42:37, Mark Seaborn wrote: > So that this works on x86-64, you can use "ElfW(Phdr)" instead of "Elf32_Phdr", > if you #include <link.h> and <elf.h> instead of > "native_client/src/include/elf.h". (I think that's a small dependency on glibc, > but that's OK for now.) Thank you for your navigation. Done. https://codereview.chromium.org/100373005/diff/40001/content/browser/zygote_h... File content/browser/zygote_host/zygote_host_impl_linux.cc (right): https://codereview.chromium.org/100373005/diff/40001/content/browser/zygote_h... content/browser/zygote_host/zygote_host_impl_linux.cc:116: switches::kDisableNaClSfi, On 2013/12/06 21:42:37, Mark Seaborn wrote: > The content/ directory is not supposed to have references to NaCl in it. Given > that constraint, I'm not sure there is a way to forward this through to > nacl_helper as a command line argument. > > Another way to forward this option through is to add it to 'struct > NaClStartParams' (from components/nacl/common/nacl_types.h), which > nacl_listener.cc receives. nacl_process_host.cc can set the new field from the > command line option. > > (In a later CL, nacl_process_host.cc can receive this option from the plugin, so > using NaClStartParams is the right direction to go in.) Great to know. Done. https://codereview.chromium.org/100373005/diff/40001/content/public/common/co... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/100373005/diff/40001/content/public/common/co... content/public/common/content_switches.cc:265: const char kDisableNaClSfi[] = "disable-nacl-sfi"; On 2013/12/06 21:42:37, Mark Seaborn wrote: > Can we call this something like kEnableNaClNonSfiMode = > "enable-nacl-nonsfi-mode? This is more about enabling a feature than disabling > something. (Also, it's already possible to disable the SFI protections by > disabling the validator via an env var, and I wouldn't want this option to be > confused with that.) Done.
I tried testing this on x86-64 using the method I described in: https://codereview.chromium.org/100373005/diff/20001/components/nacl/loader/b... Specifically: $ cd native_client $ cat hellow_bmm.html Hello world NaCl example. Writes to stdout, so nothing appears here. <embed type="application/x-nacl" src="hellow_bmm.nmf" width="0" height="0"> </embed> $ cat hellow_bmm.nmf {"program": {"x86-64": {"url": "hellow_bare"}}} $ gcc hellow_bare.c -I../third_party/lss/ -o hellow_bare -nostdlib -shared -fPIC -Wall $ ./tools/httpd.py & $ cd .. $ xvfb-run -a ./out/Release/chrome --enable-nacl --enable-nacl-nonsfi-mode http://localhost:5103/hellow_bmm.html Initially that gave: [26,4094916992:18:19:02.508650] Cannot open system random source /dev/urandom This might be a problem with my local Chromium build. But I added "--no-sandbox" and it gave: [16379,3948869376:10:22:55.393296] non executable which indicated that it was using the NaCl SFI code path (since that error comes from elf_util.c), so params.enable_nonsfi_mode wasn't being propagated properly -- see below. With some fixes (see comments below), I got this to work and print "Hello world!" to stdout on x86-64. Please can you run this test manually when updating the change? Also, can you make sure to run try jobs (with "git try") when sending out a change for review? I ran try jobs on patch set 3 and you can see that "cros_daisy" failed compilation (see comment below). https://codereview.chromium.org/100373005/diff/80001/components/nacl/browser/... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/100373005/diff/80001/components/nacl/browser/... components/nacl/browser/nacl_process_host.cc:710: params.enable_dyncode_syscalls = enable_dyncode_syscalls_; The assignment to params.enable_nonsfi_mode should go after this, next to the other initialisation of "params". https://codereview.chromium.org/100373005/diff/80001/components/nacl/browser/... components/nacl/browser/nacl_process_host.cc:762: params.enable_nonsfi_mode = CommandLine::ForCurrentProcess()->HasSwitch( This is after the message has been sent, so it doesn't have an effect https://codereview.chromium.org/100373005/diff/80001/components/nacl/common/n... File components/nacl/common/nacl_switches.cc (right): https://codereview.chromium.org/100373005/diff/80001/components/nacl/common/n... components/nacl/common/nacl_switches.cc:18: // Enables Non-SFI mode. Can you add a little more explanation, e.g. "Enables Non-SFI mode, in which programs can be run without NaCl's SFI sandbox" https://codereview.chromium.org/100373005/diff/80001/components/nacl/common/n... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/100373005/diff/80001/components/nacl/common/n... components/nacl/common/nacl_types.h:63: bool enable_nonsfi_mode; This doesn't work by itself. For the field to be marshalled across IPC, you must also update components/nacl/common/nacl_messages.h. https://codereview.chromium.org/100373005/diff/80001/components/nacl/loader/n... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/100373005/diff/80001/components/nacl/loader/n... components/nacl/loader/nacl_listener.cc:340: } else You could avoid the awkward dangling else by doing: if (params.enable_nonsfi_mode) { nacl::nonsfi::MainStart(args->imc_bootstrap_handle); NOTREACHED(); return; } https://codereview.chromium.org/100373005/diff/80001/components/nacl/loader/n... File components/nacl/loader/nonsfi/elf_loader.cc (right): https://codereview.chromium.org/100373005/diff/80001/components/nacl/loader/n... components/nacl/loader/nonsfi/elf_loader.cc:24: // don't expect that the code runs correctly. We should revisit here, when we I would like to be able to run this on x86-64 to test, not just compile-test it. :-) There's just two parts to change to make this work on x86-64: * make the ELF class check accept ELFCLASS64 on x86-64 * accept the e_machine value for x86-64 https://codereview.chromium.org/100373005/diff/80001/components/nacl/loader/n... components/nacl/loader/nonsfi/elf_loader.cc:29: # define NACL_ELF_E_MACHINE EM_386 Since you don't define NACL_ELF_E_MACHINE on non-x86, the code below will fail to compile on ARM and MIPS. Please run these changes through the Chromium trybots (as I mentioned before)! You can test x86-32 with "linux_rel_precise32" and (less obviously) ARM with "cros_daisy". https://codereview.chromium.org/100373005/diff/80001/components/nacl/loader/n... components/nacl/loader/nonsfi/elf_loader.cc:32: // Copied from native_client/src/trusted/service_runtime/include/bits/mman.h This is one header from service_runtime that it's OK to depend on, because you're calling NaClDesc methods which take NACL_ABI_* flags. So you should #include this rather than duplicating definitions which could get out of sync with the code you're calling. https://codereview.chromium.org/100373005/diff/80001/components/nacl/loader/n... components/nacl/loader/nonsfi/elf_loader.cc:111: LOG(ERROR) << "Non executable"; Nit: should be something like "Not a relocatable (ET_DYN) ELF object" https://codereview.chromium.org/100373005/diff/80001/components/nacl/loader/n... File components/nacl/loader/nonsfi/elf_loader.h (right): https://codereview.chromium.org/100373005/diff/80001/components/nacl/loader/n... components/nacl/loader/nonsfi/elf_loader.h:8: #include <elf.h> It would be better if these didn't appear in the header. They could easily end up conflicting with NaCl's ELF #defines. You don't actually need to define a class in the header. This header could just expose a single function for loading and running an executable given the descriptor. Then the class can be defined in the .cc file.
Could you take another look? I ran the test which you described in the previous review manually, and "Hello world" is successfully shown. Thanks, - hidehiko https://codereview.chromium.org/100373005/diff/80001/components/nacl/browser/... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/100373005/diff/80001/components/nacl/browser/... components/nacl/browser/nacl_process_host.cc:710: params.enable_dyncode_syscalls = enable_dyncode_syscalls_; On 2013/12/10 19:56:44, Mark Seaborn wrote: > The assignment to params.enable_nonsfi_mode should go after this, next to the > other initialisation of "params". Done. https://codereview.chromium.org/100373005/diff/80001/components/nacl/browser/... components/nacl/browser/nacl_process_host.cc:762: params.enable_nonsfi_mode = CommandLine::ForCurrentProcess()->HasSwitch( On 2013/12/10 19:56:44, Mark Seaborn wrote: > This is after the message has been sent, so it doesn't have an effect Oops... Done. https://codereview.chromium.org/100373005/diff/80001/components/nacl/common/n... File components/nacl/common/nacl_switches.cc (right): https://codereview.chromium.org/100373005/diff/80001/components/nacl/common/n... components/nacl/common/nacl_switches.cc:18: // Enables Non-SFI mode. On 2013/12/10 19:56:44, Mark Seaborn wrote: > Can you add a little more explanation, e.g. "Enables Non-SFI mode, in which > programs can be run without NaCl's SFI sandbox" Done. https://codereview.chromium.org/100373005/diff/80001/components/nacl/common/n... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/100373005/diff/80001/components/nacl/common/n... components/nacl/common/nacl_types.h:63: bool enable_nonsfi_mode; On 2013/12/10 19:56:44, Mark Seaborn wrote: > This doesn't work by itself. For the field to be marshalled across IPC, you > must also update components/nacl/common/nacl_messages.h. Ah, I should test it more carefully. It looked working because the allocated memory was uninitialized and as the result it was true... Fixed. https://codereview.chromium.org/100373005/diff/80001/components/nacl/loader/n... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/100373005/diff/80001/components/nacl/loader/n... components/nacl/loader/nacl_listener.cc:340: } else On 2013/12/10 19:56:44, Mark Seaborn wrote: > You could avoid the awkward dangling else by doing: > if (params.enable_nonsfi_mode) { > nacl::nonsfi::MainStart(args->imc_bootstrap_handle); > NOTREACHED(); > return; > } Done. https://codereview.chromium.org/100373005/diff/80001/components/nacl/loader/n... File components/nacl/loader/nonsfi/elf_loader.cc (right): https://codereview.chromium.org/100373005/diff/80001/components/nacl/loader/n... components/nacl/loader/nonsfi/elf_loader.cc:24: // don't expect that the code runs correctly. We should revisit here, when we On 2013/12/10 19:56:44, Mark Seaborn wrote: > I would like to be able to run this on x86-64 to test, not just compile-test it. > :-) There's just two parts to change to make this work on x86-64: > * make the ELF class check accept ELFCLASS64 on x86-64 > * accept the e_machine value for x86-64 Done. https://codereview.chromium.org/100373005/diff/80001/components/nacl/loader/n... components/nacl/loader/nonsfi/elf_loader.cc:29: # define NACL_ELF_E_MACHINE EM_386 On 2013/12/10 19:56:44, Mark Seaborn wrote: > Since you don't define NACL_ELF_E_MACHINE on non-x86, the code below will fail > to compile on ARM and MIPS. > > Please run these changes through the Chromium trybots (as I mentioned before)! > You can test x86-32 with "linux_rel_precise32" and (less obviously) ARM with > "cros_daisy". Done, and sorry for my less test. Ran trybot. If more test should be run, please let me know. https://codereview.chromium.org/100373005/diff/80001/components/nacl/loader/n... components/nacl/loader/nonsfi/elf_loader.cc:32: // Copied from native_client/src/trusted/service_runtime/include/bits/mman.h On 2013/12/10 19:56:44, Mark Seaborn wrote: > This is one header from service_runtime that it's OK to depend on, because > you're calling NaClDesc methods which take NACL_ABI_* flags. So you should > #include this rather than duplicating definitions which could get out of sync > with the code you're calling. I see. Done. Should the file be moved eventually? https://codereview.chromium.org/100373005/diff/80001/components/nacl/loader/n... components/nacl/loader/nonsfi/elf_loader.cc:111: LOG(ERROR) << "Non executable"; On 2013/12/10 19:56:44, Mark Seaborn wrote: > Nit: should be something like "Not a relocatable (ET_DYN) ELF object" Done. https://codereview.chromium.org/100373005/diff/80001/components/nacl/loader/n... File components/nacl/loader/nonsfi/elf_loader.h (right): https://codereview.chromium.org/100373005/diff/80001/components/nacl/loader/n... components/nacl/loader/nonsfi/elf_loader.h:8: #include <elf.h> On 2013/12/10 19:56:44, Mark Seaborn wrote: > It would be better if these didn't appear in the header. They could easily end > up conflicting with NaCl's ELF #defines. > > You don't actually need to define a class in the header. This header could just > expose a single function for loading and running an executable given the > descriptor. Then the class can be defined in the .cc file. Removed the include directive. Instead of having separate functions, I used pimpl for RAII.
OK, it's mostly just small things now... https://codereview.chromium.org/100373005/diff/120001/components/nacl.gyp File components/nacl.gyp (right): https://codereview.chromium.org/100373005/diff/120001/components/nacl.gyp#new... components/nacl.gyp:189: # Currently nonsfi mode can work on 32bit architecture, but can You can remove this comment since this will work on x86-64 now https://codereview.chromium.org/100373005/diff/120001/components/nacl/common/... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/100373005/diff/120001/components/nacl/common/... components/nacl/common/nacl_types.h:63: bool enable_nonsfi_mode; You should also update components/nacl/common/nacl_types.cc to change the constructor to initialise this field to false. https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/... File components/nacl/loader/nonsfi/elf_loader.cc (right): https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/... components/nacl/loader/nonsfi/elf_loader.cc:47: void DumpElfHeader(const ElfW(Ehdr)& ehdr) { Can you remove this DumpElfHeader logging, please? It's easy enough to dump ELF headers using objdump. https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/... components/nacl/loader/nonsfi/elf_loader.cc:174: VLOG(4) << "GetLoadSize: phnum=" << phnum; Can you remove the VLOGs for brevity, please? It's unlikely these will be useful beyond the initial development of the ELF loader. https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_error_code.h (right): https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_error_code.h:11: // The values are temporarily copied from The reason I wanted to minimise dependencies on service_runtime was to avoid awkward-to-maintain dependencies. If you're copying the same error codes into here that doesn't help, because that's more stuff to keep in sync. :-) So can you do one of the following: * either not try reporting any error codes at the moment (NaCl's existing ones are only slightly useful), * or just #include service_runtime/nacl_error_code.h and use those error codes. https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_main.cc (right): https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:19: #include "native_client/src/trusted/fault_injection/fault_injection.h" This one is not needed (see below) https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:31: void LoadModuleRpc(struct NaClSrpcRpc *rpc, Nit: use Chromium's "* " spacing style https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:35: UNREFERENCED_PARAMETER(out_args); UNREFERENCED_PARAMETER isn't needed in Chromium code (which is compiled with a different set of warnings), so please remove its uses for brevity. https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:36: VLOG(4) << "LoadModuleRpc: loading module."; As with the other file, can you remove the VLOGs, please? If anyone really wants to trace execution after this is committed they can add VLOGs locally or use a debugger. https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:57: uintptr_t* info = static_cast<uintptr_t*>(alloca(sizeof(uintptr_t) * 7)); Nit: it would be simpler to use an array: uintptr_t info[7]; You don't need to use alloca() here. https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:109: { NACL_SECURE_SERVICE_LOAD_MODULE, LoadModuleRpc, }, "load" is the only method handler that's needed at the moment, so you can you remove the others for now, please? ("log" and "hard_shutdown" are cruft that never get used even in normal NaCl.) https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:179: return (struct NaClDesc *) channel; Nit: use "NaClDesc*" spacing (Chromium style) https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:182: // Starts to listend the port and runs the server loop. "listen to" or "listen on"? https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:196: void NonSfiAllModulesInit(void) { Nit: "(void)" is not needed in C++ https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:197: NaClNrdAllModulesInit(); Calling this makes this fail with the following error when "--no-sandbox" is not passed: [26,4094916992:18:19:02.508650] Cannot open system random source /dev/urandom It seems to not be necessary, so you can remove it. https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:198: NaClFaultInjectionModuleInit(); This one is not needed
PTAL.
Daisy trybot is currently failing, but according to the log, the cause looks not
related to this CL. I'll re-run it later.
Could you kindly tell me who should I ask the review for nacl_messages.{h,cc}?
Thanks,
- hidehiko
https://codereview.chromium.org/100373005/diff/120001/components/nacl.gyp
File components/nacl.gyp (right):
https://codereview.chromium.org/100373005/diff/120001/components/nacl.gyp#new...
components/nacl.gyp:189: # Currently nonsfi mode can work on 32bit architecture,
but can
On 2013/12/12 04:49:48, Mark Seaborn wrote:
> You can remove this comment since this will work on x86-64 now
Done.
https://codereview.chromium.org/100373005/diff/120001/components/nacl/common/...
File components/nacl/common/nacl_types.h (right):
https://codereview.chromium.org/100373005/diff/120001/components/nacl/common/...
components/nacl/common/nacl_types.h:63: bool enable_nonsfi_mode;
On 2013/12/12 04:49:48, Mark Seaborn wrote:
> You should also update components/nacl/common/nacl_types.cc to change the
> constructor to initialise this field to false.
Done.
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
File components/nacl/loader/nonsfi/elf_loader.cc (right):
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
components/nacl/loader/nonsfi/elf_loader.cc:47: void DumpElfHeader(const
ElfW(Ehdr)& ehdr) {
On 2013/12/12 04:49:48, Mark Seaborn wrote:
> Can you remove this DumpElfHeader logging, please? It's easy enough to dump
ELF
> headers using objdump.
Done.
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
components/nacl/loader/nonsfi/elf_loader.cc:174: VLOG(4) << "GetLoadSize:
phnum=" << phnum;
On 2013/12/12 04:49:48, Mark Seaborn wrote:
> Can you remove the VLOGs for brevity, please? It's unlikely these will be
> useful beyond the initial development of the ELF loader.
Done.
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
File components/nacl/loader/nonsfi/nonsfi_error_code.h (right):
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_error_code.h:11: // The values are
temporarily copied from
On 2013/12/12 04:49:48, Mark Seaborn wrote:
> The reason I wanted to minimise dependencies on service_runtime was to avoid
> awkward-to-maintain dependencies. If you're copying the same error codes into
> here that doesn't help, because that's more stuff to keep in sync. :-) So can
> you do one of the following:
> * either not try reporting any error codes at the moment (NaCl's existing
ones
> are only slightly useful),
> * or just #include service_runtime/nacl_error_code.h and use those error
codes.
Ok, so please let me use nacl_error_code.h for now. I've deleted this file.
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
File components/nacl/loader/nonsfi/nonsfi_main.cc (right):
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_main.cc:19: #include
"native_client/src/trusted/fault_injection/fault_injection.h"
On 2013/12/12 04:49:48, Mark Seaborn wrote:
> This one is not needed (see below)
Done.
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_main.cc:31: void LoadModuleRpc(struct
NaClSrpcRpc *rpc,
On 2013/12/12 04:49:48, Mark Seaborn wrote:
> Nit: use Chromium's "* " spacing style
Done.
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_main.cc:35:
UNREFERENCED_PARAMETER(out_args);
On 2013/12/12 04:49:48, Mark Seaborn wrote:
> UNREFERENCED_PARAMETER isn't needed in Chromium code (which is compiled with a
> different set of warnings), so please remove its uses for brevity.
So, sometimes I'm a bit confused what style I should follow.
Should I follow just a Chromium style guide (more directly, can I ignore most of
native_client/src/...'s manner in most cases)?
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_main.cc:36: VLOG(4) << "LoadModuleRpc:
loading module.";
On 2013/12/12 04:49:48, Mark Seaborn wrote:
> As with the other file, can you remove the VLOGs, please? If anyone really
> wants to trace execution after this is committed they can add VLOGs locally or
> use a debugger.
Done.
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_main.cc:57: uintptr_t* info =
static_cast<uintptr_t*>(alloca(sizeof(uintptr_t) * 7));
On 2013/12/12 04:49:48, Mark Seaborn wrote:
> Nit: it would be simpler to use an array:
> uintptr_t info[7];
> You don't need to use alloca() here.
Good point. Fixed.
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_main.cc:109: {
NACL_SECURE_SERVICE_LOAD_MODULE, LoadModuleRpc, },
On 2013/12/12 04:49:48, Mark Seaborn wrote:
> "load" is the only method handler that's needed at the moment, so you can you
> remove the others for now, please? ("log" and "hard_shutdown" are cruft that
> never get used even in normal NaCl.)
Done.
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_main.cc:179: return (struct NaClDesc *)
channel;
On 2013/12/12 04:49:48, Mark Seaborn wrote:
> Nit: use "NaClDesc*" spacing (Chromium style)
Done.
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_main.cc:182: // Starts to listend the port
and runs the server loop.
On 2013/12/12 04:49:48, Mark Seaborn wrote:
> "listen to" or "listen on"?
Done.
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_main.cc:196: void
NonSfiAllModulesInit(void) {
On 2013/12/12 04:49:48, Mark Seaborn wrote:
> Nit: "(void)" is not needed in C++
acknowledged.
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_main.cc:197: NaClNrdAllModulesInit();
On 2013/12/12 04:49:48, Mark Seaborn wrote:
> Calling this makes this fail with the following error when "--no-sandbox" is
not
> passed:
> [26,4094916992:18:19:02.508650] Cannot open system random source /dev/urandom
>
> It seems to not be necessary, so you can remove it.
I see. Done.
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_main.cc:198:
NaClFaultInjectionModuleInit();
On 2013/12/12 04:49:48, Mark Seaborn wrote:
> This one is not needed
I thought this was needed otherwise FaultInjection mechanism won't work even in
NaCl's code, e.g. NaClDesc etc.
Removed.
Hi Mark, friendly ping?
On 2013/12/12 09:26:03, hidehiko wrote:
> PTAL.
>
> Daisy trybot is currently failing, but according to the log, the cause looks
not
> related to this CL. I'll re-run it later.
>
> Could you kindly tell me who should I ask the review for nacl_messages.{h,cc}?
>
> Thanks,
> - hidehiko
>
> https://codereview.chromium.org/100373005/diff/120001/components/nacl.gyp
> File components/nacl.gyp (right):
>
>
https://codereview.chromium.org/100373005/diff/120001/components/nacl.gyp#new...
> components/nacl.gyp:189: # Currently nonsfi mode can work on 32bit
architecture,
> but can
> On 2013/12/12 04:49:48, Mark Seaborn wrote:
> > You can remove this comment since this will work on x86-64 now
>
> Done.
>
>
https://codereview.chromium.org/100373005/diff/120001/components/nacl/common/...
> File components/nacl/common/nacl_types.h (right):
>
>
https://codereview.chromium.org/100373005/diff/120001/components/nacl/common/...
> components/nacl/common/nacl_types.h:63: bool enable_nonsfi_mode;
> On 2013/12/12 04:49:48, Mark Seaborn wrote:
> > You should also update components/nacl/common/nacl_types.cc to change the
> > constructor to initialise this field to false.
>
> Done.
>
>
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
> File components/nacl/loader/nonsfi/elf_loader.cc (right):
>
>
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
> components/nacl/loader/nonsfi/elf_loader.cc:47: void DumpElfHeader(const
> ElfW(Ehdr)& ehdr) {
> On 2013/12/12 04:49:48, Mark Seaborn wrote:
> > Can you remove this DumpElfHeader logging, please? It's easy enough to dump
> ELF
> > headers using objdump.
>
> Done.
>
>
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
> components/nacl/loader/nonsfi/elf_loader.cc:174: VLOG(4) << "GetLoadSize:
> phnum=" << phnum;
> On 2013/12/12 04:49:48, Mark Seaborn wrote:
> > Can you remove the VLOGs for brevity, please? It's unlikely these will be
> > useful beyond the initial development of the ELF loader.
>
> Done.
>
>
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
> File components/nacl/loader/nonsfi/nonsfi_error_code.h (right):
>
>
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
> components/nacl/loader/nonsfi/nonsfi_error_code.h:11: // The values are
> temporarily copied from
> On 2013/12/12 04:49:48, Mark Seaborn wrote:
> > The reason I wanted to minimise dependencies on service_runtime was to avoid
> > awkward-to-maintain dependencies. If you're copying the same error codes
into
> > here that doesn't help, because that's more stuff to keep in sync. :-) So
can
> > you do one of the following:
> > * either not try reporting any error codes at the moment (NaCl's existing
> ones
> > are only slightly useful),
> > * or just #include service_runtime/nacl_error_code.h and use those error
> codes.
>
> Ok, so please let me use nacl_error_code.h for now. I've deleted this file.
>
>
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
> File components/nacl/loader/nonsfi/nonsfi_main.cc (right):
>
>
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
> components/nacl/loader/nonsfi/nonsfi_main.cc:19: #include
> "native_client/src/trusted/fault_injection/fault_injection.h"
> On 2013/12/12 04:49:48, Mark Seaborn wrote:
> > This one is not needed (see below)
>
> Done.
>
>
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
> components/nacl/loader/nonsfi/nonsfi_main.cc:31: void LoadModuleRpc(struct
> NaClSrpcRpc *rpc,
> On 2013/12/12 04:49:48, Mark Seaborn wrote:
> > Nit: use Chromium's "* " spacing style
>
> Done.
>
>
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
> components/nacl/loader/nonsfi/nonsfi_main.cc:35:
> UNREFERENCED_PARAMETER(out_args);
> On 2013/12/12 04:49:48, Mark Seaborn wrote:
> > UNREFERENCED_PARAMETER isn't needed in Chromium code (which is compiled with
a
> > different set of warnings), so please remove its uses for brevity.
>
> So, sometimes I'm a bit confused what style I should follow.
> Should I follow just a Chromium style guide (more directly, can I ignore most
of
> native_client/src/...'s manner in most cases)?
>
>
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
> components/nacl/loader/nonsfi/nonsfi_main.cc:36: VLOG(4) << "LoadModuleRpc:
> loading module.";
> On 2013/12/12 04:49:48, Mark Seaborn wrote:
> > As with the other file, can you remove the VLOGs, please? If anyone really
> > wants to trace execution after this is committed they can add VLOGs locally
or
> > use a debugger.
>
> Done.
>
>
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
> components/nacl/loader/nonsfi/nonsfi_main.cc:57: uintptr_t* info =
> static_cast<uintptr_t*>(alloca(sizeof(uintptr_t) * 7));
> On 2013/12/12 04:49:48, Mark Seaborn wrote:
> > Nit: it would be simpler to use an array:
> > uintptr_t info[7];
> > You don't need to use alloca() here.
>
> Good point. Fixed.
>
>
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
> components/nacl/loader/nonsfi/nonsfi_main.cc:109: {
> NACL_SECURE_SERVICE_LOAD_MODULE, LoadModuleRpc, },
> On 2013/12/12 04:49:48, Mark Seaborn wrote:
> > "load" is the only method handler that's needed at the moment, so you can
you
> > remove the others for now, please? ("log" and "hard_shutdown" are cruft
that
> > never get used even in normal NaCl.)
>
> Done.
>
>
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
> components/nacl/loader/nonsfi/nonsfi_main.cc:179: return (struct NaClDesc *)
> channel;
> On 2013/12/12 04:49:48, Mark Seaborn wrote:
> > Nit: use "NaClDesc*" spacing (Chromium style)
>
> Done.
>
>
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
> components/nacl/loader/nonsfi/nonsfi_main.cc:182: // Starts to listend the
port
> and runs the server loop.
> On 2013/12/12 04:49:48, Mark Seaborn wrote:
> > "listen to" or "listen on"?
>
> Done.
>
>
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
> components/nacl/loader/nonsfi/nonsfi_main.cc:196: void
> NonSfiAllModulesInit(void) {
> On 2013/12/12 04:49:48, Mark Seaborn wrote:
> > Nit: "(void)" is not needed in C++
>
> acknowledged.
>
>
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
> components/nacl/loader/nonsfi/nonsfi_main.cc:197: NaClNrdAllModulesInit();
> On 2013/12/12 04:49:48, Mark Seaborn wrote:
> > Calling this makes this fail with the following error when "--no-sandbox" is
> not
> > passed:
> > [26,4094916992:18:19:02.508650] Cannot open system random source
/dev/urandom
> >
> > It seems to not be necessary, so you can remove it.
>
> I see. Done.
>
>
https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/...
> components/nacl/loader/nonsfi/nonsfi_main.cc:198:
> NaClFaultInjectionModuleInit();
> On 2013/12/12 04:49:48, Mark Seaborn wrote:
> > This one is not needed
>
> I thought this was needed otherwise FaultInjection mechanism won't work even
in
> NaCl's code, e.g. NaClDesc etc.
> Removed.
LGTM https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_main.cc (right): https://codereview.chromium.org/100373005/diff/120001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:35: UNREFERENCED_PARAMETER(out_args); On 2013/12/12 09:26:04, hidehiko wrote: > On 2013/12/12 04:49:48, Mark Seaborn wrote: > > UNREFERENCED_PARAMETER isn't needed in Chromium code (which is compiled with a > > different set of warnings), so please remove its uses for brevity. > > So, sometimes I'm a bit confused what style I should follow. > Should I follow just a Chromium style guide (more directly, can I ignore most of > native_client/src/...'s manner in most cases)? Yes, since this is in the Chromium tree, please ignore the style of native_client/ and use the Chromium style. https://codereview.chromium.org/100373005/diff/160001/components/nacl/loader/... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/100373005/diff/160001/components/nacl/loader/... components/nacl/loader/nacl_listener.cc:335: Nit: remove one empty line here so that there's not multiple consecutive empty lines within a function https://codereview.chromium.org/100373005/diff/160001/components/nacl/loader/... File components/nacl/loader/nonsfi/elf_loader.cc (right): https://codereview.chromium.org/100373005/diff/160001/components/nacl/loader/... components/nacl/loader/nonsfi/elf_loader.cc:120: ElfW(Addr) begin = 0xFFFFFFFFU; Nit: since this could be 64-bit it would be a little bit more correct to use "~(ElfW(Addr)) 0". https://codereview.chromium.org/100373005/diff/160001/components/nacl/loader/... components/nacl/loader/nonsfi/elf_loader.cc:135: // The end address looks overflowing. If you're going to check for overflow it would be better to check for overflow in "phdr.p_vaddr + phdr.p_memsz" earlier. I suppose this check covers an absence of PT_LOAD segments (might be worth commenting that). https://codereview.chromium.org/100373005/diff/160001/components/nacl/loader/... components/nacl/loader/nonsfi/elf_loader.cc:143: // its size and bias to the load_start, load_size and load_bias. load_start and load_size aren't used in the code any more. https://codereview.chromium.org/100373005/diff/160001/components/nacl/loader/... components/nacl/loader/nonsfi/elf_loader.cc:161: << "Reservememory: Non-PIE binary loading is not supported."; Nit: "ReserveMemory" (to match function name) https://codereview.chromium.org/100373005/diff/160001/components/nacl/loader/... components/nacl/loader/nonsfi/elf_loader.cc:213: // Fill Zero between the segment end and the page boundary if necessary You might say "Handle the BSS: fill zero..." (just to use the term "BSS") https://codereview.chromium.org/100373005/diff/160001/components/nacl/loader/... components/nacl/loader/nonsfi/elf_loader.cc:321: LOG(ERROR) << "ElfImage::Load: Failed to allocate memory."; Nit: remove trailing '.' from log messages to be consistent https://codereview.chromium.org/100373005/diff/160001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_main.cc (right): https://codereview.chromium.org/100373005/diff/160001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:83: // Wraps handle by NaClDesc, and sends sercure_service_address and Typo: "sercure" https://codereview.chromium.org/100373005/diff/160001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:113: struct NaClImcTypedMsgHdr hdr; You've missed initialising "hdr.flags = 0;"
Thank you, Mark, for your review! jschuh@, could you kindly review components/nacl/common/nacl_messages.h as an OWNER? https://codereview.chromium.org/100373005/diff/160001/components/nacl/loader/... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/100373005/diff/160001/components/nacl/loader/... components/nacl/loader/nacl_listener.cc:335: On 2013/12/17 21:18:55, Mark Seaborn wrote: > Nit: remove one empty line here so that there's not multiple consecutive empty > lines within a function Done. https://codereview.chromium.org/100373005/diff/160001/components/nacl/loader/... File components/nacl/loader/nonsfi/elf_loader.cc (right): https://codereview.chromium.org/100373005/diff/160001/components/nacl/loader/... components/nacl/loader/nonsfi/elf_loader.cc:120: ElfW(Addr) begin = 0xFFFFFFFFU; On 2013/12/17 21:18:55, Mark Seaborn wrote: > Nit: since this could be 64-bit it would be a little bit more correct to use > "~(ElfW(Addr)) 0". Done. https://codereview.chromium.org/100373005/diff/160001/components/nacl/loader/... components/nacl/loader/nonsfi/elf_loader.cc:135: // The end address looks overflowing. On 2013/12/17 21:18:55, Mark Seaborn wrote: > If you're going to check for overflow it would be better to check for overflow > in "phdr.p_vaddr + phdr.p_memsz" earlier. I suppose this check covers an > absence of PT_LOAD segments (might be worth commenting that). You're right. I included no-PT_LOAD section check here. Added a short comment. https://codereview.chromium.org/100373005/diff/160001/components/nacl/loader/... components/nacl/loader/nonsfi/elf_loader.cc:143: // its size and bias to the load_start, load_size and load_bias. On 2013/12/17 21:18:55, Mark Seaborn wrote: > load_start and load_size aren't used in the code any more. Done. https://codereview.chromium.org/100373005/diff/160001/components/nacl/loader/... components/nacl/loader/nonsfi/elf_loader.cc:161: << "Reservememory: Non-PIE binary loading is not supported."; On 2013/12/17 21:18:55, Mark Seaborn wrote: > Nit: "ReserveMemory" (to match function name) Done. https://codereview.chromium.org/100373005/diff/160001/components/nacl/loader/... components/nacl/loader/nonsfi/elf_loader.cc:213: // Fill Zero between the segment end and the page boundary if necessary On 2013/12/17 21:18:55, Mark Seaborn wrote: > You might say "Handle the BSS: fill zero..." (just to use the term "BSS") Done. https://codereview.chromium.org/100373005/diff/160001/components/nacl/loader/... components/nacl/loader/nonsfi/elf_loader.cc:321: LOG(ERROR) << "ElfImage::Load: Failed to allocate memory."; On 2013/12/17 21:18:55, Mark Seaborn wrote: > Nit: remove trailing '.' from log messages to be consistent Done. https://codereview.chromium.org/100373005/diff/160001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_main.cc (right): https://codereview.chromium.org/100373005/diff/160001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:83: // Wraps handle by NaClDesc, and sends sercure_service_address and On 2013/12/17 21:18:55, Mark Seaborn wrote: > Typo: "sercure" Done. https://codereview.chromium.org/100373005/diff/160001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:113: struct NaClImcTypedMsgHdr hdr; On 2013/12/17 21:18:55, Mark Seaborn wrote: > You've missed initialising "hdr.flags = 0;" Good catch. Fixed.
On 2013/12/18 08:38:02, hidehiko wrote: > jschuh@, could you kindly review components/nacl/common/nacl_messages.h > as an OWNER? jln is also an OWNER for that file, and he would be a good reviewer since he knows about the background for NaCl's Bare Metal (non-SFI) Mode.
Ah, sorry. Given the background, @jln is probably a much better choice for reviewer. On Fri, Dec 20, 2013 at 12:35 PM, <mseaborn@chromium.org> wrote: > On 2013/12/18 08:38:02, hidehiko wrote: > >> jschuh@, could you kindly review components/nacl/common/nacl_messages.h >> as an OWNER? >> > > jln is also an OWNER for that file, and he would be a good reviewer since > he > knows about the background for NaCl's Bare Metal (non-SFI) Mode. > > https://codereview.chromium.org/100373005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 20 December 2013 12:35, <mseaborn@chromium.org> wrote: > On 2013/12/18 08:38:02, hidehiko wrote: > >> jschuh@, could you kindly review components/nacl/common/nacl_messages.h >> as an OWNER? >> > > jln is also an OWNER for that file, and he would be a good reviewer since > he > knows about the background for NaCl's Bare Metal (non-SFI) Mode. > @hidehiko: Can you commit this with "TBR=jln@chromium.org", please? It's just a one-line change that needs the owners review, and that's only to match up with another file, so I don't think that should hold up getting this committed. Cheers, Mark To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/01/02 18:12:19, Mark Seaborn wrote: > On 20 December 2013 12:35, <mailto:mseaborn@chromium.org> wrote: > > > On 2013/12/18 08:38:02, hidehiko wrote: > > > >> jschuh@, could you kindly review components/nacl/common/nacl_messages.h > >> as an OWNER? > >> > > > > jln is also an OWNER for that file, and he would be a good reviewer since > > he > > knows about the background for NaCl's Bare Metal (non-SFI) Mode. > > > > @hidehiko: Can you commit this with "TBR=jln@chromium.org", please? It's > just a one-line change that needs the owners review, and that's only to > match up with another file, so I don't think that should hold up getting > this committed. > > Cheers, > Mark > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Hi Mark, Thank you for your advice. Sending to CQ. - hidehiko
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hidehiko@chromium.org/100373005/250001
Message was sent while issue was closed.
Change committed as 243060
Message was sent while issue was closed.
components/nacl/common/nacl_messages.h lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
