Fix more aspects of the NaCl build configs.

Fix more aspects of the NaCl build configs.

As part of the un-forking of the NaCl GN build configs, we need
to be a little more careful about which NaCl toolchains are considered
to be "clang" or not and whether some of the clang-specific compiler
settings need to be applied in the NaCl contexts.

Also, remove more stray references to the old Nacl build configs.

R=brettw@chromium.org, mcgrathr@chromium.org, thakis@chromium.org
BUG=433528
Committed: https://crrev.com/b8c6bf7592cf6cb412f2d3f151bc29bd96ee7380
Cr-Commit-Position: refs/heads/master@{#347262}

[Nico, 2015-09-03 05:32:00]

The changes in build/config/compiler/BUILD.gn look wrong to me. Why are
they needed? From what I understand, NaCl sometimes uses clang, their clang
revision just trails ours a bit. But it's new enough to understand
-fcolor-diagnostics. Likewise, NaCl should need -std=gnu++11 independently
of it using clang or gcc. (But I'm not very familiar with how the NaCl
build is hooked up in gn; maybe I'm missing something)
On Sep 2, 2015 3:32 PM, <dpranke@chromium.org> wrote:

> Reviewers: brettw, Roland McGrath, Nico,
>
> Description:
> Fix more aspects of the NaCl build configs.
>
> As part of the un-forking of the NaCl GN build configs, we need
> to be a little more careful about which NaCl toolchains are considered
> to be "clang" or not and whether some of the clang-specific compiler
> settings need to be applied in the NaCl contexts.
>
> Also, remove more stray references to the old Nacl build configs.
>
> R=brettw@chromium.org, mcgrathr@chromium.org, thakis@chromium.org
> BUG=433528
>
> Please review this at https://codereview.chromium.org/1307653008/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+9, -14 lines):
>   M .gn
>   M build/config/compiler/BUILD.gn
>   M build/toolchain/nacl/BUILD.gn
>   M ppapi/BUILD.gn
>
>
> Index: .gn
> diff --git a/.gn b/.gn
> index
>
ad3f726c4a09daa8e93775efbaa57af6e71f14f5..5b010e2ecd08688a5eea194da1526536810196b7
> 100644
> --- a/.gn
> +++ b/.gn
> @@ -168,17 +168,6 @@ exec_script_whitelist = [
>    "//google_apis/BUILD.gn",
>    "//gpu/gles2_conform_support/BUILD.gn",
>    "//jingle/BUILD.gn",
> -  "//native_client/build/config/android/BUILD.gn",
> -  "//native_client/build/config/gcc/gcc_version.gni",
> -  "//native_client/build/config/ios/ios_sdk.gni",
> -  "//native_client/build/config/linux/BUILD.gn",
> -  "//native_client/build/config/linux/pkg_config.gni",
> -  "//native_client/build/config/mac/mac_sdk.gni",
> -  "//native_client/build/config/win/visual_studio_version.gni",
> -  "//native_client/build/toolchain/gcc_toolchain.gni",
> -  "//native_client/build/toolchain/mac/BUILD.gn",
> -  "//native_client/build/toolchain/nacl/BUILD.gn",
> -  "//native_client/build/toolchain/win/BUILD.gn",
>    "//net/BUILD.gn",
>    "//ppapi/ppapi_sources.gni",
>    "//printing/BUILD.gn",
> Index: build/config/compiler/BUILD.gn
> diff --git a/build/config/compiler/BUILD.gn
> b/build/config/compiler/BUILD.gn
> index
>
e7af924c4dc5703a2ea49ce18f1444b829262161..1c8e55d0d485caf9fc55eb3c539b3d4095b5ab1e
> 100644
> --- a/build/config/compiler/BUILD.gn
> +++ b/build/config/compiler/BUILD.gn
> @@ -516,13 +516,13 @@ config("compiler") {
>
>    # Clang-specific compiler flags setup.
>    # ------------------------------------
> -  if (is_clang) {
> +  if (is_clang && !is_nacl) {
>      cflags += [ "-fcolor-diagnostics" ]
>    }
>
>    # C++11 compiler flags setup.
>    # ---------------------------
> -  if (is_linux || is_android || is_nacl) {
> +  if (is_linux || is_android || (is_nacl && is_clang)) {
>      # gnu++11 instead of c++11 is needed because some code uses typeof()
> (a
>      # GNU extension).
>      # TODO(thakis): Eventually switch this to c++11 instead,
> Index: build/toolchain/nacl/BUILD.gn
> diff --git a/build/toolchain/nacl/BUILD.gn b/build/toolchain/nacl/BUILD.gn
> index
>
1a30bb9a8f88e4d4912dbcdca3da70d19679a65c..1013e1560ad864137c1c6996af90dc454ad897cb
> 100644
> --- a/build/toolchain/nacl/BUILD.gn
> +++ b/build/toolchain/nacl/BUILD.gn
> @@ -33,6 +33,7 @@ nacl_toolchain("newlib_arm") {
>    toolchain_revision = nacl_arm_newlib_rev
>    toolchain_cpu = "arm"
>    toolprefix = "${os_toolchain_dir}/${toolchain_package}/bin/arm-nacl-"
> +  is_clang = false
>
>    cc = toolprefix + "gcc"
>    cxx = toolprefix + "g++"
> @@ -45,6 +46,7 @@ nacl_toolchain("newlib_x86") {
>    toolchain_revision = nacl_x86_newlib_rev
>    toolchain_cpu = "x86"
>    toolprefix = "${os_toolchain_dir}/${toolchain_package}/bin/i686-nacl-"
> +  is_clang = false
>
>    cc = toolprefix + "gcc"
>    cxx = toolprefix + "g++"
> @@ -57,6 +59,7 @@ nacl_toolchain("newlib_x64") {
>    toolchain_revision = nacl_x86_newlib_rev
>    toolchain_cpu = "x64"
>    toolprefix = "${os_toolchain_dir}/${toolchain_package}/bin/x86_64-nacl-"
> +  is_clang = false
>
>    cc = toolprefix + "gcc"
>    cxx = toolprefix + "g++"
> @@ -89,6 +92,7 @@ nacl_toolchain("glibc_x86") {
>    toolchain_revision = nacl_x86_glibc_rev
>    toolchain_cpu = "x86"
>    toolprefix = "${os_toolchain_dir}/${toolchain_package}/bin/i686-nacl-"
> +  is_clang = false
>
>    cc = toolprefix + "gcc"
>    cxx = toolprefix + "g++"
> @@ -101,6 +105,7 @@ nacl_toolchain("glibc_x64") {
>    toolchain_revision = nacl_x86_glibc_rev
>    toolchain_cpu = "x64"
>    toolprefix = "${os_toolchain_dir}/${toolchain_package}/bin/x86_64-nacl-"
> +  is_clang = false
>
>    cc = toolprefix + "gcc"
>    cxx = toolprefix + "g++"
> @@ -186,6 +191,7 @@ nacl_toolchain("irt_arm") {
>    toolchain_revision = nacl_arm_newlib_rev
>    toolchain_cpu = "arm"
>    toolprefix = "${os_toolchain_dir}/${toolchain_package}/bin/arm-nacl-"
> +  is_clang = false
>
>    cc = toolprefix + "gcc"
>    cxx = toolprefix + "g++"
> Index: ppapi/BUILD.gn
> diff --git a/ppapi/BUILD.gn b/ppapi/BUILD.gn
> index
>
07eafbfb9870367fbc74c84dc8605af7c5afd2a2..d76cdbf3090349c00880fb5d7663a15d57732caf
> 100644
> --- a/ppapi/BUILD.gn
> +++ b/ppapi/BUILD.gn
> @@ -200,7 +200,7 @@ if (host_os != "win") {
>    group("ppapi_nacl_tests") {
>      deps = []
>      if (target_cpu == "x86" || target_cpu == "x64") {
> -      deps += [
>
":nacl_tests_copy(//native_client/build/toolchain/nacl:clang_newlib_${target_cpu})"
> ]
> +      deps += [
> ":nacl_tests_copy(//build/toolchain/nacl:clang_newlib_${target_cpu})" ]
>      }
>    }
>  }
>
>
>

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

[Dirk Pranke, 2015-09-03 15:36:10]

On 2015/09/03 05:32:00, Nico (vacation Thu Sep 2) wrote:
> The changes in build/config/compiler/BUILD.gn look wrong to me. Why are
> they needed? From what I understand, NaCl sometimes uses clang, their clang
> revision just trails ours a bit. But it's new enough to understand
> -fcolor-diagnostics. Likewise, NaCl should need -std=gnu++11 independently
> of it using clang or gcc. (But I'm not very familiar with how the NaCl
> build is hooked up in gn; maybe I'm missing something)

I will double-check to make sure I didn't miss something (since I already got
this wrong
once), but I'm pretty sure the toolchain compilers complained about not
recognizing
either of these flags. I will report back with my findings, but maybe Roland or
Petr
can comment as well.

[Dirk Pranke, 2015-09-03 20:24:31]

On 2015/09/03 15:36:10, Dirk Pranke wrote:
> On 2015/09/03 05:32:00, Nico (vacation Thu Sep 2) wrote:
> > The changes in build/config/compiler/BUILD.gn look wrong to me. Why are
> > they needed? From what I understand, NaCl sometimes uses clang, their clang
> > revision just trails ours a bit. But it's new enough to understand
> > -fcolor-diagnostics. Likewise, NaCl should need -std=gnu++11 independently
> > of it using clang or gcc. (But I'm not very familiar with how the NaCl
> > build is hooked up in gn; maybe I'm missing something)
> 
> I will double-check to make sure I didn't miss something (since I already got
> this wrong
> once), but I'm pretty sure the toolchain compilers complained about not
> recognizing
> either of these flags. I will report back with my findings, but maybe Roland
or
> Petr
> can comment as well.

Okay, Nico is correct that -fcolor-diagnostics seems to work.

However, the
native_client/toolchain/linux_x86/nacl_x86_newlib/bin/x86_64-nacl-g++ binary
does not recognize either -std=c++11 or -std=gnu++11 . Roland, does that sound
correct?

[chromium-reviews, 2015-09-03 20:26:59]

Yes that's correct, nacl-gcc doesn't support C++11.

On Thu, Sep 3, 2015 at 1:24 PM <dpranke@chromium.org> wrote:

> On 2015/09/03 15:36:10, Dirk Pranke wrote:
> > On 2015/09/03 05:32:00, Nico (vacation Thu Sep 2) wrote:
> > > The changes in build/config/compiler/BUILD.gn look wrong to me. Why are
> > > they needed? From what I understand, NaCl sometimes uses clang, their
> > clang
> > > revision just trails ours a bit. But it's new enough to understand
> > > -fcolor-diagnostics. Likewise, NaCl should need -std=gnu++11
> > independently
> > > of it using clang or gcc. (But I'm not very familiar with how the NaCl
> > > build is hooked up in gn; maybe I'm missing something)
>
> > I will double-check to make sure I didn't miss something (since I already
> > got
> > this wrong
> > once), but I'm pretty sure the toolchain compilers complained about not
> > recognizing
> > either of these flags. I will report back with my findings, but maybe
> > Roland
> or
> > Petr
> > can comment as well.
>
> Okay, Nico is correct that -fcolor-diagnostics seems to work.
>
> However, the
> native_client/toolchain/linux_x86/nacl_x86_newlib/bin/x86_64-nacl-g++
> binary
> does not recognize either -std=c++11 or -std=gnu++11 . Roland, does that
> sound
> correct?
>
> https://codereview.chromium.org/1307653008/
>

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

[Roland McGrath, 2015-09-03 20:43:44]

lgtm

[Dirk Pranke, 2015-09-03 21:36:05]

The CQ bit was checked by dpranke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-03 21:36:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1307653008/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1307653008/40001

[brettw, 2015-09-03 21:38:16]

It seems like nacl clang is a hairy issue and for now we can't call the nacl
compiler clang. This leaves a long term question about how much the nacl
compiler will diverge with or be like clang, and whether the current
clang-vs-non-clang conditions express this properly. Hopefully we can keep the
complexity here under control.

LGTM on this to get us unblocked for now.

[Dirk Pranke, 2015-09-03 21:40:14]

On 2015/09/03 21:38:16, brettw wrote:
> It seems like nacl clang is a hairy issue and for now we can't call the nacl
> compiler clang. This leaves a long term question about how much the nacl
> compiler will diverge with or be like clang, and whether the current
> clang-vs-non-clang conditions express this properly. Hopefully we can keep the
> complexity here under control.
> 
> LGTM on this to get us unblocked for now.

Agreed; once we get the unforking landed, I expect Roland, I, and others will
work on
disentangling the NaCl toolchains from the default clang toolchains.

[Dirk Pranke, 2015-09-03 21:56:41]

Nico, hopefully this addresses your concerns; if it doesn't, we can talk further
and I will address them in follow-up CLs ASAP.

[commit-bot: I haz the power, 2015-09-03 22:05:29]

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

[commit-bot: I haz the power, 2015-09-03 22:05:30]

Dry run: This issue passed the CQ dry run.

[Dirk Pranke, 2015-09-03 22:10:25]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2015-09-03 22:10:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
mcgrathr@chromium.org
Link to the patchset: https://codereview.chromium.org/1307653008/#ps40001
(title: "add more clarifying comments about NaCl and the -std flag")

[commit-bot: I haz the power, 2015-09-03 22:10:47]

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

[chromium-reviews, 2015-09-03 22:10:54]

We have 3 compilers: nacl-gcc, nacl-clang and pnacl-clang. nacl-clang is
clang (implemented as a custom Clang driver), pnacl-clang is a Python
wrapper around clang which supports only subset of clang flags (and some
other on top), and nacl-gcc which obviously isn't clang. I'm currently
working on re-implementing pnacl-clang as a custom Clang driver which
should simplify the situation in the future, but nacl-gcc still needs to be
handled separately.

On Thu, Sep 3, 2015 at 2:56 PM <dpranke@chromium.org> wrote:

> Nico, hopefully this addresses your concerns; if it doesn't, we can talk
> further
> and I will address them in follow-up CLs ASAP.
>
> https://codereview.chromium.org/1307653008/
>

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

[commit-bot: I haz the power, 2015-09-03 22:18:13]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-03 22:18:49]

Patchset 3 (id:??) landed as
https://crrev.com/b8c6bf7592cf6cb412f2d3f151bc29bd96ee7380
Cr-Commit-Position: refs/heads/master@{#347262}

[Nico, 2015-09-05 01:03:20]

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

https://codereview.chromium.org/1307653008/diff/40001/build/config/compiler/B...
build/config/compiler/BUILD.gn:525: if (is_linux || is_android || (is_nacl &&
is_clang)) {
I still don't understand this change. Why is this depending on clang?

From what I understand, this probably corresponds to
https://code.google.com/p/chromium/codesearch#chromium/src/native_client/buil...
– should this just say

 if (is_nacl)
   gnu++0x  # nacl toolchain is old
  else if /* as before */

?

[Dirk Pranke, 2015-09-05 01:37:20]

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

https://codereview.chromium.org/1307653008/diff/40001/build/config/compiler/B...
build/config/compiler/BUILD.gn:525: if (is_linux || is_android || (is_nacl &&
is_clang)) {
On 2015/09/05 01:03:20, Nico (offsite Fri Sep 3) wrote:
> I still don't understand this change. Why is this depending on clang?

the clang-based NaCl toolchains understand this flag. the gcc-based ones do not.

We need the flag in some places to compile some of the code (like the irt), but
we don't 
need it everywhere (apparently).

Does that help clarify things?

> 
> From what I understand, this probably corresponds to
>
https://code.google.com/p/chromium/codesearch#chromium/src/native_client/buil...
> – should this just say
> 
>  if (is_nacl)
>    gnu++0x  # nacl toolchain is old
>   else if /* as before */
> 
> ?

Yes, that might work. The code, however, compiles just fine w/ no --std= flag at
all, too.