Added 32-bit version of nonsfi monacl shell

Added 32-bit version of nonsfi monacl shell

BUG=https://github.com/domokit/mojo/issues/396
R=mseaborn@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/7b0e143e9861e72663d31af92023953867d4909c

[Sean Klein, 2015-09-08 22:55:11]

smklein@chromium.org changed reviewers:
+ mseaborn@chromium.org

[Mark Seaborn, 2015-09-08 23:57:43]

It's nice that this turned out to be simple!

Some nits...

https://codereview.chromium.org/1327913004/diff/1/mojo/nacl/BUILD.gn
File mojo/nacl/BUILD.gn (right):

https://codereview.chromium.org/1327913004/diff/1/mojo/nacl/BUILD.gn#newcode36
mojo/nacl/BUILD.gn:36: group("monacl_shell_nonsfi_x86") {
Should this be added as a dependency of group("mojo_nacl"), so that it's
included in the set of targets built by the bots?  Currently the bots don't
build it.

Also, I think this should be conditionalised on "x86-32 or x86-64", otherwise
we'd attempt to build it when targeting ARM.

https://codereview.chromium.org/1327913004/diff/1/mojo/nacl/BUILD.gn#newcode39
mojo/nacl/BUILD.gn:39:
":monacl_shell_nonsfi(//build/toolchain/linux:clang_x86)",
Since this is Linux-specific, it seems like the target should be conditionalised
on Linux too.  Though I'm not sure whether the codebase as a whole is intended
to support non-Linux OSes.

Is there a way to do this without assuming Clang?  i.e. Is there a way to ask
for "the 32-bit variant of whatever toolchain we're using (whether it's GCC or
Clang or something else)"?  If not, can you add a comment to explain the
assumption and say it's not ideal?

[Sean Klein, 2015-09-09 17:37:07]

https://codereview.chromium.org/1327913004/diff/1/mojo/nacl/BUILD.gn
File mojo/nacl/BUILD.gn (right):

https://codereview.chromium.org/1327913004/diff/1/mojo/nacl/BUILD.gn#newcode23
mojo/nacl/BUILD.gn:23: executable("monacl_shell_nonsfi") {
I don't have a solution for this at the moment, but I just want to point out
that I do not like that this executable is being built with an x64 toolchain (if
the default is x64), even though it is a useless binary if it is built that way.

(executables build without anything depending on them, and this one must be
defined to say "build it again with a 32-bit toolchain" in the
monacl_shell_nonsfi_x86 group).

https://codereview.chromium.org/1327913004/diff/1/mojo/nacl/BUILD.gn#newcode36
mojo/nacl/BUILD.gn:36: group("monacl_shell_nonsfi_x86") {
On 2015/09/08 23:57:43, Mark Seaborn wrote:
> Should this be added as a dependency of group("mojo_nacl"), so that it's
> included in the set of targets built by the bots?  Currently the bots don't
> build it.
> 
> Also, I think this should be conditionalised on "x86-32 or x86-64", otherwise
> we'd attempt to build it when targeting ARM.

I think it should be a dependency of "mojo_nacl_tests", as it is a testonly
executable (and the regular monacl_shell is there too).

Also, adding the conditional to the deps -- this way, the target still exists
and can be referenced on ARM without throwing an error, it just doesn't build
anything.

https://codereview.chromium.org/1327913004/diff/1/mojo/nacl/BUILD.gn#newcode39
mojo/nacl/BUILD.gn:39:
":monacl_shell_nonsfi(//build/toolchain/linux:clang_x86)",
On 2015/09/08 23:57:43, Mark Seaborn wrote:
> Since this is Linux-specific, it seems like the target should be
conditionalised
> on Linux too.  Though I'm not sure whether the codebase as a whole is intended
> to support non-Linux OSes.
> 
> Is there a way to do this without assuming Clang?  i.e. Is there a way to ask
> for "the 32-bit variant of whatever toolchain we're using (whether it's GCC or
> Clang or something else)"?  If not, can you add a comment to explain the
> assumption and say it's not ideal?

Added a conditional on "is_linux".

Comment added.

[Sean Klein, 2015-09-09 17:55:45]

Sorry for fiddling with this so much -- Petr and I figured out how to use gn to
avoid building the 64 bit nonfi executable.

https://codereview.chromium.org/1327913004/diff/1/mojo/nacl/BUILD.gn
File mojo/nacl/BUILD.gn (right):

https://codereview.chromium.org/1327913004/diff/1/mojo/nacl/BUILD.gn#newcode23
mojo/nacl/BUILD.gn:23: executable("monacl_shell_nonsfi") {
On 2015/09/09 17:37:07, smklein1 wrote:
> I don't have a solution for this at the moment, but I just want to point out
> that I do not like that this executable is being built with an x64 toolchain
(if
> the default is x64), even though it is a useless binary if it is built that
way.
> 
> (executables build without anything depending on them, and this one must be
> defined to say "build it again with a 32-bit toolchain" in the
> monacl_shell_nonsfi_x86 group).

With the most recent patch, I've worked around this problem -- it is possible to
"deps" on a target which exists inside a conditional that has not yet been
evaluated.

[Mark Seaborn, 2015-09-09 19:13:14]

LGTM

https://codereview.chromium.org/1327913004/diff/60001/mojo/nacl/BUILD.gn
File mojo/nacl/BUILD.gn (right):

https://codereview.chromium.org/1327913004/diff/60001/mojo/nacl/BUILD.gn#newc...
mojo/nacl/BUILD.gn:23: if (current_cpu == "x86") {
Can you add a comment to explain the conditional?  e.g. Non-SFI NaCl currently
supports x86-32 but not x86-64 (and PNaCl pexes assume 32-bit pointers).

https://codereview.chromium.org/1327913004/diff/60001/mojo/nacl/BUILD.gn#newc...
mojo/nacl/BUILD.gn:41: if ((target_cpu == "x64" || target_cpu == "x86") &&
is_linux) {
A similar explanatory comment would be good here:  Even when Mojo is built for
x86-64 by default, we want to use x86-32 for running programs under Non-SFI
NaCl.

[Sean Klein, 2015-09-09 19:32:33]

https://codereview.chromium.org/1327913004/diff/60001/mojo/nacl/BUILD.gn
File mojo/nacl/BUILD.gn (right):

https://codereview.chromium.org/1327913004/diff/60001/mojo/nacl/BUILD.gn#newc...
mojo/nacl/BUILD.gn:23: if (current_cpu == "x86") {
On 2015/09/09 19:13:14, Mark Seaborn wrote:
> Can you add a comment to explain the conditional?  e.g. Non-SFI NaCl currently
> supports x86-32 but not x86-64 (and PNaCl pexes assume 32-bit pointers).

Done.

https://codereview.chromium.org/1327913004/diff/60001/mojo/nacl/BUILD.gn#newc...
mojo/nacl/BUILD.gn:41: if ((target_cpu == "x64" || target_cpu == "x86") &&
is_linux) {
On 2015/09/09 19:13:14, Mark Seaborn wrote:
> A similar explanatory comment would be good here:  Even when Mojo is built for
> x86-64 by default, we want to use x86-32 for running programs under Non-SFI
> NaCl.

Done.

[Sean Klein, 2015-09-09 20:25:05]

Committed patchset #5 (id:80001) manually as
7b0e143e9861e72663d31af92023953867d4909c (presubmit successful).