Use early expansion for debug_optimize and release_optimize

Use early expansion for debug_optimize and release_optimize

This fixes a bug when these flags are not overriden from ~/.gyp/include.gypi
which is the way they should be set for Valgrind builds
(see http://dev.chromium.org/developers/how-tos/using-valgrind )

BUG=70107
TEST=gclient runhooks --force && grep "\-O" base/base.target.mk

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=73304

[Timur Iskhodzhanov, 2011-01-19 17:20:31]

Hi Albert, Mark,

I'm investigating http://crbug.com/70107

I wonder if http://codereview.chromium.org/334018 was right.
What I see right now is my debug_optimize=1 and release_optimize=1 values from
~/.gyp/include.gypi are not affecting makefiles:
# With default build/common.gyp
$ gclient runhooks --force && grep "\-O" base/base.target.mk
	-O0 \
	-O2 \
...
This makes the Valgrind build instructions a bit incorrect:
http://dev.chromium.org/developers/how-tos/using-valgrind

If I replace ">" with "<" as it is for all other variables in build/common.gypi,
the values are propagated:
$ gclient runhooks --force && grep "\-O" base/base.target.mk
	-O1 \
	-O1 \
...

Are you sure you still need ">" instead of "<"?
If yes - can you please suggest a workaround so the ~/.gyp/include.gypi values
are propagated?
If no - can you please review the patch? (it has passed trybots)

Thank you,
Timur Iskhodzhanov

[Mark Mentovai, 2011-01-24 19:14:09]

http://codereview.chromium.org/6267004/diff/2001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/6267004/diff/2001/build/common.gypi#newcode954
build/common.gypi:954: '-O<(debug_optimize)',
The point of late expansion is that individual targets ought to be able to
override this as needed. In practice, this is rarely needed, and if I see it
being done anywhere, chances are I’ll give the person who wrote it a hard time
until I’m satisfied that it’s absolutely necessary. The media team (and Albert
and Frank) convinced me that it was necessary in the one spot I’m aware of where
it’s actually used.

Your proposed change is incorrect, but I understand that your needs may be
different. I see that you’ve already hunted down the original review for this
line. As I recall, some media code needed to ensure that a specific value would
be used here or the generated code would be really bad and really slow, and I’m
sure matters would be even worse under Valgrind. If you’re willing to tolerate
that sort of badness and slowness, I’m sure we can cook up a way to force this
to use a different value in Valgrind-land.

[Timur Iskhodzhanov, 2011-01-24 23:05:52]

On 2011/01/24 19:14:09, Mark Mentovai wrote:
> http://codereview.chromium.org/6267004/diff/2001/build/common.gypi
> File build/common.gypi (right):
> 
> http://codereview.chromium.org/6267004/diff/2001/build/common.gypi#newcode954
> build/common.gypi:954: '-O<(debug_optimize)',
> The point of late expansion is that individual targets ought to be able to
> override this as needed. In practice, this is rarely needed, and if I see it
> being done anywhere, chances are I’ll give the person who wrote it a hard time
> until I’m satisfied that it’s absolutely necessary. The media team (and Albert
> and Frank) convinced me that it was necessary in the one spot I’m aware of
where
> it’s actually used.
> 
> Your proposed change is incorrect, but I understand that your needs may be
> different. I see that you’ve already hunted down the original review for this
> line.
Well, in the original review the argument was that ffmpeg needs a different -O
flag. Looks like this is not the case anymore - I've sent the patch to all
dbg/rel trybots and it succeeded.
OTOH I admit I'm not that familiar with the Chromium build system so it's up to
you to decide if the original change is still needed.

> As I recall, some media code needed to ensure that a specific value would
> be used here or the generated code would be really bad and really slow, and
I’m
> sure matters would be even worse under Valgrind. If you’re willing to tolerate
> that sort of badness and slowness, I’m sure we can cook up a way to force this
> to use a different value in Valgrind-land.
The problem with Valgrind is not the slowness but rather the correctness.
In fact, Valgrind tools may produce false error reports and/or corrupted stacks
if the code is built with -O2 or more agressive optimization. What surprises (or
rather annoys) me is that it appears we can't override release_optimize for
Valgrind builds now - at least, in an easy way.
I'm fine with leaving the ">"s as they are if you can help me work around the
issues listed in the "Please review" e-mail.

[Mark Mentovai, 2011-01-24 23:22:17]

timurrrr@chromium.org wrote:
> Well, in the original review the argument was that ffmpeg needs a different -O
> flag. Looks like this is not the case anymore - I've sent the patch to all
> dbg/rel trybots and it succeeded.
> OTOH I admit I'm not that familiar with the Chromium build system so it's up
to
> you to decide if the original change is still needed.

OK, let’s wait for Albert to chime in and see if he remembers. In
http://codereview.chromium.org/334018, he said “the ffmpeg build
requires this because it has problems compiling with -O0” but
“requires” and “has problems” may be open to interpretation. The
“problems” may not have necessarily been “doesn’t compile” or “doesn’t
run,” or they may have been compilation or correctness problems
experienced only under certain circumstances or in certain
configurations. As an example, this may present problems only when
inline assembly is in use, and the compiler’s own code generation
places different constraints on the available registers for assembly
sections when different optimization levels are selected. ffmpeg can
also be built with or without certain additional features included,
and the default set varies between open-source Chromium and official
Google Chrome builds.

If you want to dig further, there may be more morsels in
http://codereview.chromium.org/300013, which probably holds some sort
of record.

Albert?

[awong, 2011-01-28 01:13:46]

Oops...lost this under a pile of e-mail.

On 2011/01/24 23:22:17, Mark Mentovai wrote:
> mailto:timurrrr@chromium.org wrote:
> > Well, in the original review the argument was that ffmpeg needs a different
-O
> > flag. Looks like this is not the case anymore - I've sent the patch to all
> > dbg/rel trybots and it succeeded.
> > OTOH I admit I'm not that familiar with the Chromium build system so it's up
> to
> > you to decide if the original change is still needed.
> 
> OK, let’s wait for Albert to chime in and see if he remembers. In
> http://codereview.chromium.org/334018, he said “the ffmpeg build
> requires this because it has problems compiling with -O0” but
> “requires” and “has problems” may be open to interpretation. The
> “problems” may not have necessarily been “doesn’t compile” or “doesn’t
> run,” or they may have been compilation or correctness problems
> experienced only under certain circumstances or in certain
> configurations. As an example, this may present problems only when
> inline assembly is in use, and the compiler’s own code generation
> places different constraints on the available registers for assembly
> sections when different optimization levels are selected. ffmpeg can
> also be built with or without certain additional features included,
> and the default set varies between open-source Chromium and official
> Google Chrome builds.
> 
> If you want to dig further, there may be more morsels in
> http://codereview.chromium.org/300013, which probably holds some sort
> of record.
> 
> Albert?

The "has problems" statement was definitely in the "doesn't compile or link"
category.

IIRC, the issue with -O0 in ffmpeg was actually due to some dangling code
references that got eliminated by any -O greater than 0.  If you used -O0, you'd
get a link error.  This was in addition to the register exhaustion issues that
Mark highlighted.

Since that time, ffmpeg has been updated so much that I no longer have a clue if
these issues are still present.  In particular, I think they cleaned up a bunch
of the issues around PIC code, inline assembly, and dead branches.

Too fully test this, you'd need to try buildling our ffmpeg in every
configuration we support.  However, I think you can get pretty good confidence
by just making sure that you can complete a 32-bit debug build, with -O0 set.

Give that a shot and see what happens?

[Timur Iskhodzhanov, 2011-02-01 01:14:53]

On 2011/01/28 01:13:46, awong wrote:
> Oops...lost this under a pile of e-mail.
> 
> On 2011/01/24 23:22:17, Mark Mentovai wrote:
> > mailto:timurrrr@chromium.org wrote:
> > > Well, in the original review the argument was that ffmpeg needs a
different
> -O
> > > flag. Looks like this is not the case anymore - I've sent the patch to all
> > > dbg/rel trybots and it succeeded.
> > > OTOH I admit I'm not that familiar with the Chromium build system so it's
up
> > to
> > > you to decide if the original change is still needed.
> > 
> > OK, let’s wait for Albert to chime in and see if he remembers. In
> > http://codereview.chromium.org/334018, he said “the ffmpeg build
> > requires this because it has problems compiling with -O0” but
> > “requires” and “has problems” may be open to interpretation. The
> > “problems” may not have necessarily been “doesn’t compile” or “doesn’t
> > run,” or they may have been compilation or correctness problems
> > experienced only under certain circumstances or in certain
> > configurations. As an example, this may present problems only when
> > inline assembly is in use, and the compiler’s own code generation
> > places different constraints on the available registers for assembly
> > sections when different optimization levels are selected. ffmpeg can
> > also be built with or without certain additional features included,
> > and the default set varies between open-source Chromium and official
> > Google Chrome builds.
> > 
> > If you want to dig further, there may be more morsels in
> > http://codereview.chromium.org/300013, which probably holds some sort
> > of record.
> > 
> > Albert?
> 
> The "has problems" statement was definitely in the "doesn't compile or link"
> category.
> 
> IIRC, the issue with -O0 in ffmpeg was actually due to some dangling code
> references that got eliminated by any -O greater than 0.  If you used -O0,
you'd
> get a link error.  This was in addition to the register exhaustion issues that
> Mark highlighted.
> 
> Since that time, ffmpeg has been updated so much that I no longer have a clue
if
> these issues are still present.  In particular, I think they cleaned up a
bunch
> of the issues around PIC code, inline assembly, and dead branches.
> 
> Too fully test this, you'd need to try buildling our ffmpeg in every
> configuration we support.  However, I think you can get pretty good confidence
> by just making sure that you can complete a 32-bit debug build, with -O0 set.
>
> Give that a shot and see what happens?
I did the following:
$ rm ~/.gyp/includes.gypi
$ GYP_DEFINES="target_arch=ia32" ../depot_tools/gclient sync && make -j7 chrome
-> Passed
$ grep "\-O" base/base.target.mk
	-O0 \
	-O2 \
	-Wl,-O1 \
(so the Debug build was done with -O0 as expected)

Given that the trybots succeded (expect for the 2nd Win job which has one test
failure which is likely a flake) I think this change is OK.

Since I have limited number of platforms and the change is relatively difficult
to test - what do you think about crossing fingers and commiting? We can revert
it if we see any problems.

[Mark Mentovai, 2011-02-01 17:52:21]

I guess there’s nothing wrong with trying it, then. Be sure to monitor the
official builds after you make this change. Ideally you’d have tested an
official build on at least Linux or Mac before landing this change too.

LGTM

[Timur Iskhodzhanov, 2011-02-01 19:57:49]

The change was reverted due to Clang failures.
Nico told me he'll take a look if this is a Clang bug.