Only use /Gw in non-ASan builds on Windows

Only use /Gw in non-ASan builds on Windows

BUG=445383
NOTRY=true

Committed: https://crrev.com/9bdaf3bf8c746cfcee5c2f318b8b3fb3edd782eb
Cr-Commit-Position: refs/heads/master@{#309742}

[Timur Iskhodzhanov, 2014-12-29 19:41:33]

timurrrr@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2014-12-29 19:50:21]

lgtm, nice catch :-)

(bruce: fyi)

https://codereview.chromium.org/811083003/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/811083003/diff/1/build/common.gypi#newcode3376
build/common.gypi:3376: # Put data in separate COMDATs, unless ASan is enabled.
nit: The "unless ASan is enabled" only repeats the conditional instead of saying
_why_ this is done. Maybe "Put data in separate COMDATs. This allows the linker
to put bit-identical constants at the same address even if they're unrelated
constants, which saves binary size – but it's incompatible with ASan which
tracks metadata for every address and assumes that every address belongs to only
one object" (there's probably a more concise phrasing of that)?

[Timur Iskhodzhanov, 2014-12-29 20:07:29]

Thanks!

https://codereview.chromium.org/811083003/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/811083003/diff/1/build/common.gypi#newcode3376
build/common.gypi:3376: # Put data in separate COMDATs, unless ASan is enabled.
Done (we came up with an even better version offline)

[Timur Iskhodzhanov, 2014-12-29 20:07:32]

The CQ bit was checked by timurrrr@chromium.org

[commit-bot: I haz the power, 2014-12-29 20:07:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/811083003/20001

[brucedawson, 2014-12-29 20:21:12]

One could make a case for disabling /OPT:ICF in ASAN builds instead of disabling
/Gw, since /OPT:ICF is the linker option that controls the collapsing of bit
identical COMDATs (both code and data). But, as long as it is only data
collapsing that is causing problems then disabling /Gw works quite nicely, and
has the lowest probability of having unintended consequences.

[commit-bot: I haz the power, 2014-12-29 22:00:16]

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

[commit-bot: I haz the power, 2014-12-29 22:00:17]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[Nico, 2014-12-29 22:01:27]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2014-12-29 22:02:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/811083003/20001

[commit-bot: I haz the power, 2014-12-29 23:58:15]

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

[commit-bot: I haz the power, 2014-12-29 23:58:15]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[Nico, 2014-12-30 02:03:28]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2014-12-30 02:03:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/811083003/20001

[commit-bot: I haz the power, 2014-12-30 02:04:52]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-12-30 02:06:45]

Patchset 2 (id:??) landed as
https://crrev.com/9bdaf3bf8c746cfcee5c2f318b8b3fb3edd782eb
Cr-Commit-Position: refs/heads/master@{#309742}

[Timur Iskhodzhanov, 2014-12-30 13:55:17]

On 2014/12/29 20:21:12, brucedawson wrote:
> One could make a case for disabling /OPT:ICF in ASAN builds instead of
disabling
> /Gw, since /OPT:ICF is the linker option that controls the collapsing of bit
> identical COMDATs (both code and data). But, as long as it is only data
> collapsing that is causing problems then disabling /Gw works quite nicely, and
> has the lowest probability of having unintended consequences.

Thanks for the idea!
There is likely a precision vs binary size trade-off there, so I wouldn't jump
into trying that just yet.

I've filed
https://code.google.com/p/chromium/issues/detail?id=445577
to think about that later.