Notable examples:

"-D__google_stl_debug_vector=1" is used to catch std::vector out-of-bounds errors with locally modified libstdc++.
(https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/8e5b1dd2c8a4ff48e249cfb0228f1d4483dd1c64/chromeos-base/chromeos-chrome/chromeos-chrome-9999.ebuild#836)

MARCH_TUNE contains flags like "-march=armv7-a" and  "-mtune=cortex-a15", and they will be appended to CFLAGS/CXXFLAGS to tune compiler.
For example:
- amd64-generic: https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master/overlay-amd64-generic/make.conf
- veyron: https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master/overlay-veyron/make.conf

Adding a few other folks that might weigh in on this ...

Wow, I had no idea this was supported, but sure enough, the GYP ninja generator will check for these env vars and write them into the files as appropriate.

If we really need this functionality, I guess we can add it, but I'd prefer to handle this in one of two ways:

1. If there are flags that are set in every CrOS Build, or just a couple of sets of flags, we should just define configs and build_args for them.

2. If there really are flags that vary w/ every board, then it seems like adding pass-through flags is probably the best way to deal with this. In this case we'd want to add some way to pass things through in the GN args themselves, if in case that wasn't clear. I.e., we do not want GN looking at env vars directly, I don't think.

Does that make sense?

Please note that CFLAGS/CXXFLAGS/LDFLAGS are set by make.conf (https://wiki.gentoo.org/wiki//etc/portage/make.conf) 
which is used to configure not only Chrome, but also other binaries of Chrome OS (e.g. power manager and network manager).
So, no matter how GN will handle this for Chrome, Chrome OS developers will continue using these flags to configure other binaries for each device.

In this regard, IMHO the option #2 (adding pass-through flags) is the easiest to implement and to maintain.

FWIW pasted below are the list of flags used to configure Chrome OS binaries for each device.
MARCH_TUNE will be appended to CFLAGS and CXXFLAGS by chromiumos-overlay/chromeos/config/make.conf.generic-target.
(https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/master/chromeos/config/make.conf.generic-target)

hashimoto:~/chromeos/src/overlays % cat $(find -name "*make.*") | grep "FLAGS\\|MARCH_TUNE\\|ARM_FPU" | sort | uniq
ARM_FPU=crypto-neon-fp-armv8
ARM_FPU=neon
ARM_FPU=vfp
CFLAGS="${CFLAGS} -clang-syntax"
CFLAGS="-O2 -pipe"
LDFLAGS="${LDFLAGS} -Wl,--fix-cortex-a53-843419"
MARCH_TUNE="-march=armv6zk -mtune=arm1176jzf-s -mfloat-abi=hard -mfpu=${ARM_FPU}"
MARCH_TUNE="-march=armv7-a -mfpu=${ARM_FPU} -mfloat-abi=hard"
MARCH_TUNE="-march=armv7-a -mtune=cortex-a15 -mfpu=${ARM_FPU} -mfloat-abi=hard"
MARCH_TUNE="-march=armv7-a -mtune=cortex-a5 -mfpu=${ARM_FPU} -mfloat-abi=hard"
MARCH_TUNE="-march=armv7-a -mtune=cortex-a7 -mfpu=${ARM_FPU} -mfloat-abi=hard"
MARCH_TUNE="-march=armv7-a -mtune=cortex-a8 -mfpu=${ARM_FPU} -mfloat-abi=hard"
MARCH_TUNE="-march=armv7-a -mtune=cortex-a9 -mfpu=${ARM_FPU} -mfloat-abi=hard"
MARCH_TUNE="-march=armv8-a+crc -mtune=cortex-a57.cortex-a53 -mfpu=${ARM_FPU} -mfloat-abi=hard"
MARCH_TUNE="-march=atom -mtune=atom -mfpmath=sse"
MARCH_TUNE="-march=core2"
MARCH_TUNE="-march=corei7"
MARCH_TUNE="-march=corei7 -mtune=generic"
MARCH_TUNE="-march=i686 -mfpmath=sse -mmmx -msse -msse2 -msse3"
MARCH_TUNE="-march=mips32"
MARCH_TUNE="-march=mips64"
MARCH_TUNE="-march=x86-64 -msse3"
MARCH_TUNE="-mcpu=power4 -maltivec -mtune=power5"

OK. Let's go with adding pass-through flags, then

Uh, we really can't be appending those flags to cflags in GN. I actually can't see how that would work correctly in GYP either.

The build will already set march, mfloat, mfpu, mfloat-abi, and mtune. Appending them in the best case will just duplicate things already there, and in the worst case will conflict and I don't know what the compiler will do.

We need to be configuring the flags in GN the way the GN build wants them to be configured. We should add configuration to the build files if the current setup is insufficient. I suspect in GYP we're just getting lucky and the flags are duplicates, but I don't think this is a good situation we want to duplicate.

I imagine we wouldn't merely append flags, but rather add something like compiler_cpu_abi_cflags and compiler_cpu_abi_ldflags as build args, and then if they are set, have the compiler_cpu_abi flag set them instead of the existing values.

Since, as hashimoto@ says, the flags are coming from portage in the first place, if we were to add specific flags for each machine type and specify the machine type as a build_arg, we just end up with two additional lookup blocks to translate cc_flags -> gn_flag -> cc_flag.

I suppose we kind of already have the latter one, but it's still a bit of a toss-up, and passthough is certainly more flexible.

Basically all of these things are exposed as build args via build/config/arm.gni and there is quite a bit more logic in build/config/compiler/BUILD.gn to deal with these.

I don't see how anything other than properly setting these build args properly is remotely practical.

To be more clear, build/config/arm.gni has a bunch of build variables in it. These are used to set flags like the FP mode on the command line.

These GN build flags have to be set properly rather than just passing in some random set of extra cflags to the compiler from outside the build. These flags are used for some things other than just computing the command line shared by all target. As an example, skia and webp both check the arm_version variable. We need to keep all of these things in sync.

If you set the build flags, the compiler flags should be set accordingly (or it's a bug we should fix). If there's anything not supported, we can add flags to do those things.

Even in cases where there's not a corresponding downside to passing an arg in (like probably "-Wl,--fix-cortex-a53-843419") I'm in principle opposed to adding "extra flags" to the build args. It has come up a number of times that people have wanted to add this kind of thing, and in each case there was a better way to do what they wanted. Passing in args is obscure and easy to break and I think it's better not to provide such a big dangerous hammer to use.

In practice, this probably means there's a script that converts ebuild's notion of flags to GN's when invoking GN to compile Chrome.

To chime in a bit ... brettw@ and I talked about this face-to-face , and I now agree that using a passthrough mode in compiler_cpu_abi is risky because there may be stuff elsewhere that depends on knowing which arch we're building as he says. 

So, I think we should probably do the mapping from env var flag to GN flag, even though it's annoying :(.

Ugh, I really wanted to avoid doing this, but it seems unavoidable :(

To implement the flags->GN converter,
IIUC this is the list of things to do:

For ARM: build/config/arm.gni provide a number of args, but it's not sufficient.
 -march: Needs to support values like "armv6zk" and "armv8-a+crc".
 -mtune: arm_tune is ignored when arm_version==6, but -mtune=arm1176jzf-s is used for Raspberry Pi
  (https://chromium.googlesource.com/chromiumos/overlays/board-overlays.git/+/master/overlay-raspberrypi/make.conf).
 -mfpu: Needs to support values like "crypto-neon-fp-armv8".
 -Wl,--fix-cortex-a53-843419: Needed once  issue 527333  is resolved (see also  issue 532532 ).


For x86/x64: 
 -march: Needs to support values like "i686", "x86-64", "atom", "core2", and "corei7"
 -mfpmath: build/common.gypi already supports this. GN should do the same thing.
 -mmmx, -msse, -msse2, -msse3: Needs to be supported.

For other archs:
 -march=mips32/64 (seems build/config/compiler/BUILD.gn already supports these?)
 -mcpu=power4 (I'm not sure if this is still needed, but if it is, something needs to be done.)

BTW, "-mxxx" flags are already used by random GN files.
(e.g. Libraries such as Skia are using -msse2 and -msse4.1 in their GN files: https://code.google.com/p/chromium/codesearch#search/&q=msse%20file:gn.?$%20case:yes&sq=package:chromium&type=cs )
Should we also fix them?

power4, huh? ....

Yes, we should probably file bugs to make the other libraries we control be consistent.

I wish my team (chromeos-toolchain) was copied on this earlier. 

We do need a good way to pass CFLAGS/CXXFLAGS/LDFLAGS from ChromeOS. 
It is very common for different devices to use different compiler flags.
Developers in ChromeOS like to optimize for their boards as much as they can. 
We cannot expect that everyone will build chrome with the same flags for every device. 

Please allow for maximum flexibility on this. 

I think that the proposal here is that the flexibility will still exist, but new flags will have to jump through bunch of extra hoops.

Here is my understanding of how things could be made to work:

1. Chrome OS build is kicked off with CFLAGS/CXXFLAGS/LDFLAGS defined just as they are right now.
2. Chrome ebuild will run a translation that we look into each flag in CFLAGS/CXXFLAGS/LDFLAGS and convert it into a GN vars.
3. If CFLAGS/CXXFLAGS/LDFLAGS flag is found that we don't know how to convert into GN var, chrome ebuild should fail.
4. Chrome's "gn gen" part should fail if we try to pass a var that it can't understand internally.

So, if we want to drop a new flag into FLAGS/CXXFLAGS/LDFLAGS from Chrome OS side, we will need to first:

a) define a GN var for it,
b) wait for Chrome PFQ to rev up, and then
c) modify flags->GN translator in chrome ebuild

This particular work item is really about building support for step #2 and making sure we can represent all existing flags as GN vars on Chromium side.

Did I get this right?

This description worries me. 

I understand GN may want to have control over some compiler flags (-march -mtune maybe) but I don't understand why it would want to have control over all of them. 

The compiler has hundreds of flags and each compiler has his own set of flags. 
It happens often that we try to use non-common flags. For example when:
1) triaging compiler problems. We need to enable/disable specific compiler optimizations, enabling/disable compiler optimizations for certain functions or range of functions. The compiler has options for all of these and different compilers have different options to do this. This is a very important use case for us. And it is usually is very time critical. 
2) trying to get better performance. We also may use non-common options for this. This is usually not as time critical as 1) but still important for us. It is important for us to be able to test different compiler options and see which ones give the best performance. 
3) adding new compiler flags. Once in a while we need to add a compiler flag to fix an error (or even to fix warnings).. Ex: -Wl,--fix-cortex-a53-843419

So, if for each of these cases I need to modify GN and teach it about a non-common or new compiler option and then wait until the PFQ is revved up (can be a week before it happens), you are going to make the work of my team very difficult. 

As I said, I understand you want to do this for a few options (should be very a very small set). You could teach GN to complain if someone tried to modify these options using a mechanism different from GN vars. 
But not having a mechanism to pass EXTRA CFLAGS or CXXFLAGS is way too restrictive and I don't see the value in it. 

fwiw, i'd echo Luis's concerns/position, also when talking about Chromium and the wider community (e.g. distros)

Btw, I am not advocating that what I described in #17 is super awesome design in any way - I am just trying to summarize what GN owners are suggesting we should do on Chrome OS side.

Fundamentally, GN isn't designed to be embedded into other build systems without implementing crutches like those I described in #17. 

The reason not to allow this is that people screw it up. From the flags in comment 4, your build was doing it wrong. The value in this restriction is to prevent this type of screw up.

You can't just pass in ARM FPU flags, for example, and append them to the set of ARM FPU flags that Chrome has internally computed and expect a rational result. There are only a couple of flags in the list in comment 4 that make sense when appended to the flags that Chrome has produced.

Likewise, the stuff that the distros will want to pass in are optimization flags which Chrome sets itself also.

I think there are two related things. Step 1 is that you really need to use chrome flags for the things that currently exist (hardfp as an example). Step 2 is what we do about things like "fix-cortex-a53-843419". Any way that the latter type of flags gets passed into Chrome needs to be not mixed up with the general CPU flags that you may be passing to other programs in the build.

locking down a subset doesn't necessarily translate into lock down everything.  plus, gn's behavior can't even handle every scenario: it's trivial (and common) for people to set default flags in the compiler itself that are different from what you expect.  so unless you're dumping the compiler specs and inspecting the preprocessed/compiled output/objects, you're not going to get the coverage you think you are.

as a real world example, tell me what fpu ABI does `armv7a-linux-gnueabi-gcc test.c -o test` produce by default ?  soft ?  hard ?  vfp3 ?  vfp4 ?  vfp5 ?  neon ?  d16 ?  d32 ?  answer: it could be any of them.  distros control the default when they configure gcc by leveraging the --with-fpu flag.

if you want to catch insanity in the common case, that's fine.  check for the few flags that you actually care about (e.g. fpu or arch/cpu).  but that doesn't mean it has to be detrimental to the "power" users who need to do more.

Why does Chrome compute and add ARM FPU flags to the build, isn't the choice of FPU flags to use board specific that should be specified by ChromeOS in some way? 

To reiterate my comment 21, there are two things: how to pass flags in that Chrome knows about (arm version, fp unit type), and how to pass things in that Chrome doesn't know about (like fix-cortex-a53-84341). Here, I'm assuming we're talking about the former. For the latter, I agree there needs to be a convenient way to set these things and we can discuss the best way to expose that.

For vapier's specific question about FPU flags, the Chrome build computes the FPU flags based on default values or, if specified, a GYP_DEFINE / gn build arg, and passes them on the command line. For ARM, it will always specify the FPU type. This hasn't changed between GYP and GN.

In GYP it would append a CFLAGS environment variable. If you didn't carefully keep the CFLAGS in sync with your GYP_DEFINES, you would generate command lines that specify, in the best case, the fpu mode twice, and in the worst case, conflicting FPU values. I suspect your GYP builds work because the flags are in sync "enough" but this doesn't mean that this situation is desirable. The chrome build variables need to be set so everything is consistent. Anything can key off of these build variables, including which source files are compiled (assembly files for specific architectures) warning flags, etc.

ChromeOS can and should specify the FPU mode, etc, but needs to do so in a consistent way that the Chrome build understands. This means setting build args rather than appending a set of flags and hoping for the best.

If you want to set everything for an entire CrOS build in the env var, that's fine, but then somebody needs to do the reverse mapping from env var to build variable when the build is instantiated. I'm saying that somebody should be the person wanting to configure the build with environment variables (the CrOS build).

As long as the set of compiler flags you want to control and sanity check is small and we have way to pass "raw" compiler options through GN (no cumbersome translations), I am ok.
ChromeOS has more than 80 different devices for 4 different architectures and many little differences in cpu configurations, FPU configurations, vector support etc.. We need a way to deal with this diversity and take advantage of it. We cannot use the "one size fits ALL" approach. You cannot guess ahead of time what compiler options we are going to need to use.

agreed -- if you want to lock down a subset (e.g. fpu control) to try and guarantee some baseline sanity, that's fine.  i get antsy when/if gn tries to force every single compiler option through specific gn vars as Luis elaborated.

brettw@ says:
"I suspect your GYP builds work because the flags are in sync "enough" but this doesn't mean that this situation is desirable", and
"ChromeOS can and should specify the FPU mode, etc, but needs to do so in a consistent way that the Chrome build understands. This means setting build args rather than appending a set of flags and hoping for the best."

As a former manager of the toolchain team and having advised teams on best flags to use, I find this a bit offensive. 

The flags to build Chromeos-chrome for each platform were carefully chosen with both experimentation for best performance and consulting with the compiler team. The implementation required correctly setting the GYP_DEFINES in the ebuild and passing in platform-specific flags via make.conf, correctly dealing with vagaries like workarounds for hardware errata. It is not accidental that it works. 

I'm not doing toolchains anymore, so I'm not very familiar with the new system, but my input here: its best if we can make ChromeOS-Chrome package behave similar to how other ChromeOS packages work (most of which respect CFLAGS) rather than have more specialized logic just for Chrome. 

Apologies for not chiming in earlier on this, my bug updates were being marked as spam :( ...

@bjanakiraman - while I respect the amount of work the CrOS team has put in to making things work and I'm sure neither Brett nor I means any offense, I suspect that there is a fair amount of truth to Brett's comments because the bulk of chromium's build files (both GYP and GN) are maintained by people who are neither aware of how the CrOS build works nor capable of verifying whether the build settings that are being set in the GYP and GN files are conflicting with flags passed externally.

So, if you are managing to ensure that we don't hit conflicts and inconsistencies, I think we'd love to know how you're doing it. Do you have scripts that look at the resulting compile lines and check for conflicting flags, or something like that?

I think we understand where you're coming from. The point we're trying to get across, which Zel got close to in comment #20, is that the *chromium build* assumes near-full control over how things are being compiled. This is currently true in GN, and AFAIK has always been true in GYP as well: unless I'm missing something, there's simply no settings in the Chromium GYP defines to say "don't set any (of any given kind of) flags at all, because that will all be configured externally". I don't even know how you would do that without recreating a fair amount of logic that exists in the GYP and GN files, since we have so many different kinds of objects compiled with different settings. If I'm misinformed about this and Brett and I have missed some subtlety in how existing Chrome builds work on CrOS, please tell us what we've missed.

(To repeat: this has nothing to do w/ GN itself, which has no built-in assumptions about compiler flags at all, but is a matter of how the chromium builds are set up).

As Brett said above, I think we're fine w/ adding escape hatches for flags that we are sure at not otherwise specified in the Chrome builds, and don't conflict with flags that are specified. If you can identify a set of flags that we want to make sure aren't specified in the Chrome builds (and that might be being set today), so that you can pass them safely via cflags, we can also look into how to do that. If you have advice on how we might restructure our build files to make this easier to maintain, I'm sure we have open ears for that as well.
 

Part of the problem (from the toolchain team side) is that there is not a hard-and-fast set of flags we need to turn on or off.  The compiler, like all other software, changes and the needed set of flags change too.  Sometimes we might need to test a new flag we are creating for a feature we are adding -- the new flag doesn't even exist in the released compiler and we really need to be able to test it before the compiler gets released.  Sometimes after a compiler release something unexpectedly breaks and we need to fix it *fast* and the best way to work around the issue is with a new or different compiler (or linker) flag, and we really can't afford to wait the 1-2 weeks it will take to teach GN about the new flag and then wait for a new version of Chrome containing the GN change to roll into ChromeOS.

I don't think any of us particularly object to your standardizing most of the flags that get passed to Chrome builds.  But removing our ability, WHEN NEEDED, to add arbitrary compiler or linker flags will truly cripple our ability to do our jobs.

Hopefully it's clear now that we will add that.

I'll take over ownership on this.

dpranke@: 

Its not possible for ChromeOS to do any validation since the knowledge of which flags are 'needed' is within Chrome. It would be great if GN did some of that. 

All GYP_DEFINES used to build ChromeOS-Chrome are specified in the chromeos-chrome ebuild (https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/master/chromeos-base/chromeos-chrome/chromeos-chrome-9999.ebuild), with make.conf adding supplemental per-board flags via CFLAGS. With this, ChromeOS was in complete control of how Chrome was built for the platform. IMO its best to keep GYP flags somewhat general, otherwise you'd end up inventing a lot of flags since the compilers support a ton of options and variants. If a particular subsystem needed to be build a particular way, I think its okay for that subsystem to hard code them and ignore CFLAGS, but that should be few in number.

One thing that I always found odd in common.gypi, ChromeOS is identified as 'OS == 'Linux' && chromeos == 1' (compared to MacOS or Android that are OSes). This led to bugs where something would be added to Linux and would not work on ChromeOS, and we'd notice build breakages later. I'm not sure if this is still happening though and worth fixing. 

We ship one chrome binary to many many different machines running Windows. We ship a very small number of binaries to many many different machines running Android. Why do we need a different chrome binary for every CrOS board?

@bjanakiraman: GN itself has very limited awareness of compiler flags, and we certainly wouldn't want to hard-code that. We could add some concept of "these cflags conflict with those cflags" per toolchain, but given that GN's main mechanism for managing flags is to use configs that are added and removed precisely to not have this problem, it seems like something we'd be reluctant to add.

We certainly do try to keep gyp/gn flags fairly generic.

In GN, you set target_os="chromeos", rather than chromeos=1. However, chromeos is still considered a variant of linux, so you may still need to write (is_linux && !is_chromeos) to get something that applies only to desktop linux. This is because we think you still want more 'is_linux' things to apply to cros than not.

However, this is also made more difficult by the fact that we don't easily distinguish os-level cros features from ui/app-level cros features. If we could do that, then some of these hoops would probably go away, and I think at least until we do that you'll get confusion regardless of which way is the default.

See https://bugs.chromium.org/p/chromium/issues/detail?id=608596#c55 for a couple of CLs that I've posted that should (hopefully) address this. More discussion has gone on in that bug as well and this one is basically a dupe of that at this point.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8ad2f49335feaddddbd3f318fc6f4d13eb52760b

commit 8ad2f49335feaddddbd3f318fc6f4d13eb52760b
Author: dpranke <dpranke@chromium.org>
Date: Wed May 18 20:33:04 2016

Rework the way ChromiumOS toolchains will work in GN.

This CL reworks the way ChromiumOS toolchains work in GN, in such a way
that they might actually have all the flags they need for the
ChromiumOS ebuild files to be able to set all of the flags it needs
(though there are still some missing GN build_args).

Specifically, the ebuild will now need to set the following in args.gn:

  host_toolchain = "//build/toolchain/cros:host"
  v8_snapshot_toolchain = "//build/toolchain/cros:v8_snapshot"

in order to support boards other than the amd64-generic build. The
ebuild should actually set these variables all the time; it just
happens that the amd64-generic build will work at the moment without
the variables, but that will not be guaranteed to remain true in the future.

This CL also adds the following optional build args that do pretty
much what you'd expect them to do:

  cros_target_ld, cros_target_extra_cflags, cros_target_extra_cppflags,
  cros_target_extra_cxx_flags, cros_target_extra_ldflags,
  cros_host_ar, cros_host_cc, cros_host_cxx, cros_host_ld,
  cros_host_is_clang, cros_host_extra_cflags, cros_host_extra_cppflags,
  cros_host_extra_cxx_flags, cros_host_extra_ldflags,
  cros_v8_snapshot_ar, cros_v8_snapshot_cc, cros_v8_snapshot_cxx,
  cros_v8_snapshot_ld, cros_v8_snapshot_extra_cflags,
  cros_v8_snapshot_extra_cppflags, cros_v8_snapshot_extra_cxx_flags,
  cros_v8_snapshot_extra_ldflags

This CL should be backwards-compatible with the existing linux desktop
ChromiumOS builds and the amd64-generic simplechrome/ebuild (i.e., it
can be landed and reverted w/o requiring any other changes to be made).
It is a big hammer intended to un-block the ChromiumOS GN migration
while we continue thinking about how to best support ChromiumOS.

R=stevenjb@chromium.org, brettw@chromium.org
BUG= 608596 ,  595653 

Review-Url: https://codereview.chromium.org/1983613002
Cr-Commit-Position: refs/heads/master@{#394534}

[modify] https://crrev.com/8ad2f49335feaddddbd3f318fc6f4d13eb52760b/build/toolchain/cros/BUILD.gn
[modify] https://crrev.com/8ad2f49335feaddddbd3f318fc6f4d13eb52760b/build/toolchain/gcc_toolchain.gni

This is fixed, at least as far as CrOS's needs go. Since we don't particularly want to provide generic hooks for this, I think we can close this now.

Closing. Please reopen it if its not fixed. Thanks!