Add support for configurable -OX optimization levels for debug builds

Add support for configurable -OX optimization levels for debug builds

Can be used via -dX or --debug-opt-level=X, e.g.

  tools/gn.py --mode=debug --arch=x64 -d0

R=vegorov@google.com, zra@google.com

Committed: https://github.com/dart-lang/sdk/commit/4e34234c28275a67a56e585a6e926e663dcc10cb

[kustermann, 2017-01-23 09:49:40]

Description was changed from

==========
Add support for configurable -OX optimization levels for debug builds
==========

to

==========
Add support for configurable -OX optimization levels for debug builds

Can be used via -dX or --debug-opt-level=X, e.g.

  tools/gn.py --mode=debug --arch=x64 -d0
==========

[kustermann, 2017-01-23 09:49:40]

kustermann@google.com changed reviewers:
+ vegorov@google.com, zra@google.com

[Kevin Millikin (Google), 2017-01-23 13:32:22]

kmillikin@google.com changed reviewers:
+ kmillikin@google.com

[Kevin Millikin (Google), 2017-01-23 13:32:22]

https://codereview.chromium.org/2645323002/diff/1/runtime/BUILD.gn
File runtime/BUILD.gn (right):

https://codereview.chromium.org/2645323002/diff/1/runtime/BUILD.gn#newcode169
runtime/BUILD.gn:169: cflags += [ "-O${dart_debug_optimization_level}" ]
This flag seems to be injected two places: once here and once in
build/config/compiler/BUILD.gn.

Is it the lower one that takes precedence?  The second one?

[kustermann, 2017-01-23 13:59:11]

https://codereview.chromium.org/2645323002/diff/1/runtime/BUILD.gn
File runtime/BUILD.gn (right):

https://codereview.chromium.org/2645323002/diff/1/runtime/BUILD.gn#newcode169
runtime/BUILD.gn:169: cflags += [ "-O${dart_debug_optimization_level}" ]
On 2017/01/23 13:32:22, Kevin Millikin (Google) wrote:
> This flag seems to be injected two places: once here and once in
> build/config/compiler/BUILD.gn.
> 
> Is it the lower one that takes precedence?  The second one?

Yes, afaik.

[kustermann, 2017-01-23 14:11:54]

https://codereview.chromium.org/2645323002/diff/1/runtime/BUILD.gn
File runtime/BUILD.gn (right):

https://codereview.chromium.org/2645323002/diff/1/runtime/BUILD.gn#newcode169
runtime/BUILD.gn:169: cflags += [ "-O${dart_debug_optimization_level}" ]
On 2017/01/23 13:59:11, kustermann wrote:
> On 2017/01/23 13:32:22, Kevin Millikin (Google) wrote:
> > This flag seems to be injected two places: once here and once in
> > build/config/compiler/BUILD.gn.
> > 
> > Is it the lower one that takes precedence?  The second one?
> 
> Yes, afaik.

Sorry, to clarify: It is the second one.

"""
If you use multiple -O options, with or without level numbers, the last such
option is the one that is effective.
"""

[Vyacheslav Egorov (Google), 2017-01-23 14:42:48]

lgtm

[zra, 2017-01-23 16:39:54]

https://codereview.chromium.org/2645323002/diff/1/runtime/BUILD.gn
File runtime/BUILD.gn (right):

https://codereview.chromium.org/2645323002/diff/1/runtime/BUILD.gn#newcode169
runtime/BUILD.gn:169: cflags += [ "-O${dart_debug_optimization_level}" ]
On 2017/01/23 14:11:54, kustermann wrote:
> On 2017/01/23 13:59:11, kustermann wrote:
> > On 2017/01/23 13:32:22, Kevin Millikin (Google) wrote:
> > > This flag seems to be injected two places: once here and once in
> > > build/config/compiler/BUILD.gn.
> > > 
> > > Is it the lower one that takes precedence?  The second one?
> > 
> > Yes, afaik.
> 
> Sorry, to clarify: It is the second one.
> 
> """
> If you use multiple -O options, with or without level numbers, the last such
> option is the one that is effective.
> """

We probably need to do this in both places, both for clarity and also to make
this work on Windows. This setting also does not cover third_party dependencies.

I think we should do this by adding a GN argument to
build/config/compiler/BUILD.gn, like "debug_optimization_level" in addition to
this one. (We need both so that we can override this one, and to set the debug
optimization level from the Flutter engine build.) In our gn.py, we'd set them
both to the same thing.

[zra, 2017-01-23 16:41:15]

On 2017/01/23 16:39:54, zra wrote:
> https://codereview.chromium.org/2645323002/diff/1/runtime/BUILD.gn
> File runtime/BUILD.gn (right):
> 
> https://codereview.chromium.org/2645323002/diff/1/runtime/BUILD.gn#newcode169
> runtime/BUILD.gn:169: cflags += [ "-O${dart_debug_optimization_level}" ]
> On 2017/01/23 14:11:54, kustermann wrote:
> > On 2017/01/23 13:59:11, kustermann wrote:
> > > On 2017/01/23 13:32:22, Kevin Millikin (Google) wrote:
> > > > This flag seems to be injected two places: once here and once in
> > > > build/config/compiler/BUILD.gn.
> > > > 
> > > > Is it the lower one that takes precedence?  The second one?
> > > 
> > > Yes, afaik.
> > 
> > Sorry, to clarify: It is the second one.
> > 
> > """
> > If you use multiple -O options, with or without level numbers, the last such
> > option is the one that is effective.
> > """
> 
> We probably need to do this in both places, both for clarity and also to make
> this work on Windows. This setting also does not cover third_party
dependencies.
> 
> I think we should do this by adding a GN argument to
> build/config/compiler/BUILD.gn, like "debug_optimization_level" in addition to
> this one. (We need both so that we can override this one, and to set the debug
> optimization level from the Flutter engine build.) In our gn.py, we'd set them
> both to the same thing.

But otherwise, this approach seems right to me.

[kustermann, 2017-01-24 11:44:52]

PTAL

https://codereview.chromium.org/2645323002/diff/1/runtime/BUILD.gn
File runtime/BUILD.gn (right):

https://codereview.chromium.org/2645323002/diff/1/runtime/BUILD.gn#newcode169
runtime/BUILD.gn:169: cflags += [ "-O${dart_debug_optimization_level}" ]
Done.


> This setting also does not cover third_party dependencies.

As a VM developer I would like to not have "<optimized out>" issues with gdb in
VM code. I would be perfectly fine with having tcmalloc/boringssl/embedder/...
compile with -O2 so the tests run faster. But I don't really have a strong
opinion on this.

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

https://codereview.chromium.org/2645323002/diff/20001/build/config/compiler/B...
build/config/compiler/BUILD.gn:762: # much even when optimization is disabled to
get the binary size down.
Not sure if this comment applies to ldflags as well, so I left it here.

[zra, 2017-01-24 16:00:37]

lgtm w/ nit

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

https://codereview.chromium.org/2645323002/diff/20001/build/config/compiler/B...
build/config/compiler/BUILD.gn:762: # much even when optimization is disabled to
get the binary size down.
On 2017/01/24 11:44:51, kustermann wrote:
> Not sure if this comment applies to ldflags as well, so I left it here.

Yah, let's leave it.

https://codereview.chromium.org/2645323002/diff/20001/tools/gn.py
File tools/gn.py (right):

https://codereview.chromium.org/2645323002/diff/20001/tools/gn.py#newcode316
tools/gn.py:316: other_group.add_argument('--debug-opt-level',
Let's leave --clang and --no-clang adjacent, and move this after --no-clang.

[kustermann, 2017-01-24 16:36:18]

Thanks.

https://codereview.chromium.org/2645323002/diff/20001/tools/gn.py
File tools/gn.py (right):

https://codereview.chromium.org/2645323002/diff/20001/tools/gn.py#newcode316
tools/gn.py:316: other_group.add_argument('--debug-opt-level',
On 2017/01/24 16:00:36, zra wrote:
> Let's leave --clang and --no-clang adjacent, and move this after --no-clang.

Done.

[kustermann, 2017-01-24 16:36:44]

Description was changed from

==========
Add support for configurable -OX optimization levels for debug builds

Can be used via -dX or --debug-opt-level=X, e.g.

  tools/gn.py --mode=debug --arch=x64 -d0
==========

to

==========
Add support for configurable -OX optimization levels for debug builds

Can be used via -dX or --debug-opt-level=X, e.g.

  tools/gn.py --mode=debug --arch=x64 -d0

R=vegorov@google.com, zra@google.com

Committed:
https://github.com/dart-lang/sdk/commit/4e34234c28275a67a56e585a6e926e663dcc10cb
==========

[kustermann, 2017-01-24 16:36:46]

Committed patchset #3 (id:40001) manually as
4e34234c28275a67a56e585a6e926e663dcc10cb (presubmit successful).