Stub clang plugin to check consistency of the Blink GC infrastructure.

Stub clang plugin to check consistency of the Blink GC infrastructure.

R=ager@chromium.org, thakis@chromium.org
BUG=334149
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245897

[zerny-chromium, 2014-01-10 08:22:30]

The plugin is enabled from the Blink gyp files:
https://codereview.chromium.org/133473002/

Until we have GOMA support it is disabled by default.

[Nico, 2014-01-14 01:54:00]

Please file a tracking bug explaining what you intend to do. Other than this,
this mostly lgtm.

If you only intend to use clang_blink_gc_plugin_flags in blink gyp files, should
it be in third_party/WebKit/Source/config.gyp instead?

[Nico, 2014-01-14 01:54:42]

https://codereview.chromium.org/133463002/diff/70001/tools/clang/blink_gc_plu...
File tools/clang/blink_gc_plugin/README.chromium (right):

https://codereview.chromium.org/133463002/diff/70001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/README.chromium:2: collection infrastructure.
Once this plugin implements diagnostics, please write a page like
http://www.chromium.org/developers/coding-style/chromium-style-checker-errors
and link to it from here.

[zerny-chromium, 2014-01-14 09:05:44]

Thanks for the review and for pointing out the documentation page. I'll create
one when extending the plugin. I've added two bug issues: for the gc-plugin and
for the plugin infrastructure.

Regarding the placement of clang_blink_gc_plugin_flags, it was moved [1] there
because putting it inside the WebKit directory required using a path that dotted
outside of the Blink repository.

I'll sit on this CL until I get an accept from the goma people regarding our
packaging of tar files.

[1] https://codereview.chromium.org/133473002/#msg3

[zerny-chromium, 2014-01-16 10:24:11]

Removed the revision packaging code so this CL only adds a stub implementation
for the plugin itself. The packaging code will follow in a subsequent CL once we
have decided what approach to use. PTAL

[Nico, 2014-01-16 22:44:30]

I still think the flag should probably be in blink. Dotting outside of that repo
is fine; the only way to build blink is with it embedded in a chromium checkout.

[Nico, 2014-01-16 22:45:05]

Other than that, slgtm

[Nico, 2014-01-17 20:35:49]

https://codereview.chromium.org/133463002/diff/140001/tools/clang/scripts/bli...
File tools/clang/scripts/blink_gc_plugin_flags.sh (right):

https://codereview.chromium.org/133463002/diff/140001/tools/clang/scripts/bli...
tools/clang/scripts/blink_gc_plugin_flags.sh:18: -Xclang -add-plugin -Xclang
blink-gc-plugin
You'll also have to add this to gn like in
https://codereview.chromium.org/142053002/ , now that gn is used for some
targets.

[zerny-chromium, 2014-01-20 08:19:45]

On 2014/01/17 20:35:49, Nico wrote:
>
https://codereview.chromium.org/133463002/diff/140001/tools/clang/scripts/bli...
> File tools/clang/scripts/blink_gc_plugin_flags.sh (right):
> 
>
https://codereview.chromium.org/133463002/diff/140001/tools/clang/scripts/bli...
> tools/clang/scripts/blink_gc_plugin_flags.sh:18: -Xclang -add-plugin -Xclang
> blink-gc-plugin
> You'll also have to add this to gn like in
> https://codereview.chromium.org/142053002/ , now that gn is used for some
> targets.

I'd like to wait with adding support in GN until it will be used. Right now the
blink gc plugin is behind a flag and default disabled, so the added setup to GN
would be unreachable atm.

I've reverted the previous gyp restructuring, so that the plugin-flags script is
called from the gyp setup in blink again
(https://codereview.chromium.org/133473002/).

[commit-bot: I haz the power, 2014-01-20 08:25:37]

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

[commit-bot: I haz the power, 2014-01-20 10:55:59]

Change committed as 245897

[hans, 2014-01-21 20:49:42]

On 2014/01/20 10:55:59, I haz the power (commit-bot) wrote:
> Change committed as 245897

Running package.sh this morning fails with this error:

+ cp tools/clang/scripts/../../../third_party/llvm-build/Release+Asserts/lib/lib
BlinkGCPlugin.so clang-199631/lib
cp: cannot stat `tools/clang/scripts/../../../third_party/llvm-build/Release+Ass
erts/lib/libBlinkGCPlugin.so': No such file or directory

Looking at this patch, I cannot see where the new plugin actually gets built. Is
that part missing? Here's an excerpt from my build log:

make: Leaving directory `/usr/local/google/work/chromium/src/third_party/llvm-bu
ild/tools/clang/runtime'
+ cd -
/work/chromium/src
+ for CHROME_TOOL_DIR in '${chrome_tools}'
+ TOOL_SRC_DIR=tools/clang/scripts/../plugins
+ TOOL_DST_DIR=tools/clang/scripts/../../../third_party/llvm/tools/clang/tools/c
hrome-plugins
+ TOOL_BUILD_DIR=tools/clang/scripts/../../../third_party/llvm/../llvm-build/too
ls/clang/tools/chrome-plugins
+ rm -rf tools/clang/scripts/../../../third_party/llvm/tools/clang/tools/chrome-
plugins
+ cp -R tools/clang/scripts/../plugins tools/clang/scripts/../../../third_party/
llvm/tools/clang/tools/chrome-plugins
+ rm -rf tools/clang/scripts/../../../third_party/llvm/../llvm-build/tools/clang
/tools/chrome-plugins
+ mkdir -p tools/clang/scripts/../../../third_party/llvm/../llvm-build/tools/cla
ng/tools/chrome-plugins
+ cp tools/clang/scripts/../plugins/Makefile tools/clang/scripts/../../../third_
party/llvm/../llvm-build/tools/clang/tools/chrome-plugins
+ MACOSX_DEPLOYMENT_TARGET=10.5
+ make -j6 -C tools/clang/scripts/../../../third_party/llvm/../llvm-build/tools/
clang/tools/chrome-plugins
make: Entering directory `/usr/local/google/work/chromium/src/third_party/llvm-b
uild/tools/clang/tools/chrome-plugins'
llvm[0]: Compiling FindBadConstructs.cpp for Release+Asserts build (PIC)
llvm[0]: Compiling ChromeClassTester.cpp for Release+Asserts build (PIC)
llvm[0]: Linking Release+Asserts Shared Library libFindBadConstructs.so
llvm[0]: Building Release+Asserts Archive Library libFindBadConstructs.a
make: Leaving directory `/usr/local/google/work/chromium/src/third_party/llvm-bu
ild/tools/clang/tools/chrome-plugins'
+ [[ -n yes ]]
+ for CHROME_TOOL_DIR in '${chrome_tools}'
+ TOOL_SRC_DIR=tools/clang/scripts/../plugins
+ [[ -f tools/clang/scripts/../plugins/tests/test.sh ]]
+ tools/clang/scripts/../plugins/tests/test.sh tools/clang/scripts/../../../thir
d_party/llvm/../llvm-build/Release+Asserts
Using clang directory /work/chromium/src/tools/clang/scripts/../../../third_part
y/llvm/../llvm-build/Release+Asserts...
PASS: base_refcounted.cpp
PASS: inline_copy_ctor.cpp
PASS: inline_ctor.cpp
PASS: missing_ctor.cpp
PASS: nested_class_inline_ctor.cpp
PASS: overridden_methods.cpp
PASS: virtual_methods.cpp
PASS: weak_ptr_factory.cpp
+ cd tools/clang/scripts/../../../third_party/llvm/../llvm-build
+ make check-all

[Mads Ager (chromium), 2014-01-21 20:52:47]

Hans, if this is blocking you, feel free to revert the change and we can
put it back in. :-)

Cheers,    -- Mads


On Tue, Jan 21, 2014 at 12:49 PM, <hans@chromium.org> wrote:

> On 2014/01/20 10:55:59, I haz the power (commit-bot) wrote:
>
>> Change committed as 245897
>>
>
> Running package.sh this morning fails with this error:
>
> + cp tools/clang/scripts/../../../third_party/llvm-build/
> Release+Asserts/lib/lib
> BlinkGCPlugin.so clang-199631/lib
> cp: cannot stat `tools/clang/scripts/../../../third_party/llvm-build/
> Release+Ass
> erts/lib/libBlinkGCPlugin.so': No such file or directory
>
> Looking at this patch, I cannot see where the new plugin actually gets
> built. Is
> that part missing? Here's an excerpt from my build log:
>
> make: Leaving directory `/usr/local/google/work/
> chromium/src/third_party/llvm-bu
> ild/tools/clang/runtime'
> + cd -
> /work/chromium/src
> + for CHROME_TOOL_DIR in '${chrome_tools}'
> + TOOL_SRC_DIR=tools/clang/scripts/../plugins
> + TOOL_DST_DIR=tools/clang/scripts/../../../third_party/
> llvm/tools/clang/tools/c
> hrome-plugins
> + TOOL_BUILD_DIR=tools/clang/scripts/../../../third_party/
> llvm/../llvm-build/too
> ls/clang/tools/chrome-plugins
> + rm -rf tools/clang/scripts/../../../third_party/llvm/tools/clang/
> tools/chrome-
> plugins
> + cp -R tools/clang/scripts/../plugins tools/clang/scripts/../../../
> third_party/
> llvm/tools/clang/tools/chrome-plugins
> + rm -rf tools/clang/scripts/../../../third_party/llvm/../llvm-
> build/tools/clang
> /tools/chrome-plugins
> + mkdir -p tools/clang/scripts/../../../third_party/llvm/../llvm-
> build/tools/cla
> ng/tools/chrome-plugins
> + cp tools/clang/scripts/../plugins/Makefile tools/clang/scripts/../../../
> third_
> party/llvm/../llvm-build/tools/clang/tools/chrome-plugins
> + MACOSX_DEPLOYMENT_TARGET=10.5
> + make -j6 -C tools/clang/scripts/../../../third_party/llvm/../llvm-
> build/tools/
> clang/tools/chrome-plugins
> make: Entering directory `/usr/local/google/work/
> chromium/src/third_party/llvm-b
> uild/tools/clang/tools/chrome-plugins'
> llvm[0]: Compiling FindBadConstructs.cpp for Release+Asserts build (PIC)
> llvm[0]: Compiling ChromeClassTester.cpp for Release+Asserts build (PIC)
> llvm[0]: Linking Release+Asserts Shared Library libFindBadConstructs.so
> llvm[0]: Building Release+Asserts Archive Library libFindBadConstructs.a
> make: Leaving directory `/usr/local/google/work/
> chromium/src/third_party/llvm-bu
> ild/tools/clang/tools/chrome-plugins'
> + [[ -n yes ]]
> + for CHROME_TOOL_DIR in '${chrome_tools}'
> + TOOL_SRC_DIR=tools/clang/scripts/../plugins
> + [[ -f tools/clang/scripts/../plugins/tests/test.sh ]]
> + tools/clang/scripts/../plugins/tests/test.sh
> tools/clang/scripts/../../../thir
> d_party/llvm/../llvm-build/Release+Asserts
> Using clang directory /work/chromium/src/tools/
> clang/scripts/../../../third_part
> y/llvm/../llvm-build/Release+Asserts...
> PASS: base_refcounted.cpp
> PASS: inline_copy_ctor.cpp
> PASS: inline_ctor.cpp
> PASS: missing_ctor.cpp
> PASS: nested_class_inline_ctor.cpp
> PASS: overridden_methods.cpp
> PASS: virtual_methods.cpp
> PASS: weak_ptr_factory.cpp
> + cd tools/clang/scripts/../../../third_party/llvm/../llvm-build
> + make check-all
>
> https://codereview.chromium.org/133463002/
>

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

[Nico, 2014-01-21 21:08:49]

https://codereview.chromium.org/133463002/diff/280001/tools/clang/scripts/upd...
File tools/clang/scripts/update.sh (right):

https://codereview.chromium.org/133463002/diff/280001/tools/clang/scripts/upd...
tools/clang/scripts/update.sh:308: cp "${TOOL_SRC_DIR}/Makefile"
"${TOOL_BUILD_DIR}"
I thought it would get build here. Maybe the new plugin is missing a makefile.

[Nico, 2014-01-21 21:09:55]

On 2014/01/21 21:08:49, Nico wrote:
>
https://codereview.chromium.org/133463002/diff/280001/tools/clang/scripts/upd...
> File tools/clang/scripts/update.sh (right):
> 
>
https://codereview.chromium.org/133463002/diff/280001/tools/clang/scripts/upd...
> tools/clang/scripts/update.sh:308: cp "${TOOL_SRC_DIR}/Makefile"
> "${TOOL_BUILD_DIR}"
> I thought it would get build here. Maybe the new plugin is missing a makefile.

No, looks like tools/clang/plugins/Makefile needs to reference the new Makefile
somehow.

Maybe just revert the update.sh bits.

[hans, 2014-01-21 21:22:12]

https://codereview.chromium.org/133463002/diff/280001/tools/clang/scripts/upd...
File tools/clang/scripts/update.sh (right):

https://codereview.chromium.org/133463002/diff/280001/tools/clang/scripts/upd...
tools/clang/scripts/update.sh:300: for CHROME_TOOL_DIR in ${chrome_tools}; do
It looks like this for loop only runs one iteration.

I guess the new thing only gets built if one passes "--chrome-tools=plugins
blink_gc_plugin", and I obviously didn't pass that.

[zerny-chromium, 2014-01-22 00:42:47]

Yes. The plugin needs to be added to the chrome-tools flag. I'll add it to
the default flags if there are no objections.
On Jan 21, 2014 10:30 PM, <hans@chromium.org> wrote:

>
> https://codereview.chromium.org/133463002/diff/280001/
> tools/clang/scripts/update.sh
> File tools/clang/scripts/update.sh (right):
>
> https://codereview.chromium.org/133463002/diff/280001/
> tools/clang/scripts/update.sh#newcode300
> tools/clang/scripts/update.sh:300: for CHROME_TOOL_DIR in
> ${chrome_tools}; do
> It looks like this for loop only runs one iteration.
>
> I guess the new thing only gets built if one passes
> "--chrome-tools=plugins blink_gc_plugin", and I obviously didn't pass
> that.
>
> https://codereview.chromium.org/133463002/
>

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