Copy ASan runtime to buildroot on Windows as well as Mac

Copy ASan runtime to buildroot on Windows as well as Mac

Otherwise executables with ASan instrumentation fail to start.

R=etienneb@chromium.org,thakis@chromium.org,dpranke@chromium.org
BUG=598761
Committed: https://crrev.com/cb4bc9e68fdefc6ebd44e20b558a465a98cf0a55
Cr-Commit-Position: refs/heads/master@{#409374}

[Nico, 2016-08-02 22:00:49]

lgtm; in gyp we do this at
https://cs.chromium.org/chromium/src/build/win/asan.gyp?rcl=0&l=22 apparently

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

https://codereview.chromium.org/2202983003/diff/1/build/config/sanitizers/BUI...
build/config/sanitizers/BUILD.gn:86: clang_rt_dso_path =
"windows/clang_rt.asan_dynamic-x86_64.dll"
Is this enough? atm asan is enabled for not just target binaries I think, so
from what I can tell in 32-bit builds we still build some nacl binaries in
64-bit with asan. We probably shouldn't do that, but at the moment I think we
do.

[etienneb, 2016-08-02 22:02:12]

I confirm it was a needed step.
I did the copy by hand to be build chrome with asan.
lgtm

[Reid Kleckner, 2016-08-02 22:13:17]

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

https://codereview.chromium.org/2202983003/diff/1/build/config/sanitizers/BUI...
build/config/sanitizers/BUILD.gn:86: clang_rt_dso_path =
"windows/clang_rt.asan_dynamic-x86_64.dll"
On 2016/08/02 22:00:49, Nico wrote:
> Is this enough? atm asan is enabled for not just target binaries I think, so
> from what I can tell in 32-bit builds we still build some nacl binaries in
> 64-bit with asan. We probably shouldn't do that, but at the moment I think we
> do.

I only know it's enough to make base_unittests and sbox_integration_tests start
working. :) Like you say, I think we can tackle the nacl thing separately.

[Reid Kleckner, 2016-08-02 22:13:23]

The CQ bit was checked by rnk@chromium.org

[commit-bot: I haz the power, 2016-08-02 22:14:32]

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-08-02 23:14:31]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-08-02 23:17:03]

Description was changed from

==========
Copy ASan runtime to buildroot on Windows as well as Mac

Otherwise executables with ASan instrumentation fail to start.

R=etienneb@chromium.org,thakis@chromium.org,dpranke@chromium.org
BUG=598761
==========

to

==========
Copy ASan runtime to buildroot on Windows as well as Mac

Otherwise executables with ASan instrumentation fail to start.

R=etienneb@chromium.org,thakis@chromium.org,dpranke@chromium.org
BUG=598761

Committed: https://crrev.com/cb4bc9e68fdefc6ebd44e20b558a465a98cf0a55
Cr-Commit-Position: refs/heads/master@{#409374}
==========

[commit-bot: I haz the power, 2016-08-02 23:17:04]

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

[Dirk Pranke, 2016-08-03 00:06:10]

I'm not real sure why we landed this instead of a fixed version of the CL I had
posted (based on etienneb's change) ...

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

https://codereview.chromium.org/2202983003/diff/1/build/config/sanitizers/BUI...
build/config/sanitizers/BUILD.gn:79: if ((is_mac || is_win) && using_sanitizer)
{
This isn't quite right, because now bundle_data("asan_runtime_bundle_data") will
be defined as a target as well. That one should be mac-only.

However, I think bundle_data() is a no-op on windows so it might be relatively
harmless.

[Nico, 2016-08-03 00:45:00]

you mean https://codereview.chromium.org/2198333002/ ? this looked like a
different bit than that cl to me (this here copies the runtime around, the
other doesn't)

On Tue, Aug 2, 2016 at 8:06 PM, <dpranke@chromium.org> wrote:

> I'm not real sure why we landed this instead of a fixed version of the CL
> I had
> posted (based on etienneb's change) ...
>
>
>
>
https://codereview.chromium.org/2202983003/diff/1/build/config/sanitizers/BUI...
> File build/config/sanitizers/BUILD.gn (right):
>
>
>
https://codereview.chromium.org/2202983003/diff/1/build/config/sanitizers/BUI...
> build/config/sanitizers/BUILD.gn:79: if ((is_mac || is_win) &&
> using_sanitizer) {
> This isn't quite right, because now
> bundle_data("asan_runtime_bundle_data") will be defined as a target as
> well. That one should be mac-only.
>
> However, I think bundle_data() is a no-op on windows so it might be
> relatively harmless.
>
> https://codereview.chromium.org/2202983003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Dirk Pranke, 2016-08-03 00:50:22]

On 2016/08/03 00:45:00, Nico wrote:
> you mean https://codereview.chromium.org/2198333002/ ? this looked like a
> different bit than that cl to me (this here copies the runtime around, the
> other doesn't)

Argh. Apparently I didn't upload the latest version of the patchset that had
this
fix.

So, yeah, that's a pretty good reason :(.

I've uploaded it now, for posterity
(https://codereview.chromium.org/2198333002/#ps40001)