Add an optimize_for_fuzzing GN flag to build with -O1

Add an optimize_for_fuzzing GN flag to build most things with -O1

This is better for coverage guided fuzzing builds as less optimization
means more branches in the generated code.

BUG=643249
Committed: https://crrev.com/aaa26c162de624ebcf7a120e85fef3e19ec02151
Cr-Commit-Position: refs/heads/master@{#423217}

[Oliver Chang, 2016-10-03 19:46:07]

Description was changed from

==========
Add an optimize_for_fuzzing GN flag to build with -O1

This is better for coverage guided fuzzing builds as less optimization
means more branches in the generated code.

BUG=643249
==========

to

==========
Add an optimize_for_fuzzing GN flag to build with -O1

This is better for coverage guided fuzzing builds as less optimization
means more branches in the generated code.

BUG=643249
==========

[Oliver Chang, 2016-10-03 19:46:07]

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

[Oliver Chang, 2016-10-03 19:46:22]

Dirk, ptal. Not sure if this is the right way to do it.

[Dirk Pranke, 2016-10-03 20:37:14]

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

https://codereview.chromium.org/2389923003/diff/20001/build/config/compiler/B...
build/config/compiler/BUILD.gn:1479: assert(!is_win)
I would:

- get rid of lines 1467-1477 since they shouldn't ever execute (due to the
ordering on lines 1488-1495)

- make lines 1478-1481 unconditional,

- add an assert(!is_nacl_irt)

- add a second argument to the (!is_win) assert explaining that this isn't
supported on windows (and delete the comment, which would become redundant).

- and get rid of the (!is_win) test on line 1494, below.

https://codereview.chromium.org/2389923003/diff/20001/build/config/compiler/B...
build/config/compiler/BUILD.gn:1495: configs = [ ":optimize_fuzzing" ]
I would get rid of the (!is_win) check here, as per the comments above.

https://codereview.chromium.org/2389923003/diff/20001/build/config/compiler/c...
File build/config/compiler/compiler.gni (right):

https://codereview.chromium.org/2389923003/diff/20001/build/config/compiler/c...
build/config/compiler/compiler.gni:53: optimize_for_fuzzing = false
If this doesn't need to be referenced outside of
//build/config/compiler/BUILD.gn, you should put this in the declare_args()
block in that file.

[Oliver Chang, 2016-10-03 20:54:25]

Thanks for the comments!

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

https://codereview.chromium.org/2389923003/diff/20001/build/config/compiler/B...
build/config/compiler/BUILD.gn:1479: assert(!is_win)
On 2016/10/03 20:37:13, Dirk Pranke (slow) wrote:
> I would:
> 
> - get rid of lines 1467-1477 since they shouldn't ever execute (due to the
> ordering on lines 1488-1495)
> 
> - make lines 1478-1481 unconditional,
> 
> - add an assert(!is_nacl_irt)
> 
> - add a second argument to the (!is_win) assert explaining that this isn't
> supported on windows (and delete the comment, which would become redundant).
> 
> - and get rid of the (!is_win) test on line 1494, below.

Done.

https://codereview.chromium.org/2389923003/diff/20001/build/config/compiler/B...
build/config/compiler/BUILD.gn:1495: configs = [ ":optimize_fuzzing" ]
On 2016/10/03 20:37:13, Dirk Pranke (slow) wrote:
> I would get rid of the (!is_win) check here, as per the comments above.

Done.

https://codereview.chromium.org/2389923003/diff/20001/build/config/compiler/c...
File build/config/compiler/compiler.gni (right):

https://codereview.chromium.org/2389923003/diff/20001/build/config/compiler/c...
build/config/compiler/compiler.gni:53: optimize_for_fuzzing = false
On 2016/10/03 20:37:13, Dirk Pranke (slow) wrote:
> If this doesn't need to be referenced outside of
> //build/config/compiler/BUILD.gn, you should put this in the declare_args()
> block in that file.

Done.

[Oliver Chang, 2016-10-03 20:55:21]

Description was changed from

==========
Add an optimize_for_fuzzing GN flag to build with -O1

This is better for coverage guided fuzzing builds as less optimization
means more branches in the generated code.

BUG=643249
==========

to

==========
Add an optimize_for_fuzzing GN flag to build most things with -O1

This is better for coverage guided fuzzing builds as less optimization
means more branches in the generated code.

BUG=643249
==========

[Dirk Pranke, 2016-10-04 20:11:32]

lgtm, that's about as clean as it can get :).

[Oliver Chang, 2016-10-04 20:31:19]

The CQ bit was checked by ochang@chromium.org

[commit-bot: I haz the power, 2016-10-04 20:31:58]

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

[commit-bot: I haz the power, 2016-10-04 20:35:13]

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

[commit-bot: I haz the power, 2016-10-04 20:35:14]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Oliver Chang, 2016-10-04 21:05:57]

The CQ bit was checked by ochang@chromium.org

[Oliver Chang, 2016-10-04 21:05:58]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/2389923003/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-04 21:06:40]

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

[Oliver Chang, 2016-10-04 21:11:17]

The CQ bit was unchecked by ochang@chromium.org

[Oliver Chang, 2016-10-04 21:28:11]

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

[commit-bot: I haz the power, 2016-10-04 21:28:58]

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

[commit-bot: I haz the power, 2016-10-04 22:37:30]

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

[commit-bot: I haz the power, 2016-10-04 22:37:31]

Dry run: This issue passed the CQ dry run.

[Oliver Chang, 2016-10-04 22:57:34]

It looks like the asserts are evaluated when the file is loaded, not when it's
used. Checking for |optimize_for_fuzzing| inside the "optimize_fuzzing" config
seemed weird to me, so I moved the assert to the "default_optimization" config. 

Dirk, does this still look good?

[Dirk Pranke, 2016-10-05 16:45:38]

still lgtm but I suggest another thing you could do if you feel like it.

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

https://codereview.chromium.org/2389923003/diff/80001/build/config/compiler/B...
build/config/compiler/BUILD.gn:1483: assert(!is_win, "Fuzzing optimize level not
supported on Windows")
One advantage to the prior version is that the asserts would fire if somehow
someone depended on :optimize_fuzzing directly.

You could prevent that by setting `visibility = [ ":default_optimization" ]` in
the :optimize_fuzzing config, which is probably a better way to do things.

However, it's not worth it to spend too much time on this, either.

[Oliver Chang, 2016-10-05 16:58:49]

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

[commit-bot: I haz the power, 2016-10-05 16:59:08]

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

[Oliver Chang, 2016-10-05 17:45:27]

The CQ bit was unchecked by ochang@chromium.org

[Oliver Chang, 2016-10-05 17:45:27]

The CQ bit was checked by ochang@chromium.org

[Oliver Chang, 2016-10-05 17:45:28]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/2389923003/#ps100001
(title: "add visibility")

[commit-bot: I haz the power, 2016-10-05 17:45:57]

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

[commit-bot: I haz the power, 2016-10-05 17:59:27]

Description was changed from

==========
Add an optimize_for_fuzzing GN flag to build most things with -O1

This is better for coverage guided fuzzing builds as less optimization
means more branches in the generated code.

BUG=643249
==========

to

==========
Add an optimize_for_fuzzing GN flag to build most things with -O1

This is better for coverage guided fuzzing builds as less optimization
means more branches in the generated code.

BUG=643249
==========

[commit-bot: I haz the power, 2016-10-05 17:59:28]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-05 18:01:46]

Description was changed from

==========
Add an optimize_for_fuzzing GN flag to build most things with -O1

This is better for coverage guided fuzzing builds as less optimization
means more branches in the generated code.

BUG=643249
==========

to

==========
Add an optimize_for_fuzzing GN flag to build most things with -O1

This is better for coverage guided fuzzing builds as less optimization
means more branches in the generated code.

BUG=643249

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

[commit-bot: I haz the power, 2016-10-05 18:01:47]

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