New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 689874 link

Starred by 2 users
Status: Fixed
Owner:
█ sigbjo...@opera.com
Email to this user bounced
Closed: Mar 2017
Components:
EstimatedDays: 0
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 685244


Sign in to add a comment

GC plugin should warn if stack allocated classes define a trace method

Reported by sigbjo...@opera.com, Feb 8 2017 
Classes annotated with STACK_ALLOCATED() disables |operator new| and can only be stack allocated.

For these, the required practice of having to define a |trace| method (if they contain Blink GC reference fields), doesn't apply as any conservative GC that might strike while an instance is stack allocated will be handled as wanted by the marking phase.

Consequently, we should make the clang Blink GC plugin detect the declaration of these unnecessary and unused trace methods for STACK_ALLOCATED() class types, and issue a warning/error.

Comment 1 by sigbjo...@opera.com, Feb 8 2017

Blocking: 685244

Comment 2 by thakis@chromium.org, Feb 8 2017

Blockedon: 685244
Blocking: -685244
the other way round, right?

Comment 3 by sigbjo...@opera.com, Feb 8 2017 

as long as both land safely, i'm content :)

(i added it in that direction to indicate that the roll should preferably not go out without this addition.)
Project Member

Comment 4 by bugdroid1@chromium.org, Feb 8 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/59dc1fe4df1db90553f572d20cf2b732438063b2

commit 59dc1fe4df1db90553f572d20cf2b732438063b2
Author: sigbjornf <sigbjornf@opera.com>
Date: Wed Feb 08 18:49:43 2017

blink_gc_plugin: warn of unused trace methods to stack allocated classes.

A STACK_ALLOCATED()-annotated class does not need a trace method; issue
a warning if encountered.

R=haraken
BUG= 689874 

Review-Url: https://codereview.chromium.org/2685583002
Cr-Commit-Position: refs/heads/master@{#449046}

[modify] https://crrev.com/59dc1fe4df1db90553f572d20cf2b732438063b2/tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp
[modify] https://crrev.com/59dc1fe4df1db90553f572d20cf2b732438063b2/tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp
[modify] https://crrev.com/59dc1fe4df1db90553f572d20cf2b732438063b2/tools/clang/blink_gc_plugin/BlinkGCPluginOptions.h
[modify] https://crrev.com/59dc1fe4df1db90553f572d20cf2b732438063b2/tools/clang/blink_gc_plugin/DiagnosticsReporter.cpp
[modify] https://crrev.com/59dc1fe4df1db90553f572d20cf2b732438063b2/tools/clang/blink_gc_plugin/DiagnosticsReporter.h
[modify] https://crrev.com/59dc1fe4df1db90553f572d20cf2b732438063b2/tools/clang/blink_gc_plugin/tests/.gitignore
[modify] https://crrev.com/59dc1fe4df1db90553f572d20cf2b732438063b2/tools/clang/blink_gc_plugin/tests/legacy_naming/.gitignore
[add] https://crrev.com/59dc1fe4df1db90553f572d20cf2b732438063b2/tools/clang/blink_gc_plugin/tests/legacy_naming/stack_allocated.flags
[modify] https://crrev.com/59dc1fe4df1db90553f572d20cf2b732438063b2/tools/clang/blink_gc_plugin/tests/legacy_naming/stack_allocated.h
[modify] https://crrev.com/59dc1fe4df1db90553f572d20cf2b732438063b2/tools/clang/blink_gc_plugin/tests/legacy_naming/stack_allocated.txt
[add] https://crrev.com/59dc1fe4df1db90553f572d20cf2b732438063b2/tools/clang/blink_gc_plugin/tests/legacy_naming/stack_allocated_no_warning.cpp
[add] https://crrev.com/59dc1fe4df1db90553f572d20cf2b732438063b2/tools/clang/blink_gc_plugin/tests/legacy_naming/stack_allocated_no_warning.h
[add] https://crrev.com/59dc1fe4df1db90553f572d20cf2b732438063b2/tools/clang/blink_gc_plugin/tests/legacy_naming/stack_allocated_no_warning.txt
[add] https://crrev.com/59dc1fe4df1db90553f572d20cf2b732438063b2/tools/clang/blink_gc_plugin/tests/stack_allocated.flags
[modify] https://crrev.com/59dc1fe4df1db90553f572d20cf2b732438063b2/tools/clang/blink_gc_plugin/tests/stack_allocated.h
[modify] https://crrev.com/59dc1fe4df1db90553f572d20cf2b732438063b2/tools/clang/blink_gc_plugin/tests/stack_allocated.txt
[add] https://crrev.com/59dc1fe4df1db90553f572d20cf2b732438063b2/tools/clang/blink_gc_plugin/tests/stack_allocated_no_warning.cpp
[add] https://crrev.com/59dc1fe4df1db90553f572d20cf2b732438063b2/tools/clang/blink_gc_plugin/tests/stack_allocated_no_warning.h
[add] https://crrev.com/59dc1fe4df1db90553f572d20cf2b732438063b2/tools/clang/blink_gc_plugin/tests/stack_allocated_no_warning.txt

Comment 5 by sigbjo...@opera.com, Feb 8 2017 

(With the dependency set up this way, I'll keep this open for the later, follow-on changes to always-enable #4.)

Comment 6 by thakis@chromium.org, Mar 1 2017 

roll landed
Project Member

Comment 7 by bugdroid1@chromium.org, Mar 2 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b140095b5c1280913696fd1b98070171b8109de1

commit b140095b5c1280913696fd1b98070171b8109de1
Author: sigbjornf <sigbjornf@opera.com>
Date: Thu Mar 02 18:09:01 2017

Enable Blink GC plugin check for stack allocated classes.

The latest clang roll ( crbug.com/685244 ) included a GC plugin with
support for checking STACK_ALLOCATED() classes w/ unwanted trace
method definitions. Add support for that check to the build system,
unconditionally enabled.

R=haraken,thakis
BUG= 689874 

Review-Url: https://codereview.chromium.org/2724353002
Cr-Commit-Position: refs/heads/master@{#454306}

[modify] https://crrev.com/b140095b5c1280913696fd1b98070171b8109de1/third_party/WebKit/Source/BUILD.gn
Project Member

Comment 8 by bugdroid1@chromium.org, Mar 26 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/67512fae29e6cdb8ef5ffaee2fa42ccef4902b34

commit 67512fae29e6cdb8ef5ffaee2fa42ccef4902b34
Author: sigbjornf <sigbjornf@opera.com>
Date: Sun Mar 26 16:48:05 2017

Remove no-op blink-gc-plugin argument.

Following the clang roll in r455977, the clang blink_gc_plugin always
warns of STACK_ALLOCATED() classes having redundant trace() methods;
drop using the gc-plugin no-op option.

R=
BUG= 689874 

Review-Url: https://codereview.chromium.org/2776023002
Cr-Commit-Position: refs/heads/master@{#459676}

[modify] https://crrev.com/67512fae29e6cdb8ef5ffaee2fa42ccef4902b34/third_party/WebKit/Source/BUILD.gn
Project Member

Comment 9 by bugdroid1@chromium.org, Mar 26 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5388a131f6f63314ef21f730f3e6151c6ff8db56

commit 5388a131f6f63314ef21f730f3e6151c6ff8db56
Author: sigbjornf <sigbjornf@opera.com>
Date: Sun Mar 26 17:37:39 2017

blink_gc_plugin: retire warn-stack-allocated-trace-method option.

As a final(!) step in phasing in the check over trace() methods inside
of STACK_ALLOCATED() classes, remove the detection of the warning option.
It's always on.

R=
BUG= 689874 

Review-Url: https://codereview.chromium.org/2776033002
Cr-Commit-Position: refs/heads/master@{#459677}

[modify] https://crrev.com/5388a131f6f63314ef21f730f3e6151c6ff8db56/tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp
[delete] https://crrev.com/67512fae29e6cdb8ef5ffaee2fa42ccef4902b34/tools/clang/blink_gc_plugin/tests/legacy_naming/stack_allocated.flags
[delete] https://crrev.com/67512fae29e6cdb8ef5ffaee2fa42ccef4902b34/tools/clang/blink_gc_plugin/tests/stack_allocated.flags

Comment 10 by sigbjo...@opera.com, Mar 26 2017

EstimatedDays: 0
Status: Fixed (was: Started)
►

Sign in to add a comment