Disable TLS destructor tests on Win x64 incremental

Disable TLS destructor tests on Win x64 incremental

The linker is coalescing sections incorrectly (x64-only,
incremental-only), so disable these tests for now in that
configuration. See crbug.com/251251 for more details.

Upstream toolchain bug at https://connect.microsoft.com/VisualStudio/feedbackdetail/view/967992

R=wfh@chromium.org
BUG=251251
Committed: https://crrev.com/a3f851d9818e5e7cf74c9035abc133b6064a7284
Cr-Commit-Position: refs/heads/master@{#295770}

[scottmg, 2014-09-18 18:39:00]

wdyt, reasonable to disable? Hopefully the upstream
https://connect.microsoft.com/VisualStudio/feedbackdetail/view/967992 will be
fixed in a future toolchain revision.

[Will Harris, 2014-09-18 19:55:50]

On 2014/09/18 18:39:00, scottmg wrote:
> wdyt, reasonable to disable? Hopefully the upstream
> https://connect.microsoft.com/VisualStudio/feedbackdetail/view/967992 will be
> fixed in a future toolchain revision.

is there no way we can keep these in for the non-incremental linking e.g.
official builds?  We really don't want these to regress on x64 for other issues
(like e.g. MS update the non-incremental linker)

[scottmg, 2014-09-18 20:09:16]

On 2014/09/18 19:55:50, Will Harris wrote:
> On 2014/09/18 18:39:00, scottmg wrote:
> > wdyt, reasonable to disable? Hopefully the upstream
> > https://connect.microsoft.com/VisualStudio/feedbackdetail/view/967992 will
be
> > fixed in a future toolchain revision.
> 
> is there no way we can keep these in for the non-incremental linking e.g.
> official builds?  We really don't want these to regress on x64 for other
issues
> (like e.g. MS update the non-incremental linker)

We don't know incremental-ness at compile time. We could only do it for
non-OFFICIAL_BUILD I guess? So we would we'd notice (though it'd be a bit late)?

[scottmg, 2014-09-18 20:10:45]

On 2014/09/18 20:09:16, scottmg wrote:
> On 2014/09/18 19:55:50, Will Harris wrote:
> > On 2014/09/18 18:39:00, scottmg wrote:
> > > wdyt, reasonable to disable? Hopefully the upstream
> > > https://connect.microsoft.com/VisualStudio/feedbackdetail/view/967992 will
> be
> > > fixed in a future toolchain revision.
> > 
> > is there no way we can keep these in for the non-incremental linking e.g.
> > official builds?  We really don't want these to regress on x64 for other
> issues
> > (like e.g. MS update the non-incremental linker)
> 
> We don't know incremental-ness at compile time. We could only do it for
> non-OFFICIAL_BUILD I guess? So we would we'd notice (though it'd be a bit
late)?

(that had a lot of question marks, and didn't make much sense, but maybe you
catch what I mean anyway...)

[Will Harris, 2014-09-18 21:15:49]

On 2014/09/18 20:10:45, scottmg wrote:
> On 2014/09/18 20:09:16, scottmg wrote:
> > On 2014/09/18 19:55:50, Will Harris wrote:
> > > On 2014/09/18 18:39:00, scottmg wrote:
> > > > wdyt, reasonable to disable? Hopefully the upstream
> > > > https://connect.microsoft.com/VisualStudio/feedbackdetail/view/967992
will
> > be
> > > > fixed in a future toolchain revision.
> > > 
> > > is there no way we can keep these in for the non-incremental linking e.g.
> > > official builds?  We really don't want these to regress on x64 for other
> > issues
> > > (like e.g. MS update the non-incremental linker)
> > 
> > We don't know incremental-ness at compile time. We could only do it for
> > non-OFFICIAL_BUILD I guess? So we would we'd notice (though it'd be a bit
> late)?
> 
> (that had a lot of question marks, and didn't make much sense, but maybe you
> catch what I mean anyway...)

or force disable incremental linking on base_unittests..? - but then we would
miss the situation where we decide to go incremental for our official
builds some time in the future, and are back to losing test coverage.

I'm just worried that since we've already been stung here by a toolchain oddity
it might come back to bite us some point in the future unless we specifically
put scary warnings somewhere in the gyp/GN files to protect against it.

[scottmg, 2014-09-18 21:18:07]

We can't ever go incremental for Official, it pads the binary and it's not
compatible with /GL and /LTCG anyway.

So, I think OFFICIAL_BUILD seems safe, other than we won't notice until
late (it hits a bot that actually does OFFICIAL_BUILD).

On Thu, Sep 18, 2014 at 2:15 PM, <wfh@chromium.org> wrote:

> On 2014/09/18 20:10:45, scottmg wrote:
>
>> On 2014/09/18 20:09:16, scottmg wrote:
>> > On 2014/09/18 19:55:50, Will Harris wrote:
>> > > On 2014/09/18 18:39:00, scottmg wrote:
>> > > > wdyt, reasonable to disable? Hopefully the upstream
>> > > > https://connect.microsoft.com/VisualStudio/feedbackdetail/
>> view/967992
>>
> will
>
>> > be
>> > > > fixed in a future toolchain revision.
>> > >
>> > > is there no way we can keep these in for the non-incremental linking
>> e.g.
>> > > official builds?  We really don't want these to regress on x64 for
>> other
>> > issues
>> > > (like e.g. MS update the non-incremental linker)
>> >
>> > We don't know incremental-ness at compile time. We could only do it for
>> > non-OFFICIAL_BUILD I guess? So we would we'd notice (though it'd be a
>> bit
>> late)?
>>
>
>  (that had a lot of question marks, and didn't make much sense, but maybe
>> you
>> catch what I mean anyway...)
>>
>
> or force disable incremental linking on base_unittests..? - but then we
> would
> miss the situation where we decide to go incremental for our official
> builds some time in the future, and are back to losing test coverage.
>
> I'm just worried that since we've already been stung here by a toolchain
> oddity
> it might come back to bite us some point in the future unless we
> specifically
> put scary warnings somewhere in the gyp/GN files to protect against it.
>
>
>
>
> https://codereview.chromium.org/580213002/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Will Harris, 2014-09-18 21:19:41]

On 2014/09/18 21:15:49, Will Harris wrote:
> On 2014/09/18 20:10:45, scottmg wrote:
> > On 2014/09/18 20:09:16, scottmg wrote:
> > > On 2014/09/18 19:55:50, Will Harris wrote:
> > > > On 2014/09/18 18:39:00, scottmg wrote:
> > > > > wdyt, reasonable to disable? Hopefully the upstream
> > > > > https://connect.microsoft.com/VisualStudio/feedbackdetail/view/967992
> will
> > > be
> > > > > fixed in a future toolchain revision.
> > > > 
> > > > is there no way we can keep these in for the non-incremental linking
e.g.
> > > > official builds?  We really don't want these to regress on x64 for other
> > > issues
> > > > (like e.g. MS update the non-incremental linker)
> > > 
> > > We don't know incremental-ness at compile time. We could only do it for
> > > non-OFFICIAL_BUILD I guess? So we would we'd notice (though it'd be a bit
> > late)?
> > 
> > (that had a lot of question marks, and didn't make much sense, but maybe you
> > catch what I mean anyway...)
> 
> or force disable incremental linking on base_unittests..? - but then we would
> miss the situation where we decide to go incremental for our official
> builds some time in the future, and are back to losing test coverage.
> 
> I'm just worried that since we've already been stung here by a toolchain
oddity
> it might come back to bite us some point in the future unless we specifically
> put scary warnings somewhere in the gyp/GN files to protect against it.

Can gyp/GN define a preprocessor variable when incremental_chrome_dll is set
and then we can Maybe on this in the test case (with a link to bug 251251)

[scottmg, 2014-09-18 21:24:44]

On 2014/09/18 21:19:41, Will Harris wrote:
> On 2014/09/18 21:15:49, Will Harris wrote:
> > On 2014/09/18 20:10:45, scottmg wrote:
> > > On 2014/09/18 20:09:16, scottmg wrote:
> > > > On 2014/09/18 19:55:50, Will Harris wrote:
> > > > > On 2014/09/18 18:39:00, scottmg wrote:
> > > > > > wdyt, reasonable to disable? Hopefully the upstream
> > > > > >
https://connect.microsoft.com/VisualStudio/feedbackdetail/view/967992
> > will
> > > > be
> > > > > > fixed in a future toolchain revision.
> > > > > 
> > > > > is there no way we can keep these in for the non-incremental linking
> e.g.
> > > > > official builds?  We really don't want these to regress on x64 for
other
> > > > issues
> > > > > (like e.g. MS update the non-incremental linker)
> > > > 
> > > > We don't know incremental-ness at compile time. We could only do it for
> > > > non-OFFICIAL_BUILD I guess? So we would we'd notice (though it'd be a
bit
> > > late)?
> > > 
> > > (that had a lot of question marks, and didn't make much sense, but maybe
you
> > > catch what I mean anyway...)
> > 
> > or force disable incremental linking on base_unittests..? - but then we
would
> > miss the situation where we decide to go incremental for our official
> > builds some time in the future, and are back to losing test coverage.
> > 
> > I'm just worried that since we've already been stung here by a toolchain
> oddity
> > it might come back to bite us some point in the future unless we
specifically
> > put scary warnings somewhere in the gyp/GN files to protect against it.
> 
> Can gyp/GN define a preprocessor variable when incremental_chrome_dll is set
> and then we can Maybe on this in the test case (with a link to bug 251251)

Yeah, we could do that. I don't really want people changing behaviour based on
that though (i.e. like this CL).

[Will Harris, 2014-09-18 21:26:01]

On 2014/09/18 21:24:44, scottmg wrote:
> On 2014/09/18 21:19:41, Will Harris wrote:
> > On 2014/09/18 21:15:49, Will Harris wrote:
> > > On 2014/09/18 20:10:45, scottmg wrote:
> > > > On 2014/09/18 20:09:16, scottmg wrote:
> > > > > On 2014/09/18 19:55:50, Will Harris wrote:
> > > > > > On 2014/09/18 18:39:00, scottmg wrote:
> > > > > > > wdyt, reasonable to disable? Hopefully the upstream
> > > > > > >
> https://connect.microsoft.com/VisualStudio/feedbackdetail/view/967992
> > > will
> > > > > be
> > > > > > > fixed in a future toolchain revision.
> > > > > > 
> > > > > > is there no way we can keep these in for the non-incremental linking
> > e.g.
> > > > > > official builds?  We really don't want these to regress on x64 for
> other
> > > > > issues
> > > > > > (like e.g. MS update the non-incremental linker)
> > > > > 
> > > > > We don't know incremental-ness at compile time. We could only do it
for
> > > > > non-OFFICIAL_BUILD I guess? So we would we'd notice (though it'd be a
> bit
> > > > late)?
> > > > 
> > > > (that had a lot of question marks, and didn't make much sense, but maybe
> you
> > > > catch what I mean anyway...)
> > > 
> > > or force disable incremental linking on base_unittests..? - but then we
> would
> > > miss the situation where we decide to go incremental for our official
> > > builds some time in the future, and are back to losing test coverage.
> > > 
> > > I'm just worried that since we've already been stung here by a toolchain
> > oddity
> > > it might come back to bite us some point in the future unless we
> specifically
> > > put scary warnings somewhere in the gyp/GN files to protect against it.
> > 
> > Can gyp/GN define a preprocessor variable when incremental_chrome_dll is set
> > and then we can Maybe on this in the test case (with a link to bug 251251)
> 
> Yeah, we could do that. I don't really want people changing behaviour based on
> that though (i.e. like this CL).

just precede the #define with a triple underscore and nobody will use it :)

[scottmg, 2014-09-19 00:44:35]

Fine, fine. Done, localized only to base_unittests.

[Will Harris, 2014-09-19 00:52:21]

lgtm

[scottmg, 2014-09-19 00:53:55]

scottmg@chromium.org changed reviewers:
+ thakis@chomium.org

[scottmg, 2014-09-19 00:54:04]

+thakis for OWNERS

[scottmg, 2014-09-19 18:27:58]

scottmg@chromium.org changed reviewers:
+ brettw@chromium.org
- thakis@chomium.org

[scottmg, 2014-09-19 18:28:11]

or how about brettw for owners

[brettw, 2014-09-19 19:27:56]

lgtm

[scottmg, 2014-09-19 19:29:08]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-09-19 19:30:34]

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

[commit-bot: I haz the power, 2014-09-19 20:28:45]

Committed patchset #4 (id:60001) as caf8d346e003a7948cd12ce0c3b5cbf1ece7f859

[commit-bot: I haz the power, 2014-09-19 20:28:56]

Committed patchset #4 (id:60001) as 5a95f25be43aa2463ae6d86e8c94d0d399f8377e

[commit-bot: I haz the power, 2014-09-19 20:30:37]

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