Fix GN generation for WinASAN build

Fix GN generation for WinASAN build

This is a step toward a working Asan build with GN.
There are still a few missing pieces to land.

This patch is fixing an assert triggering due to courgette using an
harcoded toolchains.

----------------------
 ERROR at //build/config/sanitizers/sanitizers.gni:12:1: Assertion failed.
  assert(!using_sanitizer || is_clang,
  ^-----
  Sanitizers (is_*san) require setting is_clang = true in 'gn args'
----------------------

This patch is also fixing the compiler-rt runtime paths for a x64 bit
build. The port of WinASAN-64 is almost done and should part of the next
clang roll-deps.

R=thakis@chromium.org, dpranke@chromium.org
TBR=brettw@chromium.org
BUG=598761
Committed: https://crrev.com/364deb713883f28d2647c5ff160c1a6f7b7ef342
Cr-Commit-Position: refs/heads/master@{#408857}

[etienneb, 2016-07-28 15:04:00]

PTAL.

Please, I need a GN guru to look over my shoulder.
I'm not sure this is the clean way to fix the
toolchain issue that courgette introduced.


FYI, this is my current GN configuration:
------------------------------------------
target_cpu = "x86"

is_component_build = true
is_debug = false
is_asan = true
enable_nacl = false
is_clang = true
clang_base_path = "d:\src\llvm\ninja"
clang_use_chrome_plugins = false
clang_version = "4.0.0"
------------------------------------------

[chrisha, 2016-07-28 15:29:57]

https://codereview.chromium.org/2192833002/diff/1/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2192833002/diff/1/BUILD.gn#newcode117
BUILD.gn:117: deps += [ "//courgette(//build/toolchain/win:x64)" ]
I'm not 100% up on how to do things in GN. Does it make sense to have
conditional logic related to courgette in THIS build file? Or should Courgette's
GN file have the appropriate logic gated on 'is_clang'?

https://codereview.chromium.org/2192833002/diff/1/build/config/sanitizers/BUI...
File build/config/sanitizers/BUILD.gn (right):

https://codereview.chromium.org/2192833002/diff/1/build/config/sanitizers/BUI...
build/config/sanitizers/BUILD.gn:238: # TODO(rnk): DLLs in the non-component
build should link against
TODO(etienneb), given that you're now driving this?

[etienneb, 2016-07-28 15:35:39]

https://codereview.chromium.org/2192833002/diff/1/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2192833002/diff/1/BUILD.gn#newcode117
BUILD.gn:117: deps += [ "//courgette(//build/toolchain/win:x64)" ]
On 2016/07/28 15:29:57, chrisha (slow) wrote:
> I'm not 100% up on how to do things in GN. Does it make sense to have
> conditional logic related to courgette in THIS build file? Or should
Courgette's
> GN file have the appropriate logic gated on 'is_clang'?

I was expecting a cleaner way to do this:

  deps += [ "//courgette($host_compiler)" ]

But, I'm not convince what is the clean way. I hope brettw@ has some inside for
me.

But, I agree... is_clang here is working, but doesn't sound clean.

https://codereview.chromium.org/2192833002/diff/1/courgette/BUILD.gn
File courgette/BUILD.gn (right):

https://codereview.chromium.org/2192833002/diff/1/courgette/BUILD.gn#newcode134
courgette/BUILD.gn:134: "$root_out_dir/courgette64.exe",
Just in case you missed it, with this "is_clang" here,
we are copying the source over the destination.

I feel this is not clean, but I didn't have too figure out a way
to remove the dependency.

[etienneb, 2016-07-28 16:28:00]

rebased.

Someone modified courgette code.

https://codereview.chromium.org/2192833002/diff/1/build/config/sanitizers/BUI...
File build/config/sanitizers/BUILD.gn (right):

https://codereview.chromium.org/2192833002/diff/1/build/config/sanitizers/BUI...
build/config/sanitizers/BUILD.gn:238: # TODO(rnk): DLLs in the non-component
build should link against
On 2016/07/28 15:29:57, chrisha (slow) wrote:
> TODO(etienneb), given that you're now driving this?

Done.

[Nico, 2016-07-29 17:00:48]

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

[Nico, 2016-07-29 17:00:49]

base/* and build/* lgtm

Someone else (brettw or dpranke?) should look at //BULID.gn and the courgette
change -- that looks a bit strange to me, but I don't know if there's a better
way to say "build this with a toolchain that's 64-bit but otherwise identical to
the default toolchain".

[etienneb, 2016-07-29 18:28:28]

Description was changed from

==========
Fix GN generation for WinASAN build

This is a step toward a working Asan build with GN.
There are still a few missing pieces to land.

This patch is fixing an assert triggering due to courgette using an
harcoded toolchains.

----------------------
 ERROR at //build/config/sanitizers/sanitizers.gni:12:1: Assertion failed.
  assert(!using_sanitizer || is_clang,
  ^-----
  Sanitizers (is_*san) require setting is_clang = true in 'gn args'
----------------------

This patch is also fixing the compiler-rt runtime paths for a x64 bit
build. The port of WinASAN-64 is almost done and should part of the next
clang roll-deps.

R=brettw@chromium.org, chrisha@chromium.org, rnk@chromium.org

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

to

==========
Fix GN generation for WinASAN build

This is a step toward a working Asan build with GN.
There are still a few missing pieces to land.

This patch is fixing an assert triggering due to courgette using an
harcoded toolchains.

----------------------
 ERROR at //build/config/sanitizers/sanitizers.gni:12:1: Assertion failed.
  assert(!using_sanitizer || is_clang,
  ^-----
  Sanitizers (is_*san) require setting is_clang = true in 'gn args'
----------------------

This patch is also fixing the compiler-rt runtime paths for a x64 bit
build. The port of WinASAN-64 is almost done and should part of the next
clang roll-deps.

R=brettw@chromium.org, chrisha@chromium.org, rnk@chromium.org

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

[etienneb, 2016-07-29 18:28:28]

etienneb@chromium.org changed reviewers:
+ mcgrathr@chromium.org

[etienneb, 2016-07-29 18:29:19]

thx nico.
Dirk looking to this too.

[etienneb, 2016-07-29 19:12:37]

This config built successfully:
---------------------------------
target_cpu = "x86"
is_component_build = true
is_debug = false
is_asan = true
enable_nacl = false
is_clang = true


This one too:
---------------------------------
is_component_build = true
is_debug = false
enable_nacl = false
is_clang = true
is_asan = true
clang_base_path = "d:\src\llvm\ninja64"
clang_use_chrome_plugins = false
clang_version = "4.0.0"


Unfortunately, there is still something to fix here:
---------------------------------
target_cpu = "x86"
is_debug = false
is_asan = true
enable_nacl = false
is_clang = true

[Reid Kleckner, 2016-07-29 19:28:27]

On 2016/07/29 19:12:37, etienneb wrote:
> Unfortunately, there is still something to fix here:
> ---------------------------------
> target_cpu = "x86"
> is_debug = false
> is_asan = true
> enable_nacl = false
> is_clang = true

Right, the TODO(rnk) in the code about runtime libraries is what this is about.

[Dirk Pranke, 2016-07-30 00:57:38]

I can confirm that this patch plus my toolchain_args() patches produced a
compile that completed and a content_shell that seemed to run, once I took the
additional step noted below to copy the runtime over.

I did not have enough time today to confirm that my toolchain_args() patches
were needed.

So, LGTM if we want to land this now and continue fixing things, or you can try
to address the comments, too.

https://codereview.chromium.org/2192833002/diff/80001/build/config/sanitizers...
File build/config/sanitizers/BUILD.gn (right):

https://codereview.chromium.org/2192833002/diff/80001/build/config/sanitizers...
build/config/sanitizers/BUILD.gn:83: copy("copy_asan_runtime") {
I think you need to make this rule work as well on win, so that the right .dll
is copied into root_build_dir ; I had to manually copy them over before any of
the host targets would run as part of the build, and I assume I needed the
target dll as well for the targets binaries to run.

You'd need to fix the condition on line 70-73, above, as well.

https://codereview.chromium.org/2192833002/diff/80001/build/config/sanitizers...
build/config/sanitizers/BUILD.gn:266: libs = [ "clang_rt.asan-i386.lib" ]
why not just change this to clang_rt.asan_dll_thunk-i386.lib, if this is true?

[Dirk Pranke, 2016-07-30 01:00:18]

The two toolchain_args() CLs I was referring to are:

https://codereview.chromium.org/2186753006/
https://codereview.chromium.org/2189153002/

However, as I said, I'm not actually sure that they're needed (the various
targets might actually work w/o them,
and I think they compiled okay for etienneb@). In addition, I'm not sure that
they're the right way to solve the
problem. It might actually be better to only enable the sanitizers for the
default toolchain and not have
to worry about explicitly turning them off in every non-default toolchain.

[Dirk Pranke, 2016-07-30 01:04:52]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2016-07-30 01:05:20]

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

[commit-bot: I haz the power, 2016-07-30 01:11:47]

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

[commit-bot: I haz the power, 2016-07-30 01:11:48]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Dirk Pranke, 2016-07-30 01:16:31]

Description was changed from

==========
Fix GN generation for WinASAN build

This is a step toward a working Asan build with GN.
There are still a few missing pieces to land.

This patch is fixing an assert triggering due to courgette using an
harcoded toolchains.

----------------------
 ERROR at //build/config/sanitizers/sanitizers.gni:12:1: Assertion failed.
  assert(!using_sanitizer || is_clang,
  ^-----
  Sanitizers (is_*san) require setting is_clang = true in 'gn args'
----------------------

This patch is also fixing the compiler-rt runtime paths for a x64 bit
build. The port of WinASAN-64 is almost done and should part of the next
clang roll-deps.

R=brettw@chromium.org, chrisha@chromium.org, rnk@chromium.org

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

to

==========
Fix GN generation for WinASAN build

This is a step toward a working Asan build with GN.
There are still a few missing pieces to land.

This patch is fixing an assert triggering due to courgette using an
harcoded toolchains.

----------------------
 ERROR at //build/config/sanitizers/sanitizers.gni:12:1: Assertion failed.
  assert(!using_sanitizer || is_clang,
  ^-----
  Sanitizers (is_*san) require setting is_clang = true in 'gn args'
----------------------

This patch is also fixing the compiler-rt runtime paths for a x64 bit
build. The port of WinASAN-64 is almost done and should part of the next
clang roll-deps.

R=thakis@chromium.org, dpranke@chromium.org
TBR=brettw@chromium.org
BUG=598761
==========

[Dirk Pranke, 2016-07-30 01:17:14]

TBR'ing brettw@ so I can land this. The courgette change is safe (and right),
and the BUILDCONFIG changes are just fixing typos in comments.

I will address my comments about copying the libs in follow-up CLs.

[Dirk Pranke, 2016-07-30 01:18:37]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2016-07-30 01:18:50]

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

[commit-bot: I haz the power, 2016-07-30 02:12:18]

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

[commit-bot: I haz the power, 2016-07-30 02:12:19]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Nico, 2016-07-30 03:15:05]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2016-07-30 03:15:13]

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

[commit-bot: I haz the power, 2016-07-30 03:44:18]

Description was changed from

==========
Fix GN generation for WinASAN build

This is a step toward a working Asan build with GN.
There are still a few missing pieces to land.

This patch is fixing an assert triggering due to courgette using an
harcoded toolchains.

----------------------
 ERROR at //build/config/sanitizers/sanitizers.gni:12:1: Assertion failed.
  assert(!using_sanitizer || is_clang,
  ^-----
  Sanitizers (is_*san) require setting is_clang = true in 'gn args'
----------------------

This patch is also fixing the compiler-rt runtime paths for a x64 bit
build. The port of WinASAN-64 is almost done and should part of the next
clang roll-deps.

R=thakis@chromium.org, dpranke@chromium.org
TBR=brettw@chromium.org
BUG=598761
==========

to

==========
Fix GN generation for WinASAN build

This is a step toward a working Asan build with GN.
There are still a few missing pieces to land.

This patch is fixing an assert triggering due to courgette using an
harcoded toolchains.

----------------------
 ERROR at //build/config/sanitizers/sanitizers.gni:12:1: Assertion failed.
  assert(!using_sanitizer || is_clang,
  ^-----
  Sanitizers (is_*san) require setting is_clang = true in 'gn args'
----------------------

This patch is also fixing the compiler-rt runtime paths for a x64 bit
build. The port of WinASAN-64 is almost done and should part of the next
clang roll-deps.

R=thakis@chromium.org, dpranke@chromium.org
TBR=brettw@chromium.org
BUG=598761
==========

[commit-bot: I haz the power, 2016-07-30 03:44:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-30 03:46:51]

Description was changed from

==========
Fix GN generation for WinASAN build

This is a step toward a working Asan build with GN.
There are still a few missing pieces to land.

This patch is fixing an assert triggering due to courgette using an
harcoded toolchains.

----------------------
 ERROR at //build/config/sanitizers/sanitizers.gni:12:1: Assertion failed.
  assert(!using_sanitizer || is_clang,
  ^-----
  Sanitizers (is_*san) require setting is_clang = true in 'gn args'
----------------------

This patch is also fixing the compiler-rt runtime paths for a x64 bit
build. The port of WinASAN-64 is almost done and should part of the next
clang roll-deps.

R=thakis@chromium.org, dpranke@chromium.org
TBR=brettw@chromium.org
BUG=598761
==========

to

==========
Fix GN generation for WinASAN build

This is a step toward a working Asan build with GN.
There are still a few missing pieces to land.

This patch is fixing an assert triggering due to courgette using an
harcoded toolchains.

----------------------
 ERROR at //build/config/sanitizers/sanitizers.gni:12:1: Assertion failed.
  assert(!using_sanitizer || is_clang,
  ^-----
  Sanitizers (is_*san) require setting is_clang = true in 'gn args'
----------------------

This patch is also fixing the compiler-rt runtime paths for a x64 bit
build. The port of WinASAN-64 is almost done and should part of the next
clang roll-deps.

R=thakis@chromium.org, dpranke@chromium.org
TBR=brettw@chromium.org
BUG=598761

Committed: https://crrev.com/364deb713883f28d2647c5ff160c1a6f7b7ef342
Cr-Commit-Position: refs/heads/master@{#408857}
==========

[commit-bot: I haz the power, 2016-07-30 03:46:52]

Patchset 5 (id:??) landed as
https://crrev.com/364deb713883f28d2647c5ff160c1a6f7b7ef342
Cr-Commit-Position: refs/heads/master@{#408857}