Add gn Blink GC plugin options

Add gn Blink GC plugin options

The Blink GC clang plugin supports a couple of extra options which the
Blink GN configuration did not expose. Do so here, but without depending
on the 'flags' script used by the gyp build system
(tools/clang/scripts/blink_gc_plugin_flags.py).

Specifically, this adds the following Blink GN variables:

 - blink_gc_plugin_lib_path = "<path>"
     overridable path to the directory of the GC plugin dynamic library.
 - blink_gc_plugin_option_do_dump_graph [ = false ]
     emit JSON-serialized representation of class graph.
 - blink_gc_plugin_option_warn_unneeded_finalizer [ = false ]
     warn of unnecessary destructor usage.

R=
BUG=

[sof, 2016-06-19 07:33:25]

Description was changed from

==========
Add gn Blink GC plugin options

The Blink GC clang plugin supports a couple of extra options which the
Blink GN configuration did not expose. Do so here, but without depending
on the 'flags' script used by the gyp build system
(tools/clang/scripts/blink_gc_plugin_flags.py).

Specifically, this adds the following Blink GN variables:

blink_gc_plugin_lib_path = "<path>"
overridable path to the directory of the GC plugin dynamic library.
blink_gc_plugin_option_do_dump_graph [ = false ]
emit JSON-serialized representation of class graph.
blink_gc_plugin_option_warn_unneeded_finalizer [ = false ]
warn of unnecessary destructor usage.

R=
BUG=
==========

to

==========
Add gn Blink GC plugin options

The Blink GC clang plugin supports a couple of extra options which the
Blink GN configuration did not expose. Do so here, but without depending
on the 'flags' script used by the gyp build system
(tools/clang/scripts/blink_gc_plugin_flags.py).

Specifically, this adds the following Blink GN variables:

 - blink_gc_plugin_lib_path = "<path>"
     overridable path to the directory of the GC plugin dynamic library.
 - blink_gc_plugin_option_do_dump_graph [ = false ]
     emit JSON-serialized representation of class graph.
 - blink_gc_plugin_option_warn_unneeded_finalizer [ = false ]
     warn of unnecessary destructor usage.

R=
BUG=
==========

[sof, 2016-06-19 07:36:56]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org, thakis@chromium.org

[sof, 2016-06-19 07:36:56]

please take a look.

I'm not familiar with GN, so exposing these plugin options as top-level options
for Blink might not be the preferred way to structure things. Please advise, if
so.

[haraken, 2016-06-19 11:07:27]

Yeah, it looks better to ask Nico for the review.

[Nico, 2016-06-21 20:53:14]

Generally lgtm. If you want to land this without another hop:

* move declare_args into the BUILD.gn
* remove the blink_gc_plugin_lib_path arg for now

Or if you rather want to land one Cl instead of several, happy to iterate on
this one too :-)

https://codereview.chromium.org/2081543002/diff/1/third_party/WebKit/Source/B...
File third_party/WebKit/Source/BUILD.gn (right):

https://codereview.chromium.org/2081543002/diff/1/third_party/WebKit/Source/B...
third_party/WebKit/Source/BUILD.gn:66:
"${blink_gc_plugin_lib_path}/libBlinkGCPlugin.${_blink_gc_plugin_dll_extension}",
this should only be done if is_mac || is_ios || is_linux (i.e. !is_win -- on
Windows, we compile the plugins into clang.exe itself since the way symbol
visibility works on windows makes "real" clang plugins tricky there)

https://codereview.chromium.org/2081543002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/config.gni (right):

https://codereview.chromium.org/2081543002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/config.gni:22: blink_gc_plugin_option_do_dump_graph =
false
you can have declare_args in BUILD.gn files, so if this is only needed in one
file (it looks like it is), then this should be in that file (random example:
https://cs.chromium.org/chromium/src/skia/BUILD.gn?rcl=0&l=23)

https://codereview.chromium.org/2081543002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/config.gni:32: blink_gc_plugin_lib_path =
"//third_party/llvm-build/Release+Asserts/lib/"
Hm, the plugin usually only works with clangs build at the same revision --
should we have a "local_clang_build_path" thingy that's used for the clang
compiler, the normal style plugin, and the blink plugin?

[sof, 2016-06-21 21:12:39]

thanks, i'll follow up the feedback tomorrow.

https://codereview.chromium.org/2081543002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/config.gni (right):

https://codereview.chromium.org/2081543002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/config.gni:32: blink_gc_plugin_lib_path =
"//third_party/llvm-build/Release+Asserts/lib/"
On 2016/06/21 20:53:13, Nico wrote:
> Hm, the plugin usually only works with clangs build at the same revision --
> should we have a "local_clang_build_path" thingy that's used for the clang
> compiler, the normal style plugin, and the blink plugin?

I think it makes sense to broaden the configurable option like that; I haven't
investigated (yet) why https://codereview.chromium.org/1052033006 singled out
libBlinkGCPlugin

[sof, 2016-06-22 10:58:04]

https://codereview.chromium.org/2081543002/diff/1/third_party/WebKit/Source/B...
File third_party/WebKit/Source/BUILD.gn (right):

https://codereview.chromium.org/2081543002/diff/1/third_party/WebKit/Source/B...
third_party/WebKit/Source/BUILD.gn:66:
"${blink_gc_plugin_lib_path}/libBlinkGCPlugin.${_blink_gc_plugin_dll_extension}",
On 2016/06/21 20:53:13, Nico wrote:
> this should only be done if is_mac || is_ios || is_linux (i.e. !is_win -- on
> Windows, we compile the plugins into clang.exe itself since the way symbol
> visibility works on windows makes "real" clang plugins tricky there)

Thanks, fixed (the regression.)

https://codereview.chromium.org/2081543002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/config.gni (right):

https://codereview.chromium.org/2081543002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/config.gni:22: blink_gc_plugin_option_do_dump_graph =
false
On 2016/06/21 20:53:13, Nico wrote:
> you can have declare_args in BUILD.gn files, so if this is only needed in one
> file (it looks like it is), then this should be in that file (random example:
> https://cs.chromium.org/chromium/src/skia/BUILD.gn?rcl=0&l=23)

I've moved the GC plugin ones to Source/BUILD.gn , if we don't want to keep
Source/ configurables in one place.

https://codereview.chromium.org/2081543002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/config.gni:32: blink_gc_plugin_lib_path =
"//third_party/llvm-build/Release+Asserts/lib/"
On 2016/06/21 21:12:39, sof wrote:
> On 2016/06/21 20:53:13, Nico wrote:
> > Hm, the plugin usually only works with clangs build at the same revision --
> > should we have a "local_clang_build_path" thingy that's used for the clang
> > compiler, the normal style plugin, and the blink plugin?
> 
> I think it makes sense to broaden the configurable option like that; I haven't
> investigated (yet) why https://codereview.chromium.org/1052033006 singled out
> libBlinkGCPlugin

Didn't go the whole hog, but added clang_plugin_lib_path to clang.gni

[Nico, 2016-06-22 14:31:47]

https://codereview.chromium.org/2081543002/diff/40001/build/config/clang/clan...
File build/config/clang/clang.gni (right):

https://codereview.chromium.org/2081543002/diff/40001/build/config/clang/clan...
build/config/clang/clang.gni:13: # distributed compilation setups.
Is it useful to switch plugin path independent from compiler path though? I'd
expect that if you set this, the plugin will just crash if it runs with the
pinned compiler.

(If this is useful to you, I'm not opposed to it, but I don't understand how it
can be useful yet.)

[sof, 2016-06-22 14:39:49]

https://codereview.chromium.org/2081543002/diff/40001/build/config/clang/clan...
File build/config/clang/clang.gni (right):

https://codereview.chromium.org/2081543002/diff/40001/build/config/clang/clan...
build/config/clang/clang.gni:13: # distributed compilation setups.
On 2016/06/22 14:31:47, Nico wrote:
> Is it useful to switch plugin path independent from compiler path though? I'd
> expect that if you set this, the plugin will just crash if it runs with the
> pinned compiler.
> 
> (If this is useful to you, I'm not opposed to it, but I don't understand how
it
> can be useful yet.)

I'm not using it, just carrying over pre-existing functionality.

But if you arrange for a suitable PATH, it should work.

[Nico, 2016-06-22 14:41:46]

On 2016/06/22 14:39:49, sof wrote:
>
https://codereview.chromium.org/2081543002/diff/40001/build/config/clang/clan...
> File build/config/clang/clang.gni (right):
> 
>
https://codereview.chromium.org/2081543002/diff/40001/build/config/clang/clan...
> build/config/clang/clang.gni:13: # distributed compilation setups.
> On 2016/06/22 14:31:47, Nico wrote:
> > Is it useful to switch plugin path independent from compiler path though?
I'd
> > expect that if you set this, the plugin will just crash if it runs with the
> > pinned compiler.
> > 
> > (If this is useful to you, I'm not opposed to it, but I don't understand how
> it
> > can be useful yet.)
> 
> I'm not using it, just carrying over pre-existing functionality.

Let's add it when someone wants to use it then.

> But if you arrange for a suitable PATH, it should work.

No, we put a qualified path to the compiler in the generated ninja file:

$ head out/gn/toolchain.ninja 
rule cc
  command = ../../third_party/llvm-build/Release+Asserts/bin/clang

[sof, 2016-06-22 14:45:15]

On 2016/06/22 14:41:46, Nico wrote:
> On 2016/06/22 14:39:49, sof wrote:
> >
>
https://codereview.chromium.org/2081543002/diff/40001/build/config/clang/clan...
> > File build/config/clang/clang.gni (right):
> > 
> >
>
https://codereview.chromium.org/2081543002/diff/40001/build/config/clang/clan...
> > build/config/clang/clang.gni:13: # distributed compilation setups.
> > On 2016/06/22 14:31:47, Nico wrote:
> > > Is it useful to switch plugin path independent from compiler path though?
> I'd
> > > expect that if you set this, the plugin will just crash if it runs with
the
> > > pinned compiler.
> > > 
> > > (If this is useful to you, I'm not opposed to it, but I don't understand
how
> > it
> > > can be useful yet.)
> > 
> > I'm not using it, just carrying over pre-existing functionality.
> 
> Let's add it when someone wants to use it then.
> 
> > But if you arrange for a suitable PATH, it should work.
> 
> No, we put a qualified path to the compiler in the generated ninja file:
> 
> $ head out/gn/toolchain.ninja 
> rule cc
>   command = ../../third_party/llvm-build/Release+Asserts/bin/clang

Really, I had to tweak my PATH here locally just the other day to prevent the
system-installed clang from being used. But perhaps that was because of CXX
being defined as an env var.

[Nico, 2016-06-22 14:50:02]

On 2016/06/22 14:45:15, sof wrote:
> On 2016/06/22 14:41:46, Nico wrote:
> > On 2016/06/22 14:39:49, sof wrote:
> > >
> >
>
https://codereview.chromium.org/2081543002/diff/40001/build/config/clang/clan...
> > > File build/config/clang/clang.gni (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2081543002/diff/40001/build/config/clang/clan...
> > > build/config/clang/clang.gni:13: # distributed compilation setups.
> > > On 2016/06/22 14:31:47, Nico wrote:
> > > > Is it useful to switch plugin path independent from compiler path
though?
> > I'd
> > > > expect that if you set this, the plugin will just crash if it runs with
> the
> > > > pinned compiler.
> > > > 
> > > > (If this is useful to you, I'm not opposed to it, but I don't understand
> how
> > > it
> > > > can be useful yet.)
> > > 
> > > I'm not using it, just carrying over pre-existing functionality.
> > 
> > Let's add it when someone wants to use it then.
> > 
> > > But if you arrange for a suitable PATH, it should work.
> > 
> > No, we put a qualified path to the compiler in the generated ninja file:
> > 
> > $ head out/gn/toolchain.ninja 
> > rule cc
> >   command = ../../third_party/llvm-build/Release+Asserts/bin/clang
> 
> Really, I had to tweak my PATH here locally just the other day to prevent the
> system-installed clang from being used. But perhaps that was because of CXX
> being defined as an env var.

gyp looks at os.environ['CXX'], but I think gn doesn't (?)

[sof, 2016-06-22 15:24:09]

On 2016/06/22 14:50:02, Nico wrote:
> On 2016/06/22 14:45:15, sof wrote:
> > On 2016/06/22 14:41:46, Nico wrote:
> > > On 2016/06/22 14:39:49, sof wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2081543002/diff/40001/build/config/clang/clan...
> > > > File build/config/clang/clang.gni (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2081543002/diff/40001/build/config/clang/clan...
> > > > build/config/clang/clang.gni:13: # distributed compilation setups.
> > > > On 2016/06/22 14:31:47, Nico wrote:
> > > > > Is it useful to switch plugin path independent from compiler path
> though?
> > > I'd
> > > > > expect that if you set this, the plugin will just crash if it runs
with
> > the
> > > > > pinned compiler.
> > > > > 
> > > > > (If this is useful to you, I'm not opposed to it, but I don't
understand
> > how
> > > > it
> > > > > can be useful yet.)
> > > > 
> > > > I'm not using it, just carrying over pre-existing functionality.
> > > 
> > > Let's add it when someone wants to use it then.
> > > 
> > > > But if you arrange for a suitable PATH, it should work.
> > > 
> > > No, we put a qualified path to the compiler in the generated ninja file:
> > > 
> > > $ head out/gn/toolchain.ninja 
> > > rule cc
> > >   command = ../../third_party/llvm-build/Release+Asserts/bin/clang
> > 
> > Really, I had to tweak my PATH here locally just the other day to prevent
the
> > system-installed clang from being used. But perhaps that was because of CXX
> > being defined as an env var.
> 
> gyp looks at os.environ['CXX'], but I think gn doesn't (?)

Yes, you're right - just tested + looked at build/toolchain/gcc_toolchain.gni
(and others in that directory.)

[sof, 2016-06-22 19:44:19]

On 2016/06/22 15:24:09, sof wrote:
> On 2016/06/22 14:50:02, Nico wrote:
...
> > 
> > gyp looks at os.environ['CXX'], but I think gn doesn't (?)
> 
> Yes, you're right - just tested + looked at build/toolchain/gcc_toolchain.gni
> (and others in that directory.)

Now generalized, adding clang_base_path.

(If this is worth introducing overall, this CL should probably be split into
two.)

[Nico, 2016-06-22 20:01:56]

lgtm

Thanks, this looks great. I wished for a toggle like this in the past, and I
know pcc did too.

I agree that splitting this in too sounds like a good idea (feel free to tbr
each to me)

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

https://codereview.chromium.org/2081543002/diff/80001/build/config/sanitizers...
build/config/sanitizers/BUILD.gn:54: data += [
"$clang_base_path/bin/llvm-symbolizer${exe}" ]
not sure about the changes in this file -- these don't exist in normal local
builds but are put there by the clang packaging script. But let's go with this
for now.

[pcc1, 2016-06-22 20:31:41]

Thanks for working on this!

When I envisioned adding a configuration flag to override the toolchain path, I
imagined that such a flag would only be used to provide the location of a
self-built LLVM build tree to build against. As a consequence the plugins would
not be available, and so providing that flag would cause them to be disabled
altogether.

Are there use cases for building with a toolchain outside of the default
location with the plugins enabled?

[sof, 2016-06-22 21:00:10]

On 2016/06/22 20:31:41, pcc1 wrote:
> Thanks for working on this!
> 
> When I envisioned adding a configuration flag to override the toolchain path,
I
> imagined that such a flag would only be used to provide the location of a
> self-built LLVM build tree to build against. As a consequence the plugins
would
> not be available, and so providing that flag would cause them to be disabled
> altogether.
> 
> Are there use cases for building with a toolchain outside of the default
> location with the plugins enabled?

distcc / icecc slaves would be one, where you really don't want to build Blink
without the GC plugin available.

[sof, 2016-06-23 09:53:51]

On 2016/06/22 20:01:56, Nico wrote:
> lgtm
> 
> Thanks, this looks great. I wished for a toggle like this in the past, and I
> know pcc did too.
> 
> I agree that splitting this in too sounds like a good idea (feel free to tbr
> each to me)
> 

Split up as https://codereview.chromium.org/2097433002/ +
https://codereview.chromium.org/2088373002/.

Thanks for the reviews, closing this one.