Only use sanitizers with default toolchain on Windows as well.

Only use sanitizers with default toolchain on Windows as well.

There's currently code to disable sanitizers for non-default toolchains
in gcc_toolchain.gni.  Move it to sanitizers.gni, then it's closer to
where all these args are declared and it works on Windows as well.

No intended behavior change on non-Windows.

BUG=598761
TBR=dpranke

Committed: https://crrev.com/146d122ae8ed166f294231fbe6ca340ceffc356b
Cr-Commit-Position: refs/heads/master@{#409963}

[Nico, 2016-08-05 00:50:08]

The CQ bit was checked by thakis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-05 00:50:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nico, 2016-08-05 00:51:32]

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

[Nico, 2016-08-05 00:51:33]

This is for https://bugs.chromium.org/p/chromium/issues/detail?id=598761#c63

This seemed a bit nicer to me than duplicating this (as
https://codereview.chromium.org/2198333002/diff/40001/build/toolchain/win/BUI...
did).

[Nico, 2016-08-05 01:43:38]

ima tbr this – it seems to work locally (I generated asan ninja files on mac
with and without this patch and verified they're identical except for gn
nondeterminism in writing ninja files, and I checked that an the ninja files
below nacl_clang_win64 in an is_asan win build no longer contain
-fsanitize=address), and I'd like the bots to cycle with this over night. I'm
going to address eventual comments in a follow-up.

[Nico, 2016-08-05 01:43:54]

Description was changed from

==========
Only use sanitizers with default toolchain on Windows as well.

There's currently code to disable sanitizers for non-default toolchains
in gcc_toolchain.gni.  Move it to sanitizers.gni, then it's closer to
where all these args are declared and it works on Windows as well.

No intended behavior change on non-Windows.

BUG=598761
==========

to

==========
Only use sanitizers with default toolchain on Windows as well.

There's currently code to disable sanitizers for non-default toolchains
in gcc_toolchain.gni.  Move it to sanitizers.gni, then it's closer to
where all these args are declared and it works on Windows as well.

No intended behavior change on non-Windows.

BUG=598761
TBR=dpranke
==========

[commit-bot: I haz the power, 2016-08-05 01:48:22]

Description was changed from

==========
Only use sanitizers with default toolchain on Windows as well.

There's currently code to disable sanitizers for non-default toolchains
in gcc_toolchain.gni.  Move it to sanitizers.gni, then it's closer to
where all these args are declared and it works on Windows as well.

No intended behavior change on non-Windows.

BUG=598761
TBR=dpranke
==========

to

==========
Only use sanitizers with default toolchain on Windows as well.

There's currently code to disable sanitizers for non-default toolchains
in gcc_toolchain.gni.  Move it to sanitizers.gni, then it's closer to
where all these args are declared and it works on Windows as well.

No intended behavior change on non-Windows.

BUG=598761
TBR=dpranke

Committed: https://crrev.com/146d122ae8ed166f294231fbe6ca340ceffc356b
Cr-Commit-Position: refs/heads/master@{#409963}
==========

[commit-bot: I haz the power, 2016-08-05 01:48:22]

Patchset 1 (id:??) landed as
https://crrev.com/146d122ae8ed166f294231fbe6ca340ceffc356b
Cr-Commit-Position: refs/heads/master@{#409963}

[Dirk Pranke, 2016-08-05 20:17:10]

it's a little strange (i.e., unidiomatic) to un-set values in the .gni file like
this, but it could be that the idiom is just wrong. 

In particular, I wonder about cases where you do really want to also instrument
things in *some* non-default toolchains (e.g., some of the x64 stuff that is
built for nacl on an x86 win build). However, expecting every toolchain to have
to know about every variable and every context also doesn't scale.

I'm not sure what the best way to do this is, so I guess this change is as good
as any.

[brettw, 2016-08-06 04:48:19]

I think this approach is undesirable in GN, just as it would be in C++.

Some alternatives.

1.:
  is_default_toolchain = current_toolchain == defualt_toolchain
  is_asan = is_default_toolchain && ...

2.:
  if (current_toolchain == default_toolchain) {
    declare_args() {
      is_asan = ...
    }
  } else {
    is_asan = false
This latter one may not actually work, GN like to try to make you define
everything in one place, so if you like this approach try a test first.

[Nico, 2016-08-09 01:32:53]

Sorry, I forgot about this until now. Thanks for the concrete suggestions!

On 2016/08/06 04:48:19, brettw (ping on IM after 24h) wrote:
> I think this approach is undesirable in GN, just as it would be in C++.
> 
> Some alternatives.
> 
> 1.:
>   is_default_toolchain = current_toolchain == defualt_toolchain
>   is_asan = is_default_toolchain && ...

I don't understand this suggestion. Where does this go? In the declare args?
Instead of

declare_args() {
  is_asan = false
}

have

declare_args() {
  is_asan = is_default_toolchain
}

Or have

declare_args() {
  is_asan = false
}
is_asan = is_default_toolchain && is_asan

If the first one, then I don't understand how that's supposed to work – gn is
run with `is_asan=true` as arg, so the default value isn't used. If the second
one, I don't see how it's different from the current patch. I can't think of a
third way to interpret it.

> 2.:
>   if (current_toolchain == default_toolchain) {
>     declare_args() {
>       is_asan = ...
>     }
>   } else {
>     is_asan = false
> This latter one may not actually work, GN like to try to make you define
> everything in one place, so if you like this approach try a test first.

This appears to work (poc https://codereview.chromium.org/2225073004) and looks
like an improvement to me. (But I'd like to understand what you meant what you
meant in 1 first, I'm sure I just didn't understand it.)

[brettw, 2016-08-09 20:10:28]

On 2016/08/09 01:32:53, Nico (away until Mon Aug 22) wrote:
> Sorry, I forgot about this until now. Thanks for the concrete suggestions!
> 
> On 2016/08/06 04:48:19, brettw (ping on IM after 24h) wrote:
> > I think this approach is undesirable in GN, just as it would be in C++.
> > 
> > Some alternatives.
> > 
> > 1.:
> >   is_default_toolchain = current_toolchain == defualt_toolchain
> >   is_asan = is_default_toolchain && ...
> 
> I don't understand this suggestion. Where does this go? In the declare args?
> Instead of
> 
> declare_args() {
>   is_asan = false
> }
> 
> have
> 
> declare_args() {
>   is_asan = is_default_toolchain
> }
> 
> Or have
> 
> declare_args() {
>   is_asan = false
> }
> is_asan = is_default_toolchain && is_asan
> 
> If the first one, then I don't understand how that's supposed to work – gn is
> run with `is_asan=true` as arg, so the default value isn't used. If the second
> one, I don't see how it's different from the current patch. I can't think of a
> third way to interpret it.

You understood my suggestion but I forgot they were overridden explicitly so it
won't work.

 
> > 2.:
> >   if (current_toolchain == default_toolchain) {
> >     declare_args() {
> >       is_asan = ...
> >     }
> >   } else {
> >     is_asan = false
> > This latter one may not actually work, GN like to try to make you define
> > everything in one place, so if you like this approach try a test first.
> 
> This appears to work (poc https://codereview.chromium.org/2225073004) and
looks
> like an improvement to me. (But I'd like to understand what you meant what you
> meant in 1 first, I'm sure I just didn't understand it.)

If GN is happy with this, I prefer this approach. We should document this
clearly, the average developer doesn't understand GN toolchains at all so this
can easily be a source of confusion.

[Dirk Pranke, 2016-08-09 20:47:50]

GN doesn't complain if you set is_asan=true in args.gn when using your second
approach?

I guess maybe it doesn't because we probably have other args that are only
declared in some toolchains ...

I'm still not super-happy about this solution but I'm not super-happy about any
other solution, either.