Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(64)

Issue 1274403002: GC plugin: consider references equal to raw pointers. (Closed)

Created:
5 years, 4 months ago by sof
Modified:
5 years, 4 months ago
Reviewers:
oilpan-reviews, haraken
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

GC plugin: consider references equal to raw pointers. Unmanaged raw pointers to managed(GC) classes aren't safe across GCs, hence fields with raw pointer types to such GCed classes are identified and reported as errors or warnings. Extend that checking to also apply to reference pointer types. R=haraken BUG=420515 Committed: https://crrev.com/1a33d92721acdfc37d71fd891eade8fbddd6c7dd Cr-Commit-Position: refs/heads/master@{#342615}

Patch Set 1 #

Patch Set 2 : Propagate reference type into error/warning messages #

Unified diffs Side-by-side diffs Delta from patch set Stats (+74 lines, -16 lines) Patch
M tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp View 1 9 chunks +47 lines, -8 lines 0 comments Download
M tools/clang/blink_gc_plugin/Edge.h View 1 2 chunks +11 lines, -2 lines 0 comments Download
M tools/clang/blink_gc_plugin/RecordInfo.cpp View 1 2 chunks +3 lines, -3 lines 0 comments Download
M tools/clang/blink_gc_plugin/tests/raw_ptr_to_gc_managed_class.h View 1 chunk +4 lines, -0 lines 0 comments Download
M tools/clang/blink_gc_plugin/tests/raw_ptr_to_gc_managed_class.txt View 1 1 chunk +9 lines, -3 lines 0 comments Download

Messages

Total messages: 16 (3 generated)
sof
please take a look. Will separately follow up on the build errors that this triggers ...
5 years, 4 months ago (2015-08-07 07:05:31 UTC) #2
haraken
Thanks, LGTM
5 years, 4 months ago (2015-08-07 07:11:25 UTC) #3
sof
Thanks, extended it a bit so as to propagate the notion of reference type into ...
5 years, 4 months ago (2015-08-07 07:50:45 UTC) #4
haraken
Still LGTM
5 years, 4 months ago (2015-08-07 08:08:51 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1274403002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1274403002/20001
5 years, 4 months ago (2015-08-07 08:24:15 UTC) #7
commit-bot: I haz the power
Committed patchset #2 (id:20001)
5 years, 4 months ago (2015-08-07 08:28:03 UTC) #8
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/70fe23896f81cd66ba1a205b0336f91c64e3fa4e Cr-Commit-Position: refs/heads/master@{#342320}
5 years, 4 months ago (2015-08-07 08:28:40 UTC) #9
Nico
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1275383002/ by thakis@chromium.org. ...
5 years, 4 months ago (2015-08-07 16:45:25 UTC) #10
sof
With Blink r200242 in place, relanding.
5 years, 4 months ago (2015-08-10 14:18:25 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1274403002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1274403002/20001
5 years, 4 months ago (2015-08-10 14:18:50 UTC) #13
commit-bot: I haz the power
Committed patchset #2 (id:20001)
5 years, 4 months ago (2015-08-10 14:37:39 UTC) #14
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/1a33d92721acdfc37d71fd891eade8fbddd6c7dd Cr-Commit-Position: refs/heads/master@{#342615}
5 years, 4 months ago (2015-08-10 14:38:15 UTC) #15
Nico
5 years, 4 months ago (2015-08-10 16:58:45 UTC) #16
Message was sent while issue was closed.
Still doesn't build:
http://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dll%29/builds...

..\..\third_party\WebKit\Source\core/css/parser/CSSParserObserverWrapper.h(12,1)
:  error: [blink-gc] Class 'CSSParserObserverWrapper' contains invalid fields.
class CSSParserObserverWrapper {
^
..\..\third_party\WebKit\Source\core/css/parser/CSSParserObserverWrapper.h(39,5)
:  note: [blink-gc] Stack-allocated field 'm_observer' declared here:
    CSSParserObserver& m_observer;
    ^

Maybe you didn't wait for that blink change to roll in? I'll wait another build
before reverting, but if this was the case please only land plugin changes
without them being behind a flag if the codebase is known to build with the
plugin change and if it's very unlikely that the codebase will regress until the
next clang roll. Otherwise, plugin changes should always be behind flags.

Powered by Google App Engine
This is Rietveld 408576698