Start landing build support for Clang-style ASan on Windows

Start landing build support for Clang-style ASan on Windows

If this breaks your SyzyASan build, make sure you're passing syzyasan=1
in GYP_DEFINES instead of asan=1. I have updated all affected buildbots
I know about.

BUG=82385
R=thakis@chromium.org, timurrrr@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252707

[hans, 2014-02-12 17:44:17]

Hi Timur, with this I can build base_unittests just by setting
GYP_DEFINES="clang=1 asan=1" and no extra local hacks :) Please take a look.

There are two things that still concern me with this patch:

1. We need make_clang_dir to be an absolute path, and the way I do that feels
hacky.

2. We were previously building Clang in 64-bit mode, but that doesn't build the
asan runtime, so I'm switching it to 32-bit. However, even if we re-run CMake, I
suspect it might cache the previous compiler version :/

[Nico, 2014-02-12 18:34:34]

Why do you need an absolute path?

Does this mean clang is 32bit now too? If so, that kinda defeats much of
the purpose.

Can these two things be fixed before turning this on?


On Wed, Feb 12, 2014 at 9:44 AM, <hans@chromium.org> wrote:

> Reviewers: Timur Iskhodzhanov, Nico (away Feb 11 - Feb 17),
>
> Message:
> Hi Timur, with this I can build base_unittests just by setting
> GYP_DEFINES="clang=1 asan=1" and no extra local hacks :) Please take a
> look.
>
> There are two things that still concern me with this patch:
>
> 1. We need make_clang_dir to be an absolute path, and the way I do that
> feels
> hacky.
>
> 2. We were previously building Clang in 64-bit mode, but that doesn't
> build the
> asan runtime, so I'm switching it to 32-bit. However, even if we re-run
> CMake, I
> suspect it might cache the previous compiler version :/
>
> Description:
> Start landing build support for Clang-style ASan on Windows
>
> BUG=82385
>
> Please review this at https://codereview.chromium.org/137823014/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+35, -6 lines):
>   M build/common.gypi
>   M tools/clang/scripts/update.py
>
>
> Index: build/common.gypi
> diff --git a/build/common.gypi b/build/common.gypi
> index ddd6546f379437be9af2d050f59f8c5e0091df40..
> 01ef244f8a967f3d64c089ffeb0c21135be87f42 100644
> --- a/build/common.gypi
> +++ b/build/common.gypi
> @@ -1080,7 +1080,7 @@
>
>      # Clang stuff.
>      'clang%': '<(clang)',
> -    'make_clang_dir%': 'third_party/llvm-build/Release+Asserts',
> +    'make_clang_dir%': '<!(python -c "import sys, os; print
> os.path.abspath(sys.argv[1])" <(DEPTH)/third_party/llvm-
> build/Release+Asserts)',
>
>      # These two variables can be set in GYP_DEFINES while running
>      # |gclient runhooks| to let clang run a plugin in every compilation.
> @@ -1579,6 +1579,9 @@
>              # Turn on multiple dll by default on Windows when in
> static_library.
>              'chrome_multiple_dll%': 1,
>            }],
> +          ['clang==1 and asan==1', {
> +            'win_use_allocator_shim%': 0,
> +          }],
>            ['component=="shared_library" and "<(GENERATOR)"=="ninja"', {
>              # Only enabled by default for ninja because it's buggy in VS.
>              # Not enabled for component=static_library because some
> targets
> @@ -2321,8 +2324,8 @@
>            'ENABLE_EGLIMAGE=1',
>          ],
>        }],
> -      ['asan==1 and OS=="win"', {
> -        # Since asan on windows uses Syzygy, we need /PROFILE turned on to
> +      ['asan==1 and OS=="win" and clang!=1', {
> +        # For SyzyAsan, we need /PROFILE turned on to
>          # produce appropriate pdbs.
>          'msvs_settings': {
>            'VCLinkerTool': {
> @@ -4727,6 +4730,31 @@
>                  }],
>                ],
>              }],
> +            ['clang==1 and asan==1', {
> +              # Building with Clang on Windows is a work in progress and
> very
> +              # experimental. See crbug.com/82385.
> +              'VCCLCompilerTool': {
> +                'AdditionalOptions': [
> +                  '-fsanitize=address',
> +                ],
> +              },
> +              'target_conditions': [
> +                ['_type=="executable"', {
> +                  'VCLinkerTool': {
> +                    'AdditionalOptions': [
> +                       '<(make_clang_dir)/lib/clang/
> 3.5/lib/windows/clang_rt.asan-i386.lib',
> +                    ],
> +                  },
> +                }],
> +                ['_type=="shared_library" or _type=="loadable_module"', {
> +                  'VCLinkerTool': {
> +                    'AdditionalOptions': [
> +                       '<(make_clang_dir)/lib/clang/
> 3.5/lib/windows/clang_rt.asan_dll_thunk-i386.lib',
> +                    ],
> +                  },
> +                }],
> +              ],
> +            }],
>            ],
>          },
>        },
> Index: tools/clang/scripts/update.py
> diff --git a/tools/clang/scripts/update.py b/tools/clang/scripts/update.py
> index 7d0585966d17c3a254b1521604de6cd5b3c83539..
> 8a7b7c7f041498782d94b63b6514c983a9b87177 100755
> --- a/tools/clang/scripts/update.py
> +++ b/tools/clang/scripts/update.py
> @@ -15,7 +15,7 @@ import sys
>  # https://code.google.com/p/chromium/wiki/UpdatingClang
>  # Reverting problematic clang rolls is safe, though.
>  # Note: this revision is only used for Windows. Other platforms use
> update.sh.
> -LLVM_WINDOWS_REVISION = '201116'
> +LLVM_WINDOWS_REVISION = '201117'
>
>  # Path constants. (All of these should be absolute paths.)
>  THIS_DIR = os.path.abspath(os.path.dirname(__file__))
> @@ -131,11 +131,12 @@ def UpdateClang():
>      # If CMake is not on the path, try looking in a standard location.
>      os.environ['PATH'] += os.pathsep + 'C:\\Program Files (x86)\\CMake
> 2.8\\bin'
>
> -  RunCommand(GetVSVersion().SetupScript('x64') +
> +  # TODO(hans): Do an amd64 build once that supports building ASan
> runtime.
> +  RunCommand(GetVSVersion().SetupScript('x86') +
>               ['&&', 'cmake', '-GNinja', '-DCMAKE_BUILD_TYPE=Release',
>                '-DLLVM_ENABLE_ASSERTIONS=ON', LLVM_DIR])
>
> -  RunCommand(GetVSVersion().SetupScript('x64') + ['&&', 'ninja', 'all'])
> +  RunCommand(GetVSVersion().SetupScript('x86') + ['&&', 'ninja', 'all'])
>
>    WriteStampFile(LLVM_WINDOWS_REVISION)
>    print 'Clang update was successful.'
>
>
>

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

[hans, 2014-02-12 19:09:44]

On 2014/02/12 18:34:34, Nico (away Feb 11 - Feb 17) wrote:
> Why do you need an absolute path?

I need to be able to find the asan run-time libraries to link against.

Currently, we set make_clang_dir to be relative to Chromium src. So I could to
"<(DEPTH)/<(make_clang_dir)/lib/clang/blah" to find the libs.

But make_clang_dir can be overridden from the command line, which is very
useful, possibly to an absolute path, and then the <(DEPTH) trick fails. We need
to enforce it to always be either absolute or relative. This was the best way I
could think of.

> Does this mean clang is 32bit now too? If so, that kinda defeats much of
> the purpose.

Yes. It would just be temporary though, until the 64-bit clang build provides
32-bit asan runtime libraries.

> Can these two things be fixed before turning this on?

I don't think we need to block this on the 64-bitness of clang, but I'm
interested to hear if you have any ideas about the clang path issue.

[Timur Iskhodzhanov, 2014-02-13 09:18:30]

LGTM, but you may want Nico to review too.

> Does this mean clang is 32bit now too?

I think currently Clang and ASan on Windows are mostly tested with x86 targets -
partly because Chrome itself is not a first-class arch on x64.

We do plan to work on x64 targets as well, but we're not there yet. The x64
support can be easily added to ASan when needed.

> If so, that kinda defeats much of the purpose.

I think most of the anticipated advantage of a x64 Clang toolchain is the
ability to *link* large binaries rather than compile them?

https://codereview.chromium.org/137823014/diff/110001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/137823014/diff/110001/build/common.gypi#newco...
build/common.gypi:1083: 'make_clang_dir%': '<!(python -c "import sys, os; print
os.path.abspath(sys.argv[1])" <(DEPTH)/third_party/llvm-build/Release+Asserts)',
Maybe want to add a comment explaining why absolute paths are needed.

[hans, 2014-02-13 17:21:42]

I didn't like the absolute path thing. I've uploaded a new patch where I just
assume make_clang_path is relative.

If the user overrides it with an absolute path from the command line it will
break, but maybe we don't have to worry about that now?

Nico, what do you think?

As for the 64-bit issue, I think we should try to make it possible to build a
64-bit clang with 32-bit asan runtime, but I don't want to block on it here.

[hans, 2014-02-14 00:57:07]

On 2014/02/13 17:21:42, hans wrote:
> As for the 64-bit issue, I think we should try to make it possible to build a
> 64-bit clang with 32-bit asan runtime, but I don't want to block on it here.

Actually, let's just do the 32-bit build of compiler-rt on the side for now.

New patch uploaded, let me know what you think.

[Nico, 2014-02-14 01:27:17]

https://codereview.chromium.org/137823014/diff/220001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/137823014/diff/220001/build/common.gypi#newco...
build/common.gypi:2327: ['asan==1 and OS=="win" and clang!=1', {
can we replace "asan==1" on windows with "syszyasan" or similar and make "asan"
mean asan like on other platforms? having to check "asan==1 and clang==1", but
only on windows, is pretty lame :-/

Or is there a lot of stuff that's shared in gyp between syzyasan and asan?

(I asked the syzyasan folks to call there stuff syzyasan and not just asan and
they did most of that in https://codereview.appspot.com/13352050/ , but
apparently not the gyp define)

[hans, 2014-02-14 01:43:52]

On 2014/02/14 01:27:17, Nico (away Feb 11 - Feb 17) wrote:
> https://codereview.chromium.org/137823014/diff/220001/build/common.gypi
> File build/common.gypi (right):
> 
>
https://codereview.chromium.org/137823014/diff/220001/build/common.gypi#newco...
> build/common.gypi:2327: ['asan==1 and OS=="win" and clang!=1', {
> can we replace "asan==1" on windows with "syszyasan" or similar and make
"asan"
> mean asan like on other platforms? having to check "asan==1 and clang==1", but
> only on windows, is pretty lame :-/

I agree, it's very lame.

> Or is there a lot of stuff that's shared in gyp between syzyasan and asan?

Nope.

> (I asked the syzyasan folks to call there stuff syzyasan and not just asan and
> they did most of that in https://codereview.appspot.com/13352050/ , but
> apparently not the gyp define)

The only problem is external dependencies. I don't know how the syzyasan folks
run their tests, what buildbots are passing these flags, etc.

I think we should try to do this flag rename in a separate patch and rope in the
relevant people.

[Timur Iskhodzhanov, 2014-02-14 05:30:33]

Totally agreed with (asan && !clang) -> syzyasan renaming and agree it
should be done separately

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

[Nico, 2014-02-14 05:34:46]

Can we do it before landing real asan stuff to gyp then?


On Thu, Feb 13, 2014 at 9:30 PM, Timur Iskhodzhanov
<timurrrr@chromium.org>wrote:

> Totally agreed with (asan && !clang) -> syzyasan renaming and agree it
> should be done separately
>

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

[Nico, 2014-02-14 05:34:53]

s/landing/adding/


On Thu, Feb 13, 2014 at 9:34 PM, Nico Weber <thakis@chromium.org> wrote:

> Can we do it before landing real asan stuff to gyp then?
>
>
> On Thu, Feb 13, 2014 at 9:30 PM, Timur Iskhodzhanov <timurrrr@chromium.org
> > wrote:
>
>> Totally agreed with (asan && !clang) -> syzyasan renaming and agree it
>> should be done separately
>>
>
>

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

[Timur Iskhodzhanov, 2014-02-14 08:32:48]

> Can we do it before adding real asan stuff to gyp then?

Why add an extra dependency and slow down the effort?
I'm perfectly fine if we add the clang-asan first with a TODO and do
the rename later.

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

[Nico, 2014-02-14 18:51:48]

On Fri, Feb 14, 2014 at 12:32 AM, Timur Iskhodzhanov
<timurrrr@chromium.org>wrote:

> > Can we do it before adding real asan stuff to gyp then?
>
> Why add an extra dependency and slow down the effort?
> I'm perfectly fine if we add the clang-asan first with a TODO and do
> the rename later.
>

Because there's a true dependency here: Right now "asan" meaning something
else on windows is slightly confusing. With this CL in, it's actively
confusing.

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

[hans, 2014-02-14 18:56:48]

On 2014/02/14 18:51:48, Nico (away Feb 11 - Feb 17) wrote:
> On Fri, Feb 14, 2014 at 12:32 AM, Timur Iskhodzhanov
> <timurrrr@chromium.org>wrote:
> 
> > > Can we do it before adding real asan stuff to gyp then?
> >
> > Why add an extra dependency and slow down the effort?
> > I'm perfectly fine if we add the clang-asan first with a TODO and do
> > the rename later.
> >
> 
> Because there's a true dependency here: Right now "asan" meaning something
> else on windows is slightly confusing. With this CL in, it's actively
> confusing.

OK, I'll spin up a separate patch.

[hans, 2014-02-19 02:22:56]

On 2014/02/14 18:56:48, hans wrote:
> On 2014/02/14 18:51:48, Nico (away Feb 11 - Feb 17) wrote:
> > On Fri, Feb 14, 2014 at 12:32 AM, Timur Iskhodzhanov
> > <timurrrr@chromium.org>wrote:
> > 
> > > > Can we do it before adding real asan stuff to gyp then?
> > >
> > > Why add an extra dependency and slow down the effort?
> > > I'm perfectly fine if we add the clang-asan first with a TODO and do
> > > the rename later.
> > >
> > 
> > Because there's a true dependency here: Right now "asan" meaning something
> > else on windows is slightly confusing. With this CL in, it's actively
> > confusing.
> 
> OK, I'll spin up a separate patch.

Circling back on this, everything is now landed to make SyzyASan use syzyasan=1
instead of asan=1, we're just waiting for an internal buildbot master restart.

[hans, 2014-02-21 22:18:05]

On 2014/02/19 02:22:56, hans wrote:
> Circling back on this, everything is now landed to make SyzyASan use
syzyasan=1
> instead of asan=1, we're just waiting for an internal buildbot master restart.

All the buildbot configs I know about are now updated. New patch uploaded,
please take a look.

[Nico, 2014-02-21 22:22:39]

lgtm

You can do the variable scope suggestion in a follow-up if you want.

https://codereview.chromium.org/137823014/diff/370001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/137823014/diff/370001/build/common.gypi#newco...
build/common.gypi:1980: ['OS=="win" and (clang==1 or asan==1)', {
If you put the "if asan = 1 then clang = 1" block in a deeper variables scope
and then copy it out, you won't need this change I think (?)

https://codereview.chromium.org/137823014/diff/370001/build/common.gypi#newco...
build/common.gypi:4824: # See crbug.com/82385.
Maybe have a separate bug for asan/win and make it blocked on 82385 instead.

https://codereview.chromium.org/137823014/diff/370001/tools/clang/scripts/upd...
File tools/clang/scripts/update.py (right):

https://codereview.chromium.org/137823014/diff/370001/tools/clang/scripts/upd...
tools/clang/scripts/update.py:142: # TODO(hans): This is pretty hacky.
Is this an actionable todo? If so, say what's here to be done. If it's hacky and
likely to stay that way, remove the TODO.

[hans, 2014-02-21 22:33:00]

https://codereview.chromium.org/137823014/diff/370001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/137823014/diff/370001/build/common.gypi#newco...
build/common.gypi:1980: ['OS=="win" and (clang==1 or asan==1)', {
On 2014/02/21 22:22:40, Nico (vacation Feb 24 - 29) wrote:
> If you put the "if asan = 1 then clang = 1" block in a deeper variables scope
> and then copy it out, you won't need this change I think (?)

I'll see if I can do this in a follow-up. This is an aspect of gyp that always
scares me :)

https://codereview.chromium.org/137823014/diff/370001/build/common.gypi#newco...
build/common.gypi:4824: # See crbug.com/82385.
On 2014/02/21 22:22:40, Nico (vacation Feb 24 - 29) wrote:
> Maybe have a separate bug for asan/win and make it blocked on 82385 instead.

Done.

https://codereview.chromium.org/137823014/diff/370001/tools/clang/scripts/upd...
File tools/clang/scripts/update.py (right):

https://codereview.chromium.org/137823014/diff/370001/tools/clang/scripts/upd...
tools/clang/scripts/update.py:142: # TODO(hans): This is pretty hacky.
On 2014/02/21 22:22:40, Nico (vacation Feb 24 - 29) wrote:
> Is this an actionable todo? If so, say what's here to be done. If it's hacky
and
> likely to stay that way, remove the TODO.

I've reformulated it. Basically I think we want to fix the build upstream to
produce both 32- and 64-bit versions of the run-time.

[hans, 2014-02-21 22:33:14]

The CQ bit was checked by hans@chromium.org

[commit-bot: I haz the power, 2014-02-21 22:35:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hans@chromium.org/137823014/430001

[commit-bot: I haz the power, 2014-02-21 22:35:40]

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

[commit-bot: I haz the power, 2014-02-21 22:35:40]

Failed to apply patch for tools/clang/scripts/update.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file tools/clang/scripts/update.py
  Hunk #2 FAILED at 16.
  1 out of 5 hunks FAILED -- saving rejects to file
tools/clang/scripts/update.py.rej

Patch:       tools/clang/scripts/update.py
Index: tools/clang/scripts/update.py
diff --git a/tools/clang/scripts/update.py b/tools/clang/scripts/update.py
index
bce2f224f1342a97cc267f237a27dad3d086023f..b64c8271b2b5fcce8a55dce53952141821377c2c
100755
--- a/tools/clang/scripts/update.py
+++ b/tools/clang/scripts/update.py
@@ -8,6 +8,7 @@ update.sh. This script should replace update.sh on all platforms
eventually."""
 
 import os
 import re
+import shutil
 import subprocess
 import sys
 
@@ -15,7 +16,7 @@ import sys
 # https://code.google.com/p/chromium/wiki/UpdatingClang
 # Reverting problematic clang rolls is safe, though.
 # Note: this revision is only used for Windows. Other platforms use update.sh.
-LLVM_WINDOWS_REVISION = '201742'
+LLVM_WINDOWS_REVISION = '201744'
 
 # Path constants. (All of these should be absolute paths.)
 THIS_DIR = os.path.abspath(os.path.dirname(__file__))
@@ -23,6 +24,7 @@ CHROMIUM_DIR = os.path.abspath(os.path.join(THIS_DIR, '..',
'..', '..'))
 LLVM_DIR = os.path.join(CHROMIUM_DIR, 'third_party', 'llvm')
 LLVM_BUILD_DIR = os.path.join(CHROMIUM_DIR, 'third_party', 'llvm-build',
                               'Release+Asserts')
+COMPILER_RT_BUILD_DIR = os.path.join(LLVM_BUILD_DIR, '32bit-compiler-rt')
 CLANG_DIR = os.path.join(LLVM_DIR, 'tools', 'clang')
 COMPILER_RT_DIR = os.path.join(LLVM_DIR, 'projects', 'compiler-rt')
 STAMP_FILE = os.path.join(LLVM_BUILD_DIR, 'cr_build_revision')
@@ -134,9 +136,29 @@ def UpdateClang():
   RunCommand(GetVSVersion().SetupScript('x64') +
              ['&&', 'cmake', '-GNinja', '-DCMAKE_BUILD_TYPE=Release',
               '-DLLVM_ENABLE_ASSERTIONS=ON', LLVM_DIR])
-
   RunCommand(GetVSVersion().SetupScript('x64') + ['&&', 'ninja', 'all'])
 
+  # Do an x86 build of compiler-rt to get the 32-bit ASan run-time.
+  # TODO(hans): Remove once the regular build above produces this.
+  if not os.path.exists(COMPILER_RT_BUILD_DIR):
+    os.makedirs(COMPILER_RT_BUILD_DIR)
+  os.chdir(COMPILER_RT_BUILD_DIR)
+  RunCommand(GetVSVersion().SetupScript('x86') +
+             ['&&', 'cmake', '-GNinja', '-DCMAKE_BUILD_TYPE=Release',
+              '-DLLVM_ENABLE_ASSERTIONS=ON', LLVM_DIR])
+  RunCommand(GetVSVersion().SetupScript('x86') + ['&&', 'ninja',
'compiler-rt'])
+  asan_rt_lib_src_dir = os.path.join(COMPILER_RT_BUILD_DIR, 'lib', 'clang',
+                                     '3.5', 'lib', 'windows')
+  asan_rt_lib_dst_dir = os.path.join(LLVM_BUILD_DIR, 'lib', 'clang',
+                                     '3.5', 'lib', 'windows')
+  if not os.path.exists(asan_rt_lib_dst_dir):
+    os.makedirs(asan_rt_lib_dst_dir)
+  for root, _, files in os.walk(asan_rt_lib_src_dir):
+    for f in files:
+      if re.match(r'^.*-i386\.lib$', f):
+        shutil.copy(os.path.join(root, f), asan_rt_lib_dst_dir)
+        print "Copying %s to %s" % (f, asan_rt_lib_dst_dir)
+
   WriteStampFile(LLVM_WINDOWS_REVISION)
   print 'Clang update was successful.'
   return 0
@@ -160,7 +182,7 @@ def main():
         [os.path.join(os.path.dirname(__file__), 'update.sh')] +  sys.argv[1:],
         stderr=os.fdopen(os.dup(sys.stdin.fileno())))
 
-  if not re.search('clang=1', os.environ.get('GYP_DEFINES', '')):
+  if not re.search('(clang|asan)=1', os.environ.get('GYP_DEFINES', '')):
     print 'Skipping Clang update (clang=1 was not set in GYP_DEFINES).'
     return 0

[hans, 2014-02-21 22:46:45]

The CQ bit was checked by hans@chromium.org

[commit-bot: I haz the power, 2014-02-21 22:48:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hans@chromium.org/137823014/440003

[commit-bot: I haz the power, 2014-02-22 00:24:00]

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

[commit-bot: I haz the power, 2014-02-22 00:24:00]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[hans, 2014-02-22 00:27:18]

Committed patchset #12 manually as r252707 (presubmit successful).