Build the Clang plugin on Windows.

Build the Clang plugin on Windows.

This also includes several fixes for the plugin logic on Windows:
- The banned directory check normalizes path separators.
- The banned directory check now returns true for files that are
  considered system headers.
- Diagnostics from the plugin are always reported as warnings.

BUG=467287
R=thakis@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/26ba172e8543b9029f320600f3aeced40028ff20

Committed: https://chromium.googlesource.com/chromium/src/+/5b5d1269ade9eeb50875b20570ef3e0d8c5d95fb

Committed: https://chromium.googlesource.com/chromium/src/+/328935a86e8f684bc7187c365b2e9639ddb65dc2

[dcheng, 2015-04-09 21:15:43]

dcheng@chromium.org changed reviewers:
+ hans@chromium.org, thakis@chromium.org

[dcheng, 2015-04-09 21:15:43]

Bwhahahahaha.

The initial approach I tried was building the plugin as a static library and
linking it in. It got a bit too complicated--I wanted to minimize the amount of
patches that we would need to apply to clang/llvm's source tree, and the linker
kept stripping out the plugin's static lib.

Instead, I use the approach of modifying SOURCES and compile the plugin directly
into clang. Unfortunately, this patch means we'll need CMake 3.1 on the bots
(which I'm not sure that we have--it was only released in December 2014).

Disclaimer: I need to test the build script/gclient runhooks on Mac/Linux to
make sure I didn't break anything there. On Windows, things appear to be fine
though. 

After this, I'd like to:
- Enable building the plugin by default on Windows.
- Force all messages to be warnings for now on Windows.
- Update build/common.gypi to let people (and bots) opt in on Windows to use the
plugin.

Open question: Do I need to build the plugin into clang-cl as well? I'm not
super familiar with how the two interact.

[Nico, 2015-04-09 21:26:03]

Sooo…if we do this on mac/linux too, we can set the thinger that hides most
symbols (CLANG_PLUGIN_SUPPORT=0 in the make build; apparently the same in the
cmake build)? The cmakefile claims that that makes compilation measurably
faster…could you measure that claim on linux or mac? If so, we probably want to
do the same on all 3 platforms maybe

[hans, 2015-04-09 22:28:37]

On 2015/04/09 21:15:43, dcheng wrote:
> Bwhahahahaha.
> 
> The initial approach I tried was building the plugin as a static library and
> linking it in. It got a bit too complicated--I wanted to minimize the amount
of
> patches that we would need to apply to clang/llvm's source tree, and the
linker
> kept stripping out the plugin's static lib.
> 
> Instead, I use the approach of modifying SOURCES and compile the plugin
directly
> into clang. Unfortunately, this patch means we'll need CMake 3.1 on the bots
> (which I'm not sure that we have--it was only released in December 2014).

We might be OK here. The Windows bot I looked at (one of the perf bots) has
cmake 3.2.1. I don't remember how this works, but I think it might be part of
the OS image that the infra folks use for all bots. In that case we'd be all
right.

The cmake used on our Linux ToT builders is 3.1.0-rc2, and the Mac ToT bots use
3.0.1 but I can upgrade that if needed.

> Open question: Do I need to build the plugin into clang-cl as well? I'm not
> super familiar with how the two interact.

clang-cl is just a symlink to clang (or copy, on Windows).

[dcheng, 2015-04-10 00:55:50]

On 2015/04/09 at 21:26:03, thakis wrote:
> Sooo…if we do this on mac/linux too, we can set the thinger that hides most
symbols (CLANG_PLUGIN_SUPPORT=0 in the make build; apparently the same in the
cmake build)? The cmakefile claims that that makes compilation measurably
faster…could you measure that claim on linux or mac? If so, we probably want to
do the same on all 3 platforms maybe

The performance difference appears to be negligible. I changed
plugins/CMakeLists.txt to gate on CLANG_PLUGIN_SUPPORT rather than WIN32,
disabled the Blink GC plugin, and then alternated between plugin and non-plugin
builds.

=== CLANG_PLUGIN_SUPPORT=on ===
dcheng@nausicaa:~/src/chrome/src$ time ninja -C out/Debug chrome                
                   
ninja: Entering directory `out/Debug'
[18644/18644] LINK chrome

real    22m25.461s
user    630m4.705s
sys     32m56.878s

dcheng@nausicaa:~/src/chrome/src$ time ninja -C out/Debug chrome                
                   
ninja: Entering directory `out/Debug'
[18644/18644] LINK chrome

real    22m17.626s
user    630m2.214s
sys     32m9.156s

=== CLANG_PLUGIN_SUPPORT=off ===
dcheng@nausicaa:~/src/chrome/src$ time ninja -C out/Debug chrome                
                   
ninja: Entering directory `out/Debug'
[18644/18644] LINK chrome

real    22m21.824s
user    628m47.873s
sys     33m2.449s

dcheng@nausicaa:~/src/chrome/src$ time ninja -C out/Debug chrome                
                   
ninja: Entering directory `out/Debug'
[18644/18644] LINK chrome

real    22m12.614s
user    628m54.487s
sys     31m54.618s

[dcheng, 2015-04-10 00:57:16]

On 2015/04/10 at 00:55:50, dcheng wrote:
> On 2015/04/09 at 21:26:03, thakis wrote:
> > Sooo…if we do this on mac/linux too, we can set the thinger that hides most
symbols (CLANG_PLUGIN_SUPPORT=0 in the make build; apparently the same in the
cmake build)? The cmakefile claims that that makes compilation measurably
faster…could you measure that claim on linux or mac? If so, we probably want to
do the same on all 3 platforms maybe
> 
> The performance difference appears to be negligible. I changed
plugins/CMakeLists.txt to gate on CLANG_PLUGIN_SUPPORT rather than WIN32,
disabled the Blink GC plugin, and then alternated between plugin and non-plugin
builds.
> 
> === CLANG_PLUGIN_SUPPORT=on ===
> dcheng@nausicaa:~/src/chrome/src$ time ninja -C out/Debug chrome              
                     
> ninja: Entering directory `out/Debug'
> [18644/18644] LINK chrome
> 
> real    22m25.461s
> user    630m4.705s
> sys     32m56.878s
> 
> dcheng@nausicaa:~/src/chrome/src$ time ninja -C out/Debug chrome              
                     
> ninja: Entering directory `out/Debug'
> [18644/18644] LINK chrome
> 
> real    22m17.626s
> user    630m2.214s
> sys     32m9.156s
> 
> === CLANG_PLUGIN_SUPPORT=off ===
> dcheng@nausicaa:~/src/chrome/src$ time ninja -C out/Debug chrome              
                     
> ninja: Entering directory `out/Debug'
> [18644/18644] LINK chrome
> 
> real    22m21.824s
> user    628m47.873s
> sys     33m2.449s
> 
> dcheng@nausicaa:~/src/chrome/src$ time ninja -C out/Debug chrome              
                     
> ninja: Entering directory `out/Debug'
> [18644/18644] LINK chrome
> 
> real    22m12.614s
> user    628m54.487s
> sys     31m54.618s

That being said, I didn't try running clang in a tight loop on a simpler .cc
file. It's possible that Chrome source is complicated enough that the startup
time of clang is just noise.

Let me know if there's other metrics you're interested in, and I can collect
those too.

[Nico, 2015-04-10 00:59:44]

That was without goma, and with a build where out/Debug/build.ninja pointed
to your local clang, yes? (Just checking… ;-))

If so, there's probably no point in doing this on non-win. Thanks for
checking!

On Thu, Apr 9, 2015 at 5:55 PM, <dcheng@chromium.org> wrote:

> On 2015/04/09 at 21:26:03, thakis wrote:
>
>> Sooo…if we do this on mac/linux too, we can set the thinger that hides
>> most
>>
> symbols (CLANG_PLUGIN_SUPPORT=0 in the make build; apparently the same in
> the
> cmake build)? The cmakefile claims that that makes compilation measurably
> faster…could you measure that claim on linux or mac? If so, we probably
> want to
> do the same on all 3 platforms maybe
>
> The performance difference appears to be negligible. I changed
> plugins/CMakeLists.txt to gate on CLANG_PLUGIN_SUPPORT rather than WIN32,
> disabled the Blink GC plugin, and then alternated between plugin and
> non-plugin
> builds.
>
> === CLANG_PLUGIN_SUPPORT=on ===
> dcheng@nausicaa:~/src/chrome/src$ time ninja -C out/Debug chrome
>
> ninja: Entering directory `out/Debug'
> [18644/18644] LINK chrome
>
> real    22m25.461s
> user    630m4.705s
> sys     32m56.878s
>
> dcheng@nausicaa:~/src/chrome/src$ time ninja -C out/Debug chrome
>
> ninja: Entering directory `out/Debug'
> [18644/18644] LINK chrome
>
> real    22m17.626s
> user    630m2.214s
> sys     32m9.156s
>
> === CLANG_PLUGIN_SUPPORT=off ===
> dcheng@nausicaa:~/src/chrome/src$ time ninja -C out/Debug chrome
>
> ninja: Entering directory `out/Debug'
> [18644/18644] LINK chrome
>
> real    22m21.824s
> user    628m47.873s
> sys     33m2.449s
>
> dcheng@nausicaa:~/src/chrome/src$ time ninja -C out/Debug chrome
>
> ninja: Entering directory `out/Debug'
> [18644/18644] LINK chrome
>
> real    22m12.614s
> user    628m54.487s
> sys     31m54.618s
>
> https://codereview.chromium.org/1072203002/
>

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

[dcheng, 2015-04-10 01:01:22]

On 2015/04/10 at 00:59:44, thakis wrote:
> That was without goma, and with a build where out/Debug/build.ninja pointed
> to your local clang, yes? (Just checking… ;-))
> 

Yep, goma was disabled and out/Debug/build.ninja points to the local clang.

> If so, there's probably no point in doing this on non-win. Thanks for
> checking!

I was hoping for more interesting results =)

[dcheng, 2015-04-10 17:45:00]

PTAL.

As far as I can tell, Windows and non-Windows builds still work fine. I also
updated 'plugins' to be built by default on Windows, and forced all plugin
messages to be warnings for now on Windows.

[Nico, 2015-04-11 23:32:31]

https://codereview.chromium.org/1072203002/diff/60001/tools/clang/plugins/Chr...
File tools/clang/plugins/ChromeClassTester.cpp (right):

https://codereview.chromium.org/1072203002/diff/60001/tools/clang/plugins/Chr...
tools/clang/plugins/ChromeClassTester.cpp:148:
banned_directories_.emplace_back("/win_toolchain/");
clang-cl should konw that stuff in win_toolchain are system headers already,
since gyp writes that into the INCLUDE env var in
out/Release/environment.{x86,x64} and clang knows that stuff in INCLUDE are
system headers. (See also https://codereview.chromium.org/406523005/ and
https://code.google.com/p/chromium/issues/detail?id=395405). If this doesn't
Just Work, we would have problems with regular warnings in system headers too.

[dcheng, 2015-04-12 03:28:58]

https://codereview.chromium.org/1072203002/diff/60001/tools/clang/plugins/Chr...
File tools/clang/plugins/ChromeClassTester.cpp (right):

https://codereview.chromium.org/1072203002/diff/60001/tools/clang/plugins/Chr...
tools/clang/plugins/ChromeClassTester.cpp:148:
banned_directories_.emplace_back("/win_toolchain/");
On 2015/04/11 23:32:31, Nico (away until Wed Apr 15) wrote:
> clang-cl should konw that stuff in win_toolchain are system headers already,
> since gyp writes that into the INCLUDE env var in
> out/Release/environment.{x86,x64} and clang knows that stuff in INCLUDE are
> system headers. (See also https://codereview.chromium.org/406523005/ and
> https://code.google.com/p/chromium/issues/detail?id=395405). If this doesn't
> Just Work, we would have problems with regular warnings in system headers too.

Interesting, didn't know about that. I updated InBannedDirectories to use the
SourceManager bit to determine this.

[Nico, 2015-04-12 04:19:14]

lgtm

tools/clang/blink_gc_plugin will happen in a different CL, I assume?

https://codereview.chromium.org/1072203002/diff/80001/tools/clang/plugins/CMa...
File tools/clang/plugins/CMakeLists.txt (right):

https://codereview.chromium.org/1072203002/diff/80001/tools/clang/plugins/CMa...
tools/clang/plugins/CMakeLists.txt:27: cr_add_test(plugins_test
Should this get a "TODO: port script to python, run on Windows too"?

https://codereview.chromium.org/1072203002/diff/80001/tools/clang/plugins/Chr...
File tools/clang/plugins/ChromeClassTester.cpp (right):

https://codereview.chromium.org/1072203002/diff/80001/tools/clang/plugins/Chr...
tools/clang/plugins/ChromeClassTester.cpp:134:
banned_namespaces_.emplace_back("std");
nit: I'd keep push_back since everybody knows what that does, this isn't
perf-critical, and it keeps the diff shorter

[dcheng, 2015-04-12 06:29:30]

https://codereview.chromium.org/1072203002/diff/80001/tools/clang/plugins/CMa...
File tools/clang/plugins/CMakeLists.txt (right):

https://codereview.chromium.org/1072203002/diff/80001/tools/clang/plugins/CMa...
tools/clang/plugins/CMakeLists.txt:27: cr_add_test(plugins_test
On 2015/04/12 04:19:14, Nico (away until Wed Apr 15) wrote:
> Should this get a "TODO: port script to python, run on Windows too"?

Done.

https://codereview.chromium.org/1072203002/diff/80001/tools/clang/plugins/Chr...
File tools/clang/plugins/ChromeClassTester.cpp (right):

https://codereview.chromium.org/1072203002/diff/80001/tools/clang/plugins/Chr...
tools/clang/plugins/ChromeClassTester.cpp:134:
banned_namespaces_.emplace_back("std");
On 2015/04/12 04:19:14, Nico (away until Wed Apr 15) wrote:
> nit: I'd keep push_back since everybody knows what that does, this isn't
> perf-critical, and it keeps the diff shorter

But but but C++11!

I'll probably revisit this in another CL in the future =)

[commit-bot: I haz the power, 2015-04-12 06:38:57]

Patchset 6 (id:??) landed as
https://crrev.com/26ba172e8543b9029f320600f3aeced40028ff20
Cr-Commit-Position: refs/heads/master@{#324793}

[dcheng, 2015-04-12 06:39:15]

Committed patchset #6 (id:100001) manually as
26ba172e8543b9029f320600f3aeced40028ff20 (presubmit successful).

[dcheng, 2015-04-12 07:33:00]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1070353003/ by dcheng@chromium.org.

The reason for reverting is: Not all the Windows bots have CMake 3.1+..

[hans, 2015-04-12 15:24:07]

On 2015/04/12 07:33:00, dcheng wrote:
> A revert of this CL (patchset #6 id:100001) has been created in
> https://codereview.chromium.org/1070353003/ by mailto:dcheng@chromium.org.
> 
> The reason for reverting is: Not all the Windows bots have CMake 3.1+..

Sorry :( I assumed they all had the same version as the one I looked at. Can you
file a bug and cc me to get it upgraded?

[dcheng, 2015-04-13 18:33:33]

On 2015/04/12 at 15:24:07, hans wrote:
> On 2015/04/12 07:33:00, dcheng wrote:
> > A revert of this CL (patchset #6 id:100001) has been created in
> > https://codereview.chromium.org/1070353003/ by mailto:dcheng@chromium.org.
> > 
> > The reason for reverting is: Not all the Windows bots have CMake 3.1+..
> 
> Sorry :( I assumed they all had the same version as the one I looked at. Can
you file a bug and cc me to get it upgraded?

Yeah, I filed https://code.google.com/p/chromium/issues/detail?id=476362 for
this. Sorry for breaking the bots =(

[Nico, 2015-04-16 00:15:10]

I think you can try relanding this.

[commit-bot: I haz the power, 2015-04-16 01:09:53]

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

[dcheng, 2015-04-16 01:10:05]

Committed patchset #7 (id:120001) manually as
5b5d1269ade9eeb50875b20570ef3e0d8c5d95fb (presubmit successful).

[dcheng, 2015-04-16 01:38:32]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1087423003/ by dcheng@chromium.org.

The reason for reverting is: Rebase went horribly wrong....

[commit-bot: I haz the power, 2015-04-16 01:49:21]

Patchset 8 (id:??) landed as
https://crrev.com/328935a86e8f684bc7187c365b2e9639ddb65dc2
Cr-Commit-Position: refs/heads/master@{#325373}

[dcheng, 2015-04-16 01:49:35]

Committed patchset #8 (id:140001) manually as
328935a86e8f684bc7187c365b2e9639ddb65dc2 (presubmit successful).