Build nacl_helper_nonsfi with GN

Build nacl_helper_nonsfi with GN

Support building nacl_helper_nonsfi under GN.

BUG=462791
Committed: https://crrev.com/636bceb3f0fc3062735203ef149e91736ec89024
Cr-Commit-Position: refs/heads/master@{#356234}

[Dirk Pranke, 2015-10-21 20:03:51]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dirk Pranke, 2015-10-21 20:03:52]

Some comments; obviously someone who understands something about what makes
_nonsfi tick needs to review this as well.

https://codereview.chromium.org/1395573003/diff/1/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/1/base/BUILD.gn#newcode677
base/BUILD.gn:677: if (!is_nacl_nonsfi) {
can you move these blocks next to the if (is_nacl) block on line 732 to try and
keep the nacl logic closer to being in one place?

https://codereview.chromium.org/1395573003/diff/1/base/BUILD.gn#newcode947
base/BUILD.gn:947: if (is_nacl_nonsfi) {
can we move this up next to the if (is_nacl) block also and consolidate the
logic 
so that we only remove these files if (is_nacl && !is_nacl_nonsfi). It's
confusing
to add them, then remove them, then add them back.

https://codereview.chromium.org/1395573003/diff/1/build/toolchain/gcc_toolcha...
File build/toolchain/gcc_toolchain.gni (right):

https://codereview.chromium.org/1395573003/diff/1/build/toolchain/gcc_toolcha...
build/toolchain/gcc_toolchain.gni:352: "is_nacl_nonsfi",
nit: sort these?

https://codereview.chromium.org/1395573003/diff/1/build/toolchain/nacl/BUILD.gn
File build/toolchain/nacl/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/1/build/toolchain/nacl/BUILD....
build/toolchain/nacl/BUILD.gn:177: #executable_extension = ".pexe"
why is the rest of this commented out?

https://codereview.chromium.org/1395573003/diff/1/content/BUILD.gn
File content/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/1/content/BUILD.gn#newcode66
content/BUILD.gn:66: } else if (is_nacl_nonsfi) {
there's a part of content that's built w/ the nacl_nonsfi toolchain? I'm
curious, why?

Also, should this be a source_set() instead of a static_library() ?

https://codereview.chromium.org/1395573003/diff/1/third_party/libevent/BUILD.gn
File third_party/libevent/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/1/third_party/libevent/BUILD....
third_party/libevent/BUILD.gn:34: } else if (is_nacl_nonsfi) {
you probably need to import //build/config/nacl/config.gni at the top of this
file.

[Petr Hosek, 2015-10-22 18:32:14]

https://codereview.chromium.org/1395573003/diff/1/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/1/base/BUILD.gn#newcode677
base/BUILD.gn:677: if (!is_nacl_nonsfi) {
On 2015/10/21 20:03:52, Dirk Pranke wrote:
> can you move these blocks next to the if (is_nacl) block on line 732 to try
and
> keep the nacl logic closer to being in one place?

Done.

https://codereview.chromium.org/1395573003/diff/1/base/BUILD.gn#newcode947
base/BUILD.gn:947: if (is_nacl_nonsfi) {
On 2015/10/21 20:03:52, Dirk Pranke wrote:
> can we move this up next to the if (is_nacl) block also and consolidate the
> logic 
> so that we only remove these files if (is_nacl && !is_nacl_nonsfi). It's
> confusing
> to add them, then remove them, then add them back.

Done.

https://codereview.chromium.org/1395573003/diff/1/build/toolchain/gcc_toolcha...
File build/toolchain/gcc_toolchain.gni (right):

https://codereview.chromium.org/1395573003/diff/1/build/toolchain/gcc_toolcha...
build/toolchain/gcc_toolchain.gni:352: "is_nacl_nonsfi",
On 2015/10/21 20:03:52, Dirk Pranke wrote:
> nit: sort these?

Done.

https://codereview.chromium.org/1395573003/diff/1/content/BUILD.gn
File content/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/1/content/BUILD.gn#newcode66
content/BUILD.gn:66: } else if (is_nacl_nonsfi) {
On 2015/10/21 20:03:52, Dirk Pranke wrote:
> there's a part of content that's built w/ the nacl_nonsfi toolchain? I'm
> curious, why?
> 
> Also, should this be a source_set() instead of a static_library() ?

Done. Why we build only a subset of content is a question onto Hidehiko or Mark,
I've followed the Gyp build.

https://codereview.chromium.org/1395573003/diff/1/third_party/libevent/BUILD.gn
File third_party/libevent/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/1/third_party/libevent/BUILD....
third_party/libevent/BUILD.gn:34: } else if (is_nacl_nonsfi) {
On 2015/10/21 20:03:52, Dirk Pranke wrote:
> you probably need to import //build/config/nacl/config.gni at the top of this
> file.

Done.

[Petr Hosek, 2015-10-22 18:32:45]

Description was changed from

==========
Build nacl_helper_nonsfi with GN

Support building nacl_helper_nonsfi under GN.

BUG=462791
==========

to

==========
Build nacl_helper_nonsfi with GN

Support building nacl_helper_nonsfi under GN.

BUG=462791
==========

[Petr Hosek, 2015-10-22 18:32:45]

phosek@chromium.org changed reviewers:
+ brettw@chromium.org, hidehiko@chromium.org, mseaborn@chromium.org

[Petr Hosek, 2015-10-22 18:32:51]

ptal

[Petr Hosek, 2015-10-22 19:10:25]

Just to clarify, nacl_helper_nonsfi builds with these changes but crashes
immediately after start. I haven't figured out the cause of the crash, but Dirk
suggested landing this change since he needs the nacl_helper_nonsfi binary to be
present (not to run though) and fix it in a subsequent change.

[Mark Seaborn, 2015-10-22 19:16:07]

On 22 October 2015 at 12:10, <phosek@chromium.org> wrote:

> Just to clarify, nacl_helper_nonsfi builds with these changes but crashes
> immediately after start. I haven't figured out the cause of the crash, but
> Dirk
> suggested landing this change since he needs the nacl_helper_nonsfi binary
> to be
> present (not to run though) and fix it in a subsequent change.


Can you put that information in the commit message, please?

Mark

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

[Mark Seaborn, 2015-10-22 21:48:31]

https://codereview.chromium.org/1395573003/diff/40001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/40001/base/BUILD.gn#newcode755
base/BUILD.gn:755: "process/kill_posix.cc",
Should this be handled in base/process/BUILD.gn instead?  kill_posix.cc is not
listed in base/BUILD.gn.

https://codereview.chromium.org/1395573003/diff/40001/build/config/compiler/B...
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/40001/build/config/compiler/B...
build/config/compiler/BUILD.gn:457: "x86-32-nonsfi",
I assume the GN formatter wants to put this on a separate line from '-arch'?

https://codereview.chromium.org/1395573003/diff/40001/build/config/nacl/BUILD.gn
File build/config/nacl/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/40001/build/config/nacl/BUILD...
build/config/nacl/BUILD.gn:54: "--pnacl-allow-native",
Is there a reason that some of these flags are in this file while others are in
build/config/compiler/BUILD.gn?

I suppose there is an intentional split from config("compiler_cpu_abi")?

https://codereview.chromium.org/1395573003/diff/40001/build/toolchain/nacl/BU...
File build/toolchain/nacl/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/40001/build/toolchain/nacl/BU...
build/toolchain/nacl/BUILD.gn:175: nacl_toolchain("newlib_pnacl_nonsfi") {
How about putting this immediately after nacl_toolchain("newlib_pnacl"), since
they share toolprefix?

https://codereview.chromium.org/1395573003/diff/40001/components/nacl/BUILD.gn
File components/nacl/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/40001/components/nacl/BUILD.g...
components/nacl/BUILD.gn:259: "-Wl,--noirt",
I think these flags (and "-lc++" etc.) belong in the toolchain description in
build/ rather than on the target.

This is because use of "--target=i686-unknown-nacl" tends to require use of
"-Wl,--noirt".

Also because "-Wl,--noirt" will be required for other executables built using
that toolchain definition, such as nacl_helper_nonsfi_unittests_{main,run}.  Can
you add those targets too?  Would it be worthwhile to add them in this change? 
Running those tests might help debug why your GN-built nacl_helper_nonsfi isn't
working.

https://codereview.chromium.org/1395573003/diff/40001/components/nacl/BUILD.g...
components/nacl/BUILD.gn:265: "c++",
libc++ should be brought in automatically by linking using pnacl-clang++.  libc
should also be brought in automatically.

Oh... this is because you're using "-nodefaultlibs".

untrusted.gypi has this comment:
      # The clang driver automatically injects -lpthread when using libc++, but
      # the toolchain doesn't have it yet.  To get around this, use
      # -nodefaultlibs and explicitly link irt_browser_lib to satisfy libc++'s
      # pthread dependency.

Can you put a similar explanation in the GN build?

https://codereview.chromium.org/1395573003/diff/40001/ipc/BUILD.gn
File ipc/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/40001/ipc/BUILD.gn#newcode126
ipc/BUILD.gn:126: sources += [
This mix of adding and removing sources is a bit confusing.

How about merging this with the "if (is_nacl)" block above?

https://codereview.chromium.org/1395573003/diff/40001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/40001/media/BUILD.gn#newcode856
media/BUILD.gn:856: "audio/audio_parameters.cc",
This doesn't seem to correspond to any conditionalisation in the Gyp build.

If this conditionalisation is needed, could you add a comment explaining why? 
e.g. Do sources not compile?

Do you know which of the conditionalisations you're adding match the Gyp build
and which don't?

https://codereview.chromium.org/1395573003/diff/40001/ppapi/proxy/BUILD.gn
File ppapi/proxy/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/40001/ppapi/proxy/BUILD.gn#ne...
ppapi/proxy/BUILD.gn:219: "../nacl_irt/irt_pnacl_translator_compile.cc",
Can you put "#if !defined(OS_NACL_NONSFI)" into these source files instead,
please?  That makes the conditionalisation build-system-agnostic.

https://codereview.chromium.org/1395573003/diff/40001/sandbox/linux/BUILD.gn
File sandbox/linux/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/40001/sandbox/linux/BUILD.gn#...
sandbox/linux/BUILD.gn:419: # this is the subset of linux build target, needed
for
Nit: Capitalise as "This"

[Petr Hosek, 2015-10-23 03:01:15]

https://codereview.chromium.org/1395573003/diff/40001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/40001/base/BUILD.gn#newcode755
base/BUILD.gn:755: "process/kill_posix.cc",
On 2015/10/22 21:48:30, Mark Seaborn wrote:
> Should this be handled in base/process/BUILD.gn instead?  kill_posix.cc is not
> listed in base/BUILD.gn.

Done.

https://codereview.chromium.org/1395573003/diff/40001/build/config/compiler/B...
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/40001/build/config/compiler/B...
build/config/compiler/BUILD.gn:457: "x86-32-nonsfi",
On 2015/10/22 21:48:30, Mark Seaborn wrote:
> I assume the GN formatter wants to put this on a separate line from '-arch'?

Yes.

https://codereview.chromium.org/1395573003/diff/40001/build/config/nacl/BUILD.gn
File build/config/nacl/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/40001/build/config/nacl/BUILD...
build/config/nacl/BUILD.gn:54: "--pnacl-allow-native",
On 2015/10/22 21:48:30, Mark Seaborn wrote:
> Is there a reason that some of these flags are in this file while others are
in
> build/config/compiler/BUILD.gn?
> 
> I suppose there is an intentional split from config("compiler_cpu_abi")?

Yes, I tried to follow the existing conventions.

https://codereview.chromium.org/1395573003/diff/40001/build/toolchain/nacl/BU...
File build/toolchain/nacl/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/40001/build/toolchain/nacl/BU...
build/toolchain/nacl/BUILD.gn:175: nacl_toolchain("newlib_pnacl_nonsfi") {
On 2015/10/22 21:48:30, Mark Seaborn wrote:
> How about putting this immediately after nacl_toolchain("newlib_pnacl"), since
> they share toolprefix?

Done.

https://codereview.chromium.org/1395573003/diff/40001/components/nacl/BUILD.gn
File components/nacl/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/40001/components/nacl/BUILD.g...
components/nacl/BUILD.gn:259: "-Wl,--noirt",
On 2015/10/22 21:48:30, Mark Seaborn wrote:
> I think these flags (and "-lc++" etc.) belong in the toolchain description in
> build/ rather than on the target.
> 
> This is because use of "--target=i686-unknown-nacl" tends to require use of
> "-Wl,--noirt".
> 
> Also because "-Wl,--noirt" will be required for other executables built using
> that toolchain definition, such as nacl_helper_nonsfi_unittests_{main,run}. 
Can
> you add those targets too?  Would it be worthwhile to add them in this change?

> Running those tests might help debug why your GN-built nacl_helper_nonsfi
isn't
> working.

Done.

Yes, I started porting the unit tests as well (exactly for the reason you
mentioned), but they're not finished yet and Dirk asked me to land the
nacl_helper_nonsfi ASAP because it's blocking other things. I planned to add
unit tests in a follow up issue.

https://codereview.chromium.org/1395573003/diff/40001/components/nacl/BUILD.g...
components/nacl/BUILD.gn:265: "c++",
On 2015/10/22 21:48:30, Mark Seaborn wrote:
> libc++ should be brought in automatically by linking using pnacl-clang++. 
libc
> should also be brought in automatically.
> 
> Oh... this is because you're using "-nodefaultlibs".
> 
> untrusted.gypi has this comment:
>       # The clang driver automatically injects -lpthread when using libc++,
but
>       # the toolchain doesn't have it yet.  To get around this, use
>       # -nodefaultlibs and explicitly link irt_browser_lib to satisfy libc++'s
>       # pthread dependency.
> 
> Can you put a similar explanation in the GN build?

Done.

https://codereview.chromium.org/1395573003/diff/40001/ipc/BUILD.gn
File ipc/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/40001/ipc/BUILD.gn#newcode126
ipc/BUILD.gn:126: sources += [
On 2015/10/22 21:48:30, Mark Seaborn wrote:
> This mix of adding and removing sources is a bit confusing.
> 
> How about merging this with the "if (is_nacl)" block above?

Done.

https://codereview.chromium.org/1395573003/diff/40001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/40001/media/BUILD.gn#newcode856
media/BUILD.gn:856: "audio/audio_parameters.cc",
On 2015/10/22 21:48:30, Mark Seaborn wrote:
> This doesn't seem to correspond to any conditionalisation in the Gyp build.
> 
> If this conditionalisation is needed, could you add a comment explaining why? 
> e.g. Do sources not compile?
> 
> Do you know which of the conditionalisations you're adding match the Gyp build
> and which don't?

shared_memory_support_nacl in Gyp includes some bits of geometry which is
different to how shared_memory_support is defined in GN. These 4 files are the
only one which actually require geometry and they don't seem to be needed.
However, geometry seems to build fine with pnacl-clang so I can drop this
condition altogether.

https://codereview.chromium.org/1395573003/diff/40001/ppapi/proxy/BUILD.gn
File ppapi/proxy/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/40001/ppapi/proxy/BUILD.gn#ne...
ppapi/proxy/BUILD.gn:219: "../nacl_irt/irt_pnacl_translator_compile.cc",
On 2015/10/22 21:48:30, Mark Seaborn wrote:
> Can you put "#if !defined(OS_NACL_NONSFI)" into these source files instead,
> please?  That makes the conditionalisation build-system-agnostic.

Done.

https://codereview.chromium.org/1395573003/diff/40001/sandbox/linux/BUILD.gn
File sandbox/linux/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/40001/sandbox/linux/BUILD.gn#...
sandbox/linux/BUILD.gn:419: # this is the subset of linux build target, needed
for
On 2015/10/22 21:48:30, Mark Seaborn wrote:
> Nit: Capitalise as "This"

Done.

[Petr Hosek, 2015-10-23 03:17:11]

On 2015/10/23 03:01:15, Petr Hosek wrote:
> https://codereview.chromium.org/1395573003/diff/40001/base/BUILD.gn
> File base/BUILD.gn (right):
> 
> https://codereview.chromium.org/1395573003/diff/40001/base/BUILD.gn#newcode755
> base/BUILD.gn:755: "process/kill_posix.cc",
> On 2015/10/22 21:48:30, Mark Seaborn wrote:
> > Should this be handled in base/process/BUILD.gn instead?  kill_posix.cc is
not
> > listed in base/BUILD.gn.
> 
> Done.
> 
>
https://codereview.chromium.org/1395573003/diff/40001/build/config/compiler/B...
> File build/config/compiler/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1395573003/diff/40001/build/config/compiler/B...
> build/config/compiler/BUILD.gn:457: "x86-32-nonsfi",
> On 2015/10/22 21:48:30, Mark Seaborn wrote:
> > I assume the GN formatter wants to put this on a separate line from '-arch'?
> 
> Yes.
> 
>
https://codereview.chromium.org/1395573003/diff/40001/build/config/nacl/BUILD.gn
> File build/config/nacl/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1395573003/diff/40001/build/config/nacl/BUILD...
> build/config/nacl/BUILD.gn:54: "--pnacl-allow-native",
> On 2015/10/22 21:48:30, Mark Seaborn wrote:
> > Is there a reason that some of these flags are in this file while others are
> in
> > build/config/compiler/BUILD.gn?
> > 
> > I suppose there is an intentional split from config("compiler_cpu_abi")?
> 
> Yes, I tried to follow the existing conventions.
> 
>
https://codereview.chromium.org/1395573003/diff/40001/build/toolchain/nacl/BU...
> File build/toolchain/nacl/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1395573003/diff/40001/build/toolchain/nacl/BU...
> build/toolchain/nacl/BUILD.gn:175: nacl_toolchain("newlib_pnacl_nonsfi") {
> On 2015/10/22 21:48:30, Mark Seaborn wrote:
> > How about putting this immediately after nacl_toolchain("newlib_pnacl"),
since
> > they share toolprefix?
> 
> Done.
> 
> https://codereview.chromium.org/1395573003/diff/40001/components/nacl/BUILD.gn
> File components/nacl/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1395573003/diff/40001/components/nacl/BUILD.g...
> components/nacl/BUILD.gn:259: "-Wl,--noirt",
> On 2015/10/22 21:48:30, Mark Seaborn wrote:
> > I think these flags (and "-lc++" etc.) belong in the toolchain description
in
> > build/ rather than on the target.
> > 
> > This is because use of "--target=i686-unknown-nacl" tends to require use of
> > "-Wl,--noirt".
> > 
> > Also because "-Wl,--noirt" will be required for other executables built
using
> > that toolchain definition, such as nacl_helper_nonsfi_unittests_{main,run}. 
> Can
> > you add those targets too?  Would it be worthwhile to add them in this
change?
> 
> > Running those tests might help debug why your GN-built nacl_helper_nonsfi
> isn't
> > working.
> 
> Done.
> 
> Yes, I started porting the unit tests as well (exactly for the reason you
> mentioned), but they're not finished yet and Dirk asked me to land the
> nacl_helper_nonsfi ASAP because it's blocking other things. I planned to add
> unit tests in a follow up issue.
> 
>
https://codereview.chromium.org/1395573003/diff/40001/components/nacl/BUILD.g...
> components/nacl/BUILD.gn:265: "c++",
> On 2015/10/22 21:48:30, Mark Seaborn wrote:
> > libc++ should be brought in automatically by linking using pnacl-clang++. 
> libc
> > should also be brought in automatically.
> > 
> > Oh... this is because you're using "-nodefaultlibs".
> > 
> > untrusted.gypi has this comment:
> >       # The clang driver automatically injects -lpthread when using libc++,
> but
> >       # the toolchain doesn't have it yet.  To get around this, use
> >       # -nodefaultlibs and explicitly link irt_browser_lib to satisfy
libc++'s
> >       # pthread dependency.
> > 
> > Can you put a similar explanation in the GN build?
> 
> Done.
> 
> https://codereview.chromium.org/1395573003/diff/40001/ipc/BUILD.gn
> File ipc/BUILD.gn (right):
> 
> https://codereview.chromium.org/1395573003/diff/40001/ipc/BUILD.gn#newcode126
> ipc/BUILD.gn:126: sources += [
> On 2015/10/22 21:48:30, Mark Seaborn wrote:
> > This mix of adding and removing sources is a bit confusing.
> > 
> > How about merging this with the "if (is_nacl)" block above?
> 
> Done.
> 
> https://codereview.chromium.org/1395573003/diff/40001/media/BUILD.gn
> File media/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1395573003/diff/40001/media/BUILD.gn#newcode856
> media/BUILD.gn:856: "audio/audio_parameters.cc",
> On 2015/10/22 21:48:30, Mark Seaborn wrote:
> > This doesn't seem to correspond to any conditionalisation in the Gyp build.
> > 
> > If this conditionalisation is needed, could you add a comment explaining
why? 
> > e.g. Do sources not compile?
> > 
> > Do you know which of the conditionalisations you're adding match the Gyp
build
> > and which don't?
> 
> shared_memory_support_nacl in Gyp includes some bits of geometry which is
> different to how shared_memory_support is defined in GN. These 4 files are the
> only one which actually require geometry and they don't seem to be needed.
> However, geometry seems to build fine with pnacl-clang so I can drop this
> condition altogether.
> 
> https://codereview.chromium.org/1395573003/diff/40001/ppapi/proxy/BUILD.gn
> File ppapi/proxy/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1395573003/diff/40001/ppapi/proxy/BUILD.gn#ne...
> ppapi/proxy/BUILD.gn:219: "../nacl_irt/irt_pnacl_translator_compile.cc",
> On 2015/10/22 21:48:30, Mark Seaborn wrote:
> > Can you put "#if !defined(OS_NACL_NONSFI)" into these source files instead,
> > please?  That makes the conditionalisation build-system-agnostic.
> 
> Done.
> 
> https://codereview.chromium.org/1395573003/diff/40001/sandbox/linux/BUILD.gn
> File sandbox/linux/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1395573003/diff/40001/sandbox/linux/BUILD.gn#...
> sandbox/linux/BUILD.gn:419: # this is the subset of linux build target, needed
> for
> On 2015/10/22 21:48:30, Mark Seaborn wrote:
> > Nit: Capitalise as "This"
> 
> Done.

So, after incorporating all the changes, nacl_helper_nonsfi have "magically"
started working and the Non-SFI tests (issue 1392973006) are now passing,
woohoo!

[Dirk Pranke, 2015-10-23 04:11:09]

On 2015/10/23 03:17:11, Petr Hosek wrote:
> So, after incorporating all the changes, nacl_helper_nonsfi have "magically"
> started working and the Non-SFI tests (issue 1392973006) are now passing,
> woohoo!

The magic of code reviews!

[Dirk Pranke, 2015-10-23 04:18:40]

*.gn* changes lgtm.

https://codereview.chromium.org/1395573003/diff/100001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/100001/base/BUILD.gn#newcode685
base/BUILD.gn:685: "//base/metrics",
nit: re-sort this (not sure why it moved?)

[brettw, 2015-10-23 09:38:13]

https://codereview.chromium.org/1395573003/diff/100001/build/config/compiler/...
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/100001/build/config/compiler/...
build/config/compiler/BUILD.gn:786: if (is_nacl_nonsfi) {
This block seems very strange to me. Why are we disabling all of these warnings
for this nacl variant? I don't see anything intrinsic in this build that
indicates we should disable things like sign compares or unused functions when
it works well otherwise.

[Petr Hosek, 2015-10-23 17:53:07]

https://codereview.chromium.org/1395573003/diff/100001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/100001/base/BUILD.gn#newcode685
base/BUILD.gn:685: "//base/metrics",
On 2015/10/23 04:18:40, Dirk Pranke wrote:
> nit: re-sort this (not sure why it moved?)

Done.

https://codereview.chromium.org/1395573003/diff/100001/build/config/compiler/...
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/100001/build/config/compiler/...
build/config/compiler/BUILD.gn:786: if (is_nacl_nonsfi) {
On 2015/10/23 09:38:13, brettw wrote:
> This block seems very strange to me. Why are we disabling all of these
warnings
> for this nacl variant? I don't see anything intrinsic in this build that
> indicates we should disable things like sign compares or unused functions when
> it works well otherwise.

I originally copied the configuration from Gyp but you're right, most of these
are not needed. Actually, it turned it's just the `-Wno-unused-function` and
`-Wno-sign-compare` which is really needed. The first one needs some fixes
inside seccomp-bfp, the second one needs a trivial fix on the NaCl side. I added
appropriate TODOs.

[brettw, 2015-10-24 13:05:12]

If there are a few warnings in a target, we should be setting those warnings on
the target with the bug, rather than every target in the build under that
configuration. Depending on the ordering of flags you need, you may need to put
the cflags on a config and append it rather than setting cflags directly on the
target. See third_party/libpng/BUILD.gn for an example of a warning cflag used
for clang.

[Mark Seaborn, 2015-10-24 17:07:49]

LGTM

https://codereview.chromium.org/1395573003/diff/120001/build/config/nacl/BUIL...
File build/config/nacl/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/120001/build/config/nacl/BUIL...
build/config/nacl/BUILD.gn:64: # -nodefaultlibs and explicitly link
irt_browser_lib to satisfy libc++'s
This part isn't linking irt_browser_lib, unlike the Gyp build.

Instead, perhaps say that we have each target depend on
"//native_client/src/nonsfi/irt:nacl_sys_private".

[Petr Hosek, 2015-10-24 22:11:49]

https://codereview.chromium.org/1395573003/diff/120001/build/config/nacl/BUIL...
File build/config/nacl/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/120001/build/config/nacl/BUIL...
build/config/nacl/BUILD.gn:64: # -nodefaultlibs and explicitly link
irt_browser_lib to satisfy libc++'s
On 2015/10/24 17:07:49, Mark Seaborn wrote:
> This part isn't linking irt_browser_lib, unlike the Gyp build.
> 
> Instead, perhaps say that we have each target depend on
> "//native_client/src/nonsfi/irt:nacl_sys_private".

Done.

[Petr Hosek, 2015-10-24 22:13:57]

On 2015/10/24 22:11:49, Petr Hosek wrote:
>
https://codereview.chromium.org/1395573003/diff/120001/build/config/nacl/BUIL...
> File build/config/nacl/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1395573003/diff/120001/build/config/nacl/BUIL...
> build/config/nacl/BUILD.gn:64: # -nodefaultlibs and explicitly link
> irt_browser_lib to satisfy libc++'s
> On 2015/10/24 17:07:49, Mark Seaborn wrote:
> > This part isn't linking irt_browser_lib, unlike the Gyp build.
> > 
> > Instead, perhaps say that we have each target depend on
> > "//native_client/src/nonsfi/irt:nacl_sys_private".
> 
> Done.

`-Wno-unused-function` is now set per target. I've fixed NaCl
(https://codereview.chromium.org/1419373002/) so once that change lands and gets
rolled into Chrome, we can also remove the `-Wno-sign-compare`.

[brettw, 2015-10-25 08:45:33]

https://codereview.chromium.org/1395573003/diff/140001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/140001/base/BUILD.gn#newcode33
base/BUILD.gn:33: config("clang_warnings") {
You just copy-and-pasted the old config name :)

This name should be something like "nacl_nonsfi_warnings"

https://codereview.chromium.org/1395573003/diff/140001/build/config/compiler/...
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/140001/build/config/compiler/...
build/config/compiler/BUILD.gn:789: "-Wno-sign-compare",
Since this is already fixed, I'm OK adding is a temporary measure. But can this
comment be specific about when this can be removed (i.e. link to the code review
and say it can be removed when that's rolled in) in case you forget and somebody
comes along later looking at it.

https://codereview.chromium.org/1395573003/diff/140001/build/toolchain/nacl/B...
File build/toolchain/nacl/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/140001/build/toolchain/nacl/B...
build/toolchain/nacl/BUILD.gn:63: executable_extension = ""
I think this should default to empty, right? I think the other toolchains don't
set it.

https://codereview.chromium.org/1395573003/diff/140001/components/nacl/BUILD.gn
File components/nacl/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/140001/components/nacl/BUILD....
components/nacl/BUILD.gn:238: if (is_nacl) {
Do we want these for all nacl variants? It seems like we would want to wrap this
in a nonsfi conditional so we don't compile it in other cases.

https://codereview.chromium.org/1395573003/diff/140001/components/nacl/BUILD....
components/nacl/BUILD.gn:300: group("nacl_helper_nonsfi") {
The way this is set up is a bit confused. There are two ways I could see doing
this, and the way it's done is kind of half of both.

Way #1 is to have one nacl_helper_nonsfi target that's compiled only in the
nonsfi toolchain. In that case, the copy step above would be renamed
nacl_helper_nonsfi.

Way #2 is to have a "nacl_helper_nonsfi" trampoline in all toolchains that then
link to the nacl_helper_nonsfi_copy step in the nonsfi toolchain. In that case,
this group should be brought out of the is_nacl conditional, and then all
references to it can have the explicit toolchain reference removed.

I'm partial to way #2 since it centralizes knowledge of the nonsfi toolchain.

[brettw, 2015-10-25 08:58:08]

https://codereview.chromium.org/1395573003/diff/140001/components/nacl/BUILD.gn
File components/nacl/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/140001/components/nacl/BUILD....
components/nacl/BUILD.gn:307: group("nacl") {
Also, in either case I mentioned above, you can delete the contents of this
else. The nacl_helper targets (except the trampoline in #2 that is run in all
cases complete outside of this conditional) will be referenced only in the
correct toolchain.

[Petr Hosek, 2015-10-25 22:50:49]

All the feedback has been incorporated, I've also reworked the build of
//sandbox/linux:sandbox; instead of having a custom target
sandbox_linux_nacl_nonsfi I just modified sandbox to remove the files not used
under nonsfi_nacl which I believe is the preferred way in GN (I'm happy to
revert this if not)?

However, with the changes to nacl_helper_nonsfi target setup, I now have a
problem, ninja is complaining that 'multiple rules generate nacl_helper_nonsfi'.
I assume this is because both the nacl_helper_nonsfi_copy and nacl_helper_nonsfi
produce the same output file? If I rename nacl_helper_nonsfi to something like
nacl_helper_nonsfi_group, the warning goes away but that looks kind of icky; is
there a better way to solve this?

https://codereview.chromium.org/1395573003/diff/140001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/140001/base/BUILD.gn#newcode33
base/BUILD.gn:33: config("clang_warnings") {
On 2015/10/25 08:45:33, brettw wrote:
> You just copy-and-pasted the old config name :)
> 
> This name should be something like "nacl_nonsfi_warnings"

Yes and no, I kept the name because it's in fact a Clang warning (which Non-SFI
mode uses) but I agree this could be a bit more specific; done.

https://codereview.chromium.org/1395573003/diff/140001/build/config/compiler/...
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/140001/build/config/compiler/...
build/config/compiler/BUILD.gn:789: "-Wno-sign-compare",
On 2015/10/25 08:45:33, brettw wrote:
> Since this is already fixed, I'm OK adding is a temporary measure. But can
this
> comment be specific about when this can be removed (i.e. link to the code
review
> and say it can be removed when that's rolled in) in case you forget and
somebody
> comes along later looking at it.

Done.

https://codereview.chromium.org/1395573003/diff/140001/build/toolchain/nacl/B...
File build/toolchain/nacl/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/140001/build/toolchain/nacl/B...
build/toolchain/nacl/BUILD.gn:63: executable_extension = ""
On 2015/10/25 08:45:33, brettw wrote:
> I think this should default to empty, right? I think the other toolchains
don't
> set it.

nacl_toolchain sets this to ".nexe" unless it's overridden by the invoker.

https://codereview.chromium.org/1395573003/diff/140001/components/nacl/BUILD.gn
File components/nacl/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/140001/components/nacl/BUILD....
components/nacl/BUILD.gn:238: if (is_nacl) {
On 2015/10/25 08:45:33, brettw wrote:
> Do we want these for all nacl variants? It seems like we would want to wrap
this
> in a nonsfi conditional so we don't compile it in other cases.

Done.

https://codereview.chromium.org/1395573003/diff/140001/components/nacl/BUILD....
components/nacl/BUILD.gn:300: group("nacl_helper_nonsfi") {
On 2015/10/25 08:45:33, brettw wrote:
> The way this is set up is a bit confused. There are two ways I could see doing
> this, and the way it's done is kind of half of both.
> 
> Way #1 is to have one nacl_helper_nonsfi target that's compiled only in the
> nonsfi toolchain. In that case, the copy step above would be renamed
> nacl_helper_nonsfi.
> 
> Way #2 is to have a "nacl_helper_nonsfi" trampoline in all toolchains that
then
> link to the nacl_helper_nonsfi_copy step in the nonsfi toolchain. In that
case,
> this group should be brought out of the is_nacl conditional, and then all
> references to it can have the explicit toolchain reference removed.
> 
> I'm partial to way #2 since it centralizes knowledge of the nonsfi toolchain.

Done; I went with the option #2.

https://codereview.chromium.org/1395573003/diff/140001/components/nacl/BUILD....
components/nacl/BUILD.gn:307: group("nacl") {
On 2015/10/25 08:58:08, brettw wrote:
> Also, in either case I mentioned above, you can delete the contents of this
> else. The nacl_helper targets (except the trampoline in #2 that is run in all
> cases complete outside of this conditional) will be referenced only in the
> correct toolchain.

Done.

[brettw, 2015-10-26 00:49:47]

lgtm

https://codereview.chromium.org/1395573003/diff/160001/build/config/nacl/conf...
File build/config/nacl/config.gni (right):

https://codereview.chromium.org/1395573003/diff/160001/build/config/nacl/conf...
build/config/nacl/config.gni:10: is_nacl_nonsfi = false
This can just be defined outside of the declare args block (i.e. at the end of
this file) as:
  is_nacl_nonsfi = current_toolchain ==
"//build/toolchain/nacl:newlib_pnacl_nonsfi"
Then you can delete the gcc_toolchain plumbing for this flag.

Also, this needs a comment (I would mention what nonsfi actually is and what
it's used for).

https://codereview.chromium.org/1395573003/diff/160001/components/nacl/BUILD.gn
File components/nacl/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/160001/components/nacl/BUILD....
components/nacl/BUILD.gn:302: group("nacl_helper_nonsfi") {
This *should* work so I filed
https://code.google.com/p/chromium/issues/detail?id=547610
for what I think is the problem.

For now, maybe rename this group? Actually, GN style would be to name this
"helper_nonsfi" anyway since it avoids duplicating the name of the directory. I
believe this will fix the collision.

[Petr Hosek, 2015-10-26 01:55:11]

https://codereview.chromium.org/1395573003/diff/160001/build/config/nacl/conf...
File build/config/nacl/config.gni (right):

https://codereview.chromium.org/1395573003/diff/160001/build/config/nacl/conf...
build/config/nacl/config.gni:10: is_nacl_nonsfi = false
On 2015/10/26 00:49:47, brettw wrote:
> This can just be defined outside of the declare args block (i.e. at the end of
> this file) as:
>   is_nacl_nonsfi = current_toolchain ==
> "//build/toolchain/nacl:newlib_pnacl_nonsfi"
> Then you can delete the gcc_toolchain plumbing for this flag.
> 
> Also, this needs a comment (I would mention what nonsfi actually is and what
> it's used for).

Done.

https://codereview.chromium.org/1395573003/diff/160001/components/nacl/BUILD.gn
File components/nacl/BUILD.gn (right):

https://codereview.chromium.org/1395573003/diff/160001/components/nacl/BUILD....
components/nacl/BUILD.gn:302: group("nacl_helper_nonsfi") {
On 2015/10/26 00:49:47, brettw wrote:
> This *should* work so I filed
> https://code.google.com/p/chromium/issues/detail?id=547610
> for what I think is the problem.
> 
> For now, maybe rename this group? Actually, GN style would be to name this
> "helper_nonsfi" anyway since it avoids duplicating the name of the directory.
I
> believe this will fix the collision.

Done.

[hidehiko, 2015-10-26 08:48:45]

lgtm

LGTM

[Petr Hosek, 2015-10-27 02:52:41]

The CQ bit was checked by phosek@chromium.org

[Petr Hosek, 2015-10-27 02:52:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, mseaborn@chromium.org, brettw@chromium.org,
hidehiko@chromium.org
Link to the patchset: https://codereview.chromium.org/1395573003/#ps200001
(title: "Rebase")

[commit-bot: I haz the power, 2015-10-27 02:53:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1395573003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1395573003/200001

[commit-bot: I haz the power, 2015-10-27 03:36:55]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-10-27 03:38:04]

Patchset 11 (id:??) landed as
https://crrev.com/636bceb3f0fc3062735203ef149e91736ec89024
Cr-Commit-Position: refs/heads/master@{#356234}