Pepper: Move GetSandboxISA() to PPB_NaCl_Private.

Pepper: Move GetSandboxISA() to PPB_NaCl_Private.

This is part of a series of changes to move the "trusted plugin" code inside
ppapi/native_client. I was moving the logic for JsonManifest and
PNaClManifest when I noticed that PNaClManifest depends on GetSandboxISA(), and
moving that seemed big enough to justify its own change.

I think that I preserved the architecture logic in arch_x86/sandbox_isa.cc, but
it would merit some careful review.

BUG=239656
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257196

[dmichael (off chromium), 2014-03-10 22:49:35]

I haven't looked at everything, but here are some early comments based on our
discussion this afternoon.

https://codereview.chromium.org/193313002/diff/20001/components/nacl.gyp
File components/nacl.gyp (right):

https://codereview.chromium.org/193313002/diff/20001/components/nacl.gyp#newc...
components/nacl.gyp:170: 'nacl/renderer/arch_x86/sandbox_isa.cc',
I seem to remember that these sandbox_isa files are the only things in those
arch_xxx subdirectories. What do you think of putting them all in the same
directory and suffixing them?

You could even just stick them in 1 file. We usually prefer not to rely on ifdef
too much, but these are so short & simple, that might actually be the most
readable.

Up to your discretion, though I think we can do away with the extra directories.

https://codereview.chromium.org/193313002/diff/20001/components/nacl.gyp#newc...
components/nacl.gyp:173: 'TARGET_ARCH_X64',
Can you just use ARCH_CPU_64_BITS?
https://code.google.com/p/chromium/codesearch#chromium/src/build/build_config...

https://codereview.chromium.org/193313002/diff/20001/components/nacl/renderer...
File components/nacl/renderer/arch_x86/sandbox_isa.cc (right):

https://codereview.chromium.org/193313002/diff/20001/components/nacl/renderer...
components/nacl/renderer/arch_x86/sandbox_isa.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
nit: 2014

https://codereview.chromium.org/193313002/diff/20001/components/nacl/renderer...
components/nacl/renderer/arch_x86/sandbox_isa.cc:13: namespace nacl {
nit: should be a carriage return after this

https://codereview.chromium.org/193313002/diff/20001/components/nacl/renderer...
components/nacl/renderer/arch_x86/sandbox_isa.cc:20: return kNexeArchX86_64;
We talked about how on Windows 64, we want this to be based on the host
architecture (not target arch, which is usually 32 currently for the renderer).

I think you said you were worried this was wrong, but...  OSInfo appears to
return host architecture information, so I think this is right.

https://codereview.chromium.org/193313002/diff/20001/components/nacl/renderer...
components/nacl/renderer/arch_x86/sandbox_isa.cc:25: }  // namespace nacl
nit: should be a carriage return before this

[teravest, 2014-03-11 18:47:36]

Thanks for the quick review.

On Mon, Mar 10, 2014 at 4:49 PM,  <dmichael@chromium.org> wrote:
> I haven't looked at everything, but here are some early comments based on
> our
> discussion this afternoon.
>
>
> https://codereview.chromium.org/193313002/diff/20001/components/nacl.gyp
> File components/nacl.gyp (right):
>
>
https://codereview.chromium.org/193313002/diff/20001/components/nacl.gyp#newc...
> components/nacl.gyp:170: 'nacl/renderer/arch_x86/sandbox_isa.cc',
> I seem to remember that these sandbox_isa files are the only things in
> those arch_xxx subdirectories. What do you think of putting them all in
> the same directory and suffixing them?

Joined all into one file.

>
> You could even just stick them in 1 file. We usually prefer not to rely
> on ifdef too much, but these are so short & simple, that might actually
> be the most readable.
>
> Up to your discretion, though I think we can do away with the extra
> directories.
>
>
https://codereview.chromium.org/193313002/diff/20001/components/nacl.gyp#newc...
> components/nacl.gyp:173: 'TARGET_ARCH_X64',
> Can you just use ARCH_CPU_64_BITS?

I reorganized this.

>
https://code.google.com/p/chromium/codesearch#chromium/src/build/build_config...
>
>
https://codereview.chromium.org/193313002/diff/20001/components/nacl/renderer...
> File components/nacl/renderer/arch_x86/sandbox_isa.cc (right):
>
>
https://codereview.chromium.org/193313002/diff/20001/components/nacl/renderer...
> components/nacl/renderer/arch_x86/sandbox_isa.cc:1: // Copyright (c)
> 2012 The Chromium Authors. All rights reserved.
> nit: 2014

Done.

>
>
https://codereview.chromium.org/193313002/diff/20001/components/nacl/renderer...
> components/nacl/renderer/arch_x86/sandbox_isa.cc:13: namespace nacl {
> nit: should be a carriage return after this

Done.

>
>
https://codereview.chromium.org/193313002/diff/20001/components/nacl/renderer...
> components/nacl/renderer/arch_x86/sandbox_isa.cc:20: return
> kNexeArchX86_64;
> We talked about how on Windows 64, we want this to be based on the host
> architecture (not target arch, which is usually 32 currently for the
> renderer).
>
> I think you said you were worried this was wrong, but...  OSInfo appears
> to return host architecture information, so I think this is right.

I was concerned that the architecture flags in the native_client build
might not match those passed when we build the renderer-- I don't know
enough about how we build native_client to be sure about that.

>
>
https://codereview.chromium.org/193313002/diff/20001/components/nacl/renderer...
> components/nacl/renderer/arch_x86/sandbox_isa.cc:25: }  // namespace
> nacl
> nit: should be a carriage return before this

Done.

>
> https://codereview.chromium.org/193313002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmichael (off chromium), 2014-03-11 19:00:15]

https://codereview.chromium.org/193313002/diff/40001/components/nacl/renderer...
File components/nacl/renderer/sandbox_isa.cc (right):

https://codereview.chromium.org/193313002/diff/40001/components/nacl/renderer...
components/nacl/renderer/sandbox_isa.cc:35: return kNexeArchX86_64;
That doesn't seem right for Linux 32 and possibly Mac?

https://codereview.chromium.org/193313002/diff/40001/components/nacl/renderer...
File components/nacl/renderer/sandbox_isa.h (right):

https://codereview.chromium.org/193313002/diff/40001/components/nacl/renderer...
components/nacl/renderer/sandbox_isa.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
nit: 2014

https://codereview.chromium.org/193313002/diff/40001/components/nacl/renderer...
components/nacl/renderer/sandbox_isa.h:9: #define
CONTENT_NACL_RENDERER_NEXE_ARCH_H
^^^ doesn't match path anymore

https://codereview.chromium.org/193313002/diff/40001/components/nacl/renderer...
components/nacl/renderer/sandbox_isa.h:13: // platform.  See the implementations
in arch_*/sandbox_isa.cc for possible
ditto

https://codereview.chromium.org/193313002/diff/40001/components/nacl/renderer...
components/nacl/renderer/sandbox_isa.h:18: // since, for example, an x86-32
plugin is capable of launching a 64-bit NaCl
plugin->browser ?

[teravest, 2014-03-11 19:24:22]

On Tue, Mar 11, 2014 at 1:00 PM,  <dmichael@chromium.org> wrote:
>
>
https://codereview.chromium.org/193313002/diff/40001/components/nacl/renderer...
> File components/nacl/renderer/sandbox_isa.cc (right):
>
>
https://codereview.chromium.org/193313002/diff/40001/components/nacl/renderer...
> components/nacl/renderer/sandbox_isa.cc:35: return kNexeArchX86_64;
> That doesn't seem right for Linux 32 and possibly Mac?

You're right. Thanks. Fixed.

>
>
https://codereview.chromium.org/193313002/diff/40001/components/nacl/renderer...
> File components/nacl/renderer/sandbox_isa.h (right):
>
>
https://codereview.chromium.org/193313002/diff/40001/components/nacl/renderer...
> components/nacl/renderer/sandbox_isa.h:1: // Copyright (c) 2012 The
>
> Chromium Authors. All rights reserved.
> nit: 2014

Done.

>
>
https://codereview.chromium.org/193313002/diff/40001/components/nacl/renderer...
> components/nacl/renderer/sandbox_isa.h:9: #define
> CONTENT_NACL_RENDERER_NEXE_ARCH_H
> ^^^ doesn't match path anymore

Done.

>
>
https://codereview.chromium.org/193313002/diff/40001/components/nacl/renderer...
> components/nacl/renderer/sandbox_isa.h:13: // platform.  See the
> implementations in arch_*/sandbox_isa.cc for possible
> ditto

Done.

>
>
https://codereview.chromium.org/193313002/diff/40001/components/nacl/renderer...
> components/nacl/renderer/sandbox_isa.h:18: // since, for example, an
> x86-32 plugin is capable of launching a 64-bit NaCl
> plugin->browser ?

Done.

>
> https://codereview.chromium.org/193313002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmichael (off chromium), 2014-03-11 21:58:03]

lgtm

https://codereview.chromium.org/193313002/diff/60001/components/nacl/renderer...
File components/nacl/renderer/sandbox_isa.cc (right):

https://codereview.chromium.org/193313002/diff/60001/components/nacl/renderer...
components/nacl/renderer/sandbox_isa.cc:29: if
(base::win::OSInfo::GetInstance()->architecture() == X64_ARCHITECTURE)
Wasn't there a comment at some point about checking the host (not target)
architecture? It's subtle enough it might be nice to comment.

https://codereview.chromium.org/193313002/diff/60001/components/nacl/renderer...
components/nacl/renderer/sandbox_isa.cc:34: #if ARCH_CPU_64_BITS
optional nit: #elif might be clearer?

https://codereview.chromium.org/193313002/diff/60001/components/nacl/renderer...
File components/nacl/renderer/sandbox_isa.h (right):

https://codereview.chromium.org/193313002/diff/60001/components/nacl/renderer...
components/nacl/renderer/sandbox_isa.h:9: #define
CONTENT_NACL_RENDERER_SANDBOX_ISA_H
CONTENT->COMPONENTS

[teravest, 2014-03-11 22:09:39]

The CQ bit was checked by teravest@chromium.org

[teravest1, 2014-03-11 22:09:41]

On Tue, Mar 11, 2014 at 3:58 PM,  <dmichael@chromium.org> wrote:
> lgtm
>
>
>
>
>
https://codereview.chromium.org/193313002/diff/60001/components/nacl/renderer...
> File components/nacl/renderer/sandbox_isa.cc (right):
>
>
https://codereview.chromium.org/193313002/diff/60001/components/nacl/renderer...
> components/nacl/renderer/sandbox_isa.cc:29: if
> (base::win::OSInfo::GetInstance()->architecture() == X64_ARCHITECTURE)
> Wasn't there a comment at some point about checking the host (not
> target) architecture? It's subtle enough it might be nice to comment.

Added a comment.

>
>
https://codereview.chromium.org/193313002/diff/60001/components/nacl/renderer...
> components/nacl/renderer/sandbox_isa.cc:34: #if ARCH_CPU_64_BITS
> optional nit: #elif might be clearer?

Done.

>
>
https://codereview.chromium.org/193313002/diff/60001/components/nacl/renderer...
> File components/nacl/renderer/sandbox_isa.h (right):
>
>
https://codereview.chromium.org/193313002/diff/60001/components/nacl/renderer...
> components/nacl/renderer/sandbox_isa.h:9: #define
> CONTENT_NACL_RENDERER_SANDBOX_ISA_H
> CONTENT->COMPONENTS

Done.

>
> https://codereview.chromium.org/193313002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[teravest, 2014-03-11 22:09:44]

The CQ bit was unchecked by teravest@chromium.org

[teravest, 2014-03-11 22:10:23]

+mseaborn for components/nacl.gyp

[Mark Seaborn, 2014-03-12 18:24:13]

LGTM

https://codereview.chromium.org/193313002/diff/80001/components/nacl.gyp
File components/nacl.gyp (right):

https://codereview.chromium.org/193313002/diff/80001/components/nacl.gyp#newc...
components/nacl.gyp:149: 'nacl/renderer/sandbox_isa.cc',
How about calling these "sandbox_arch.{cc,h}"?  "arch" is the more usual term in
NaCl, and "isa" is more obscure.  This seems like a good opportunity to make the
code a little more readable. :-)

https://codereview.chromium.org/193313002/diff/80001/components/nacl/renderer...
File components/nacl/renderer/sandbox_isa.cc (right):

https://codereview.chromium.org/193313002/diff/80001/components/nacl/renderer...
components/nacl/renderer/sandbox_isa.cc:5: // The list of supported ISA strings
for x86.  See issue:
Drop "for x86"?

https://codereview.chromium.org/193313002/diff/80001/components/nacl/renderer...
components/nacl/renderer/sandbox_isa.cc:14: const char* const kNexeArchARM =
"arm";
Better to write:
static const char kFoo[] = "...";
since that potentially avoids a relocation.

This should be static (or in an anon namespace).  If that gives warnings about
unused variables, just use string literals in GetSandboxISA().  There's really
no benefit to putting strings in global variables.

https://codereview.chromium.org/193313002/diff/80001/components/nacl/renderer...
components/nacl/renderer/sandbox_isa.cc:36: return kNexeArchX86_64;
Reduce indentation by 2

https://codereview.chromium.org/193313002/diff/80001/components/nacl/renderer...
components/nacl/renderer/sandbox_isa.cc:38: return kNexeArchX86_32;
ditto

https://codereview.chromium.org/193313002/diff/80001/components/nacl/renderer...
components/nacl/renderer/sandbox_isa.cc:43: NOTREACHED();
This won't be compiled if you've just #error'd, so you could drop this

https://codereview.chromium.org/193313002/diff/80001/components/nacl/renderer...
File components/nacl/renderer/sandbox_isa.h (right):

https://codereview.chromium.org/193313002/diff/80001/components/nacl/renderer...
components/nacl/renderer/sandbox_isa.h:12: // Returns the kind of SFI sandbox
implemented by sel_ldr on this
"sel_ldr" -> "NaCl"

https://codereview.chromium.org/193313002/diff/80001/components/nacl/renderer...
components/nacl/renderer/sandbox_isa.h:19: // sandbox if a 64-bit sel_ldr is
installed (and indeed, may only be capable of
Chromium doesn't use sel_ldr any more.  You could comment about using an x86-64
sandbox when chrome.exe is 32-bit, but that will eventually change with 64-bit
Chrome, so maybe leave this part out?  Better not to put implementation details
here.

https://codereview.chromium.org/193313002/diff/80001/components/nacl/renderer...
components/nacl/renderer/sandbox_isa.h:21: const char* GetSandboxISA();
Maybe GetSandboxArch() here too?

https://codereview.chromium.org/193313002/diff/80001/ppapi/api/private/ppb_na...
File ppapi/api/private/ppb_nacl_private.idl (right):

https://codereview.chromium.org/193313002/diff/80001/ppapi/api/private/ppb_na...
ppapi/api/private/ppb_nacl_private.idl:281: str_t GetSandboxISA();
Maybe add a comment, since the other functions here have comments?

[teravest, 2014-03-12 20:05:41]

Thanks.

On Wed, Mar 12, 2014 at 12:24 PM,  <mseaborn@chromium.org> wrote:
> LGTM
>
>
> https://codereview.chromium.org/193313002/diff/80001/components/nacl.gyp
> File components/nacl.gyp (right):
>
>
https://codereview.chromium.org/193313002/diff/80001/components/nacl.gyp#newc...
> components/nacl.gyp:149: 'nacl/renderer/sandbox_isa.cc',
> How about calling these "sandbox_arch.{cc,h}"?  "arch" is the more usual
> term in NaCl, and "isa" is more obscure.  This seems like a good
> opportunity to make the code a little more readable. :-)

Sounds good. isa -> arch throughout.

>
>
https://codereview.chromium.org/193313002/diff/80001/components/nacl/renderer...
> File components/nacl/renderer/sandbox_isa.cc (right):
>
>
https://codereview.chromium.org/193313002/diff/80001/components/nacl/renderer...
> components/nacl/renderer/sandbox_isa.cc:5: // The list of supported ISA
> strings for x86.  See issue:
> Drop "for x86"?

Done.

>
>
https://codereview.chromium.org/193313002/diff/80001/components/nacl/renderer...
> components/nacl/renderer/sandbox_isa.cc:14: const char* const
> kNexeArchARM = "arm";
> Better to write:
> static const char kFoo[] = "...";
> since that potentially avoids a relocation.
>
> This should be static (or in an anon namespace).  If that gives warnings
> about unused variables, just use string literals in GetSandboxISA().
> There's really no benefit to putting strings in global variables.

Done.

>
>
https://codereview.chromium.org/193313002/diff/80001/components/nacl/renderer...
> components/nacl/renderer/sandbox_isa.cc:36: return kNexeArchX86_64;
> Reduce indentation by 2

Done.

>
>
https://codereview.chromium.org/193313002/diff/80001/components/nacl/renderer...
> components/nacl/renderer/sandbox_isa.cc:38: return kNexeArchX86_32;
> ditto

Done.

>
>
https://codereview.chromium.org/193313002/diff/80001/components/nacl/renderer...
> components/nacl/renderer/sandbox_isa.cc:43: NOTREACHED();
> This won't be compiled if you've just #error'd, so you could drop this

Removed.

>
>
https://codereview.chromium.org/193313002/diff/80001/components/nacl/renderer...
> File components/nacl/renderer/sandbox_isa.h (right):
>
>
https://codereview.chromium.org/193313002/diff/80001/components/nacl/renderer...
> components/nacl/renderer/sandbox_isa.h:12: // Returns the kind of SFI
> sandbox implemented by sel_ldr on this
> "sel_ldr" -> "NaCl"

Done.

>
>
https://codereview.chromium.org/193313002/diff/80001/components/nacl/renderer...
> components/nacl/renderer/sandbox_isa.h:19: // sandbox if a 64-bit
> sel_ldr is installed (and indeed, may only be capable of
> Chromium doesn't use sel_ldr any more.  You could comment about using an
> x86-64 sandbox when chrome.exe is 32-bit, but that will eventually
> change with 64-bit Chrome, so maybe leave this part out?  Better not to
> put implementation details here.

I removed the whole block.

>
>
https://codereview.chromium.org/193313002/diff/80001/components/nacl/renderer...
> components/nacl/renderer/sandbox_isa.h:21: const char* GetSandboxISA();
> Maybe GetSandboxArch() here too?

Done.

>
>
https://codereview.chromium.org/193313002/diff/80001/ppapi/api/private/ppb_na...
> File ppapi/api/private/ppb_nacl_private.idl (right):
>
>
https://codereview.chromium.org/193313002/diff/80001/ppapi/api/private/ppb_na...
> ppapi/api/private/ppb_nacl_private.idl:281: str_t GetSandboxISA();
> Maybe add a comment, since the other functions here have comments?

Sure, done.

>
> https://codereview.chromium.org/193313002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[teravest, 2014-03-12 21:44:37]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-03-12 21:45:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/193313002/140001

[commit-bot: I haz the power, 2014-03-12 23:59:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-12 23:59:48]

Retried try job too often on win_x64_rel for step(s) base_unittests,
chrome_elf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[teravest, 2014-03-13 15:30:31]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-03-13 15:30:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/193313002/160001

[commit-bot: I haz the power, 2014-03-13 15:36:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 15:36:41]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[teravest, 2014-03-13 15:39:53]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-03-13 15:40:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/193313002/160001

[commit-bot: I haz the power, 2014-03-13 15:47:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 15:47:08]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_clang_dbg
  tryserver.chromium on linux_chromium_clang_dbg

[teravest, 2014-03-13 17:10:52]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-03-13 17:12:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/193313002/160001

[commit-bot: I haz the power, 2014-03-13 17:20:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 17:20:40]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[teravest, 2014-03-13 19:26:37]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-03-13 19:28:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/193313002/160001

[commit-bot: I haz the power, 2014-03-13 21:07:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 21:07:01]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[teravest, 2014-03-13 21:11:13]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-03-13 21:17:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/193313002/180001

[commit-bot: I haz the power, 2014-03-13 21:28:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 21:28:43]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[teravest, 2014-03-14 16:49:08]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-03-14 16:49:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/193313002/180001

[commit-bot: I haz the power, 2014-03-14 21:08:04]

Change committed as 257196