Make OmitFramePointer adjustable for Win/Release build.

Make OmitFramePointer adjustable for Win/Release build.
Also, remove the Optimization hard-coded flags that turned out not to be adjustable as well.

TEST=vimdiff against ipc\ipc.vcproj of three different versions:
1) old
2) new (matches "old" except for the addition of OmitFramePointers="true" for Release x64)
3) new with `GYP_DEFINES=win_release_OmitFramePointers=0 win_release_Optimization=2`

BUG=99446, 96326
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=104904

[Timur Iskhodzhanov, 2011-10-11 12:18:20]

Hello,

Can you please have a look at this change?
It adds adjustable OmitFramePointer variable to work-around
http://crbug.com/96326 for the Dr. Memory bots.

Question: is my change compatible with the overrides in
build/internal/release_impl_official.gypi ?
I think yes as I saw only the default Optimization/OmitFramePointer values
before I've noticed the build/internal/release_defaults.gypi file and changed
it.

Thanks,
Timur

http://codereview.chromium.org/8227011/diff/1/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/8227011/diff/1/build/common.gypi#newcode880
build/common.gypi:880: 'win_release_Optimization%': '2', # 2 = /Os
note this guy

http://codereview.chromium.org/8227011/diff/1/build/common.gypi#newcode1388
build/common.gypi:1388: 'Optimization': '<(win_release_Optimization)',
note this guy too

http://codereview.chromium.org/8227011/diff/1/build/common.gypi#newcode1403
build/common.gypi:1403: ['win_release_OmitFramePointers==1 or
win_release_OmitFramePointers=="true"', {
I saw a few other places where a condition doesn't fit into 80 chars - I believe
this is not a big deal here?

http://codereview.chromium.org/8227011/diff/1/build/internal/release_defaults...
File build/internal/release_defaults.gypi (left):

http://codereview.chromium.org/8227011/diff/1/build/internal/release_defaults...
build/internal/release_defaults.gypi:4: 'Optimization': '2',
these two options are redundant since the flags are adjustable from
build/common.gypi now

[Timur Iskhodzhanov, 2011-10-11 12:19:07]

bah, forgotten to add the reviewers :)
Please take a look at the prev message which has gone to /dev/null

[M-A Ruel, 2011-10-11 13:47:07]

http://codereview.chromium.org/8227011/diff/1/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/8227011/diff/1/build/common.gypi#newcode1403
build/common.gypi:1403: ['win_release_OmitFramePointers==1 or
win_release_OmitFramePointers=="true"', {
I'd recommend to remove the or. Just settle with one format, either 1 or true,
otherwise it's a maintenance burden. Then you won't have a 80 cols issue. :)

http://codereview.chromium.org/8227011/diff/1/build/internal/release_defaults...
File build/internal/release_defaults.gypi (left):

http://codereview.chromium.org/8227011/diff/1/build/internal/release_defaults...
build/internal/release_defaults.gypi:4: 'Optimization': '2',
On 2011/10/11 12:18:20, Timur Iskhodzhanov wrote:
> these two options are redundant since the flags are adjustable from
> build/common.gypi now

So I guess release_impl_official.gypi value is ignored? That's concerning.

[Timur Iskhodzhanov, 2011-10-11 13:56:00]

http://codereview.chromium.org/8227011/diff/1/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/8227011/diff/1/build/common.gypi#newcode1403
build/common.gypi:1403: ['win_release_OmitFramePointers==1 or
win_release_OmitFramePointers=="true"', {
The reasoning was:
a) VS only supports true/false
b) all other GYP flags are 0/1
=> we should support both

On 2011/10/11 13:47:08, Marc-Antoine Ruel wrote:
> I'd recommend to remove the or. Just settle with one format, either 1 or true,
> otherwise it's a maintenance burden. Then you won't have a 80 cols issue. :)

http://codereview.chromium.org/8227011/diff/1/build/internal/release_defaults...
File build/internal/release_defaults.gypi (left):

http://codereview.chromium.org/8227011/diff/1/build/internal/release_defaults...
build/internal/release_defaults.gypi:4: 'Optimization': '2',
Nope, the _impl_official value has higher priority and overrides those in
build/common.gypi [as my experiment showed. Otherwise I wouldn't find out
build/internal at all :) ]

On 2011/10/11 13:47:08, Marc-Antoine Ruel wrote:
> On 2011/10/11 12:18:20, Timur Iskhodzhanov wrote:
> > these two options are redundant since the flags are adjustable from
> > build/common.gypi now
> 
> So I guess release_impl_official.gypi value is ignored? That's concerning.

[Timur Iskhodzhanov, 2011-10-11 13:56:37]

http://codereview.chromium.org/8227011/diff/1/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/8227011/diff/1/build/common.gypi#newcode1403
build/common.gypi:1403: ['win_release_OmitFramePointers==1 or
win_release_OmitFramePointers=="true"', {
What'd you suggest?

On 2011/10/11 13:56:01, Timur Iskhodzhanov wrote:
> The reasoning was:
> a) VS only supports true/false
> b) all other GYP flags are 0/1
> => we should support both
> 
> On 2011/10/11 13:47:08, Marc-Antoine Ruel wrote:
> > I'd recommend to remove the or. Just settle with one format, either 1 or
true,
> > otherwise it's a maintenance burden. Then you won't have a 80 cols issue. :)
>

[M-A Ruel, 2011-10-11 13:59:28]

http://codereview.chromium.org/8227011/diff/1/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/8227011/diff/1/build/common.gypi#newcode1403
build/common.gypi:1403: ['win_release_OmitFramePointers==1 or
win_release_OmitFramePointers=="true"', {
On 2011/10/11 13:56:37, Timur Iskhodzhanov wrote:
> What'd you suggest?

I prefer consistency, so I'd use 0/1 and convert the value to true/false at
usage.

[Timur Iskhodzhanov, 2011-10-11 14:08:35]

OK, sounds reasonable.
PTAL

[M-A Ruel, 2011-10-11 14:15:02]

lgtm with one nit

http://codereview.chromium.org/8227011/diff/7001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/8227011/diff/7001/build/common.gypi#newcode1402
build/common.gypi:1402: # MSVS only accepts OmitFramePointers=true/false
I don't think the comment is relevant anymore. The code is simply doing the
right thing.

[Timur Iskhodzhanov, 2011-10-11 14:19:35]

Done, thanks!

Should I wait for Brad or just go for CQ?

[commit-bot: I haz the power, 2011-10-11 14:23:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/timurrrr@chromium.org/8227011/6002

[commit-bot: I haz the power, 2011-10-11 15:47:34]

Change committed as 104904