Add blink_gc_plugin variable to enable the clang plugin for checking GC consistency.

Add blink_gc_plugin variable to enable the clang plugin for checking GC consistency.

R=abarth@chromium.org, ager@chromium.org
BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166005

[zerny-chromium, 2014-01-10 08:01:37]

Until we have support for building with GOMA the plugin is disabled by default.

Plugin stub in CL: https://codereview.chromium.org/133463002/

[wibling-chromium, 2014-01-10 08:19:26]

lgtm

[Mads Ager (chromium), 2014-01-10 09:20:50]

I wonder if we can have this gyp flag closer to the location of the plugins so
we do not have to dot our way out of the Blink directory in a Blink-side gyp
file?

https://codereview.chromium.org/133473002/diff/1/Source/config.gyp
File Source/config.gyp (right):

https://codereview.chromium.org/133473002/diff/1/Source/config.gyp#newcode35
Source/config.gyp:35: # Set to 1 to check GC infrastructure when compiling with
clang.
Maybe write this out a bit more:

Set to 1 to enable the clang plugin that checks the usage of the Blink
garbage-collection infrastructure during compilation.

?

https://codereview.chromium.org/133473002/diff/1/Source/config.gyp#newcode106
Source/config.gyp:106: 'cflags':
['
          

[zerny-chromium, 2014-01-10 09:53:49]

Thanks for the reviews.

https://codereview.chromium.org/133473002/diff/1/Source/config.gyp
File Source/config.gyp (right):

https://codereview.chromium.org/133473002/diff/1/Source/config.gyp#newcode35
Source/config.gyp:35: # Set to 1 to check GC infrastructure when compiling with
clang.
On 2014/01/10 09:20:50, Mads Ager (chromium) wrote:
> Maybe write this out a bit more:
> 
> Set to 1 to enable the clang plugin that checks the usage of the Blink
> garbage-collection infrastructure during compilation.
> 
> ?

Done.

https://codereview.chromium.org/133473002/diff/1/Source/config.gyp#newcode106
Source/config.gyp:106: 'cflags':
['
          

[zerny-chromium, 2014-01-10 10:45:51]

> Maybe we could read the plugin flags
> into a blink_gc_plugin_flags gyp variable and then use that to set cflags
inside
> blink.

I've now restructured the gyp setup in the two patches to that effect.

[Mads Ager (chromium), 2014-01-10 10:50:44]

This looks a lot cleaner to me. A last couple of questions from me:

https://codereview.chromium.org/133473002/diff/80001/Source/config.gyp
File Source/config.gyp (right):

https://codereview.chromium.org/133473002/diff/80001/Source/config.gyp#newcod...
Source/config.gyp:106: ['blink_gc_plugin==1 and clang==1 and
clang_use_chrome_plugins==1 and OS!="win"', {
Is the OS!="win" needed? If we start using clang on Windows it seems that we
should use the plugins as well.

Do we want the use_chrome_plugins part? Do we rely on anything that is only
there when clang_use_chrome_plugins is set?

[zerny-chromium, 2014-01-10 11:33:58]

https://codereview.chromium.org/133473002/diff/80001/Source/config.gyp
File Source/config.gyp (right):

https://codereview.chromium.org/133473002/diff/80001/Source/config.gyp#newcod...
Source/config.gyp:106: ['blink_gc_plugin==1 and clang==1 and
clang_use_chrome_plugins==1 and OS!="win"', {
On 2014/01/10 10:50:45, Mads Ager (chromium) wrote:
> Is the OS!="win" needed? If we start using clang on Windows it seems that we
> should use the plugins as well.
> 
> Do we want the use_chrome_plugins part? Do we rely on anything that is only
> there when clang_use_chrome_plugins is set?

Currently build/common.gypi [1] only defines the plugin flags when not on
windows. I'm not sure that the setup actually works if clang==1 and OS=="win"
since there are unguarded uses of the variables being defined under the
assumption that OS!="win".

I'll remove the OS!="win" condition here, since if clang_blink_gc_plugin_flags
is properly defined for windows then this code works too.

The clang_use_chrome_plugins variable is default enabled but is used to disable
plugins if need be, eg, when running the type profiler which is a different
clang build IFAICT.

I think we should keep that condition because if the exisiting plugin does not
work with the type profiler then I doubt this one will.

[1] https://codereview.chromium.org/133463002/diff/70001/build/common.gypi

[Mads Ager (chromium), 2014-01-10 12:14:17]

LGTM

[zerny-chromium, 2014-01-20 08:21:06]

Reverted the gyp restructuring by request in
https://codereview.chromium.org/133463002/

[commit-bot: I haz the power, 2014-01-24 19:06:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zerny@chromium.org/133473002/170001

[commit-bot: I haz the power, 2014-01-24 19:22:43]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[abarth-chromium, 2014-01-29 00:54:22]

lgtm

[commit-bot: I haz the power, 2014-01-29 00:54:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zerny@chromium.org/133473002/170001

[commit-bot: I haz the power, 2014-01-29 07:18:20]

Change committed as 166005

[commit-bot: I haz the power, 2014-01-29 07:18:55]

CQ bit was unchecked on CL. Ignoring.