Add an `auto_profile_path` flag to GN to control AFDO.

Add an `auto_profile_path` flag to GN to control AFDO.

This change adds a new top-level compiler config to GN, so that we can
control whether AFDO should be enabled or not per-target in CrOS
official builds (which is where we use AFDO).

R=brettw@chromium.org, llozano@chromium.org
BUG=633719
Committed: https://crrev.com/bffda9f508c0376ab3ae4b9e52b0367c24f3818a
Cr-Commit-Position: refs/heads/master@{#422513}

[Dirk Pranke, 2016-09-27 22:41:37]

For more on AFDO: https://gcc.gnu.org/wiki/AutoFDO

In theory it might be nice to consolidate this sort of switch with the windows
switches for PGO, but the mechanism is fairly different and so I'm not sure how
doable that is.

[brettw, 2016-09-27 23:52:41]

https://codereview.chromium.org/2375783002/diff/1/build/config/compiler/compi...
File build/config/compiler/compiler.gni (right):

https://codereview.chromium.org/2375783002/diff/1/build/config/compiler/compi...
build/config/compiler/compiler.gni:41: auto_profile_path = ""
This appears in a different declare_args block in the previous file. That seems
wrong and I would have expected GN to give an error. Maybe we just want this
one?

[Dirk Pranke, 2016-09-28 00:08:34]

https://codereview.chromium.org/2375783002/diff/1/build/config/compiler/compi...
File build/config/compiler/compiler.gni (right):

https://codereview.chromium.org/2375783002/diff/1/build/config/compiler/compi...
build/config/compiler/compiler.gni:41: auto_profile_path = ""
On 2016/09/27 23:52:41, brettw (ping on IM after 24h) wrote:
> This appears in a different declare_args block in the previous file. That
seems
> wrong and I would have expected GN to give an error. Maybe we just want this
> one?

Oh, whoops, I forgot to back this change out. I did this change and then the
BUILD.gn one, at which point I realized we didn't need to expose the arg in the
.gni.

[Dirk Pranke, 2016-09-28 00:11:10]

Updated, please take another look?

[brettw, 2016-09-28 17:15:21]

lgtm

[Dirk Pranke, 2016-09-28 17:18:20]

On 2016/09/28 17:15:21, brettw (ping on IM after 24h) wrote:
> lgtm

@llozano, is it easy enough for you to confirm that this works for your builds
before I land it, or do you need me to land it and then modify the ebuild before
we're confident?

[llozano, 2016-09-28 18:19:34]

On 2016/09/28 17:18:20, Dirk Pranke wrote:
> On 2016/09/28 17:15:21, brettw (ping on IM after 24h) wrote:
> > lgtm
> 
> @llozano, is it easy enough for you to confirm that this works for your builds
> before I land it, or do you need me to land it and then modify the ebuild
before
> we're confident?

I can test before you land it. Working on it today.

[llozano, 2016-09-28 21:24:14]

On 2016/09/28 18:19:34, llozano wrote:
> On 2016/09/28 17:18:20, Dirk Pranke wrote:
> > On 2016/09/28 17:15:21, brettw (ping on IM after 24h) wrote:
> > > lgtm
> > 
> > @llozano, is it easy enough for you to confirm that this works for your
builds
> > before I land it, or do you need me to land it and then modify the ebuild
> before
> > we're confident?
> 
> I can test before you land it. Working on it today.

I got the following error:


ERROR at //build/config/compiler/BUILD.gn:1493:7: Condition does not evaluate to
a boolean value.
  if (auto_profile_path) {
  --  ^----------------
This is a value of type "string" instead.
See //build/config/BUILDCONFIG.gn:470:3: which caused the file to be included.
  "//build/config/compiler:afdo",

[Dirk Pranke, 2016-09-28 21:43:08]

On 2016/09/28 21:24:14, llozano wrote:
> On 2016/09/28 18:19:34, llozano wrote:
> > On 2016/09/28 17:18:20, Dirk Pranke wrote:
> > > On 2016/09/28 17:15:21, brettw (ping on IM after 24h) wrote:
> > > > lgtm
> > > 
> > > @llozano, is it easy enough for you to confirm that this works for your
> builds
> > > before I land it, or do you need me to land it and then modify the ebuild
> > before
> > > we're confident?
> > 
> > I can test before you land it. Working on it today.
> 
> I got the following error:
> 
> 
> ERROR at //build/config/compiler/BUILD.gn:1493:7: Condition does not evaluate
to
> a boolean value.
>   if (auto_profile_path) {
>   --  ^----------------
> This is a value of type "string" instead.
> See //build/config/BUILDCONFIG.gn:470:3: which caused the file to be included.
>   "//build/config/compiler:afdo",

Ah, whoops. That needs to be `auto_profile_path != ""` :).

Fix a-comin'.

[Dirk Pranke, 2016-09-28 21:53:33]

Okay, please try again?

[llozano, 2016-09-28 21:59:32]

On 2016/09/28 21:53:33, Dirk Pranke wrote:
> Okay, please try again?

Already started. I figured that was the fix you would want. so it is already
being tested.

[llozano, 2016-09-28 22:53:19]

On 2016/09/28 21:59:32, llozano wrote:
> On 2016/09/28 21:53:33, Dirk Pranke wrote:
> > Okay, please try again?
> 
> Already started. I figured that was the fix you would want. so it is already
> being tested.

another problem. we cannot pass this option to the nacl compilers. 
[17529/31460]
../../../../../../../home/llozano/chrome_root/src/native_client/toolchain/linux_x86/pnacl_newlib/bin/arm-nacl-clang
-MMD -MF
clang_newlib_arm/obj/native_client_sdk/src/libraries/nacl_io/nacl_io/log.o.d
-DNACL_TC_REV=f7d719122cd7c2fe3ebc52e7c0b011c583bf3e9c -DV8_DEPRECATION_WARNINGS
-DENABLE_NOTIFICATIONS -DENABLE_PLUGINS=1 -DENABLE_PDF=1 -DENABLE_PRINTING=1
-DENABLE_BASIC_PRINTING=1 -DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1
-DUSE_OPENSSL_CERTS=1 -DUSE_OZONE=1 -DNO_TCMALLOC -DENABLE_WEBRTC=1
-DENABLE_EXTENSIONS=1 -DENABLE_TASK_MANAGER=1 -DENABLE_THEMES=1
-DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_SESSION_SERVICE=1
-DENABLE_SUPERVISED_USERS=1 -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD
-DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD -DENABLE_MEDIA_ROUTER=1
-DFIELDTRIAL_TESTING_ENABLED -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
-D_LARGEFILE64_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS
-D_FORTIFY_SOURCE=2 -DNDEBUG -DNVALGRIND
-I../../../../../../../home/llozano/chrome_root/src/native_client_sdk/src/libraries
-I../../../../../../../home/llozano/chrome_root/src -Iclang_newlib_arm/gen
-I../../../../../../../home/llozano/chrome_root/src/native_client_sdk/src/libraries
-I../../../../../../../home/llozano/chrome_root/src/native_client_sdk/src/libraries/nacl_io/include
-I../../../../../../../home/llozano/chrome_root/src/native_client_sdk/src/libraries/third_party/newlib-extras
-fauto-profile=/build/daisy/tmp/portage/chromeos-base/chromeos-chrome-55.0.2869.0_rc-r2/work/afdo/chromeos-chrome-amd64-55.0.2869.0_rc-r2.afdo
-fno-strict-aliasing -fcolor-diagnostics -integrated-as -mtune=generic-armv7-a
-Wall -Werror -Wextra -Wno-missing-field-initializers -Wno-unused-parameter
-Wno-c++11-narrowing -Wno-covered-switch-default -Wno-deprecated-register
-Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override -O2
-fno-ident -fdata-sections -ffunction-sections -g2 -gsplit-dwarf
-fvisibility=hidden -Wno-sign-compare  -c
../../../../../../../home/llozano/chrome_root/src/native_client_sdk/src/libraries/nacl_io/log.c
-o clang_newlib_arm/obj/native_client_sdk/src/libraries/nacl_io/nacl_io/log.o
FAILED:
clang_newlib_arm/obj/native_client_sdk/src/libraries/nacl_io/nacl_io/log.o

before this CL, we were only passing this option through the following GN
variables.

cros_target_extra_cflags
cros_target_extra_cxxflags.

I assume those were not passed to the nacl compilers.

[Dirk Pranke, 2016-09-28 23:09:29]

On 2016/09/28 22:53:19, llozano wrote:
> On 2016/09/28 21:59:32, llozano wrote:
> > On 2016/09/28 21:53:33, Dirk Pranke wrote:
> > > Okay, please try again?
> > 
> > Already started. I figured that was the fix you would want. so it is already
> > being tested.
> 
> another problem. we cannot pass this option to the nacl compilers. 

Ah, right, we should add an !is_nacl to the test.

[Dirk Pranke, 2016-09-28 23:10:48]

Updated again.

[llozano, 2016-09-29 06:38:19]

On 2016/09/28 23:10:48, Dirk Pranke wrote:
> Updated again.

ok, it is mostly ok. 
However, the option is being passed to the host compiler.

x86_64-pc-linux-gnu-g++ -MMD -MF
host/obj/third_party/protobuf/protoc_lib/java_generator_factory.o.d
-DV8_DEPRECATION_WARNINGS -DENABLE_MDNS=1 -DENABLE_NOTIFICATIONS
-DENABLE_PEPPER_CDMS -DENABLE_PLUGINS=1 -DENABLE_PDF=1 -DENABLE_PRINTING=1
-DENABLE_BASIC_PRINTING=1 -DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1
-DUSE_UDEV -DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1
-DUSE_DEFAULT_RENDER_THEME=1 -DUSE_NSS_CERTS=1 -DUSE_OZONE=1 -DENABLE_WEBRTC=1
-DENABLE_EXTENSIONS=1 -DENABLE_TASK_MANAGER=1 -DENABLE_THEMES=1
-DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_SESSION_SERVICE=1
-DENABLE_SUPERVISED_USERS=1 -DENABLE_SERVICE_DISCOVERY=1 -DFULL_SAFE_BROWSING
-DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD
-DENABLE_MEDIA_ROUTER=1 -DFIELDTRIAL_TESTING_ENABLED -D_FILE_OFFSET_BITS=64
-D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -DNDEBUG -DNVALGRIND
-DDYNAMIC_ANNOTATIONS_ENABLED=0 -DGOOGLE_PROTOBUF_NO_RTTI
-DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DHAVE_PTHREAD
-I../../../../../../../home/llozano/chrome_root/src -Ihost/gen
-I../../../../../../../home/llozano/chrome_root/src/third_party/protobuf/src
-fauto-profile=

[llozano, 2016-10-01 00:47:01]

On 2016/09/29 06:38:19, llozano wrote:
> On 2016/09/28 23:10:48, Dirk Pranke wrote:
> > Updated again.
> 
> ok, it is mostly ok. 
> However, the option is being passed to the host compiler.
> 
> x86_64-pc-linux-gnu-g++ -MMD -MF
> host/obj/third_party/protobuf/protoc_lib/java_generator_factory.o.d
> -DV8_DEPRECATION_WARNINGS -DENABLE_MDNS=1 -DENABLE_NOTIFICATIONS
> -DENABLE_PEPPER_CDMS -DENABLE_PLUGINS=1 -DENABLE_PDF=1 -DENABLE_PRINTING=1
> -DENABLE_BASIC_PRINTING=1 -DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1
> -DUSE_UDEV -DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1
> -DUSE_DEFAULT_RENDER_THEME=1 -DUSE_NSS_CERTS=1 -DUSE_OZONE=1 -DENABLE_WEBRTC=1
> -DENABLE_EXTENSIONS=1 -DENABLE_TASK_MANAGER=1 -DENABLE_THEMES=1
> -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_SESSION_SERVICE=1
> -DENABLE_SUPERVISED_USERS=1 -DENABLE_SERVICE_DISCOVERY=1 -DFULL_SAFE_BROWSING
> -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD
> -DENABLE_MEDIA_ROUTER=1 -DFIELDTRIAL_TESTING_ENABLED -D_FILE_OFFSET_BITS=64
> -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -DNDEBUG -DNVALGRIND
> -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DGOOGLE_PROTOBUF_NO_RTTI
> -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DHAVE_PTHREAD
> -I../../../../../../../home/llozano/chrome_root/src -Ihost/gen
> -I../../../../../../../home/llozano/chrome_root/src/third_party/protobuf/src
> -fauto-profile=

ping?

[Dirk Pranke, 2016-10-01 02:24:51]

On 2016/10/01 00:47:01, llozano wrote:
> ping?

Oh, sorry, I posted patchset #6 on wednesday, which should address this issue
(as well as the others, of course). I thought I posted a message at that time,
but I don't see it now.

Give it a shot?

[llozano, 2016-10-03 18:49:17]

On 2016/10/01 02:24:51, Dirk Pranke (slow) wrote:
> On 2016/10/01 00:47:01, llozano wrote:
> > ping?
> 
> Oh, sorry, I posted patchset #6 on wednesday, which should address this issue
> (as well as the others, of course). I thought I posted a message at that time,
> but I don't see it now.
> 
> Give it a shot?

Everything seems fine now. 
Please commit.

[Dirk Pranke, 2016-10-03 19:27:10]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2016-10-03 19:27:10]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2375783002/#ps80001
(title: "test based on toolchain instead")

[commit-bot: I haz the power, 2016-10-03 19:27:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[llozano, 2016-10-03 19:50:19]

lgtm

[commit-bot: I haz the power, 2016-10-03 20:27:00]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-03 20:28:49]

Description was changed from

==========
Add an `auto_profile_path` flag to GN to control AFDO.

This change adds a new top-level compiler config to GN, so that we can
control whether AFDO should be enabled or not per-target in CrOS
official builds (which is where we use AFDO).

R=brettw@chromium.org, llozano@chromium.org
BUG=633719
==========

to

==========
Add an `auto_profile_path` flag to GN to control AFDO.

This change adds a new top-level compiler config to GN, so that we can
control whether AFDO should be enabled or not per-target in CrOS
official builds (which is where we use AFDO).

R=brettw@chromium.org, llozano@chromium.org
BUG=633719

Committed: https://crrev.com/bffda9f508c0376ab3ae4b9e52b0367c24f3818a
Cr-Commit-Position: refs/heads/master@{#422513}
==========

[commit-bot: I haz the power, 2016-10-03 20:28:51]

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