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

Issue 968143002: BlinkGCPlugin: Fix detection of trace dispatching after inlined tracing. (Closed)

Created:
5 years, 9 months ago by Yuta Kitamura
Modified:
5 years, 9 months ago
Reviewers:
kouhei (in TOK)
CC:
chromium-reviews, zerny-chromium, oilpan-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

BlinkGCPlugin: Fix detection of trace dispatching after inlined tracing. This patch actually fixes three different issues, as I could not separate the updated tests in a meaningful way. 1. CheckDispatchVisitor needs to look for UnresolvedMemberExpr*, since inlined tracing introduces unresolved member references. This visitor looks at every function call within a trace dispatching method, and returns whether there is a call delegating to the class in question. We need to take care of UnresolvedMemberExpr* cases in addition to MemberExpr*. 2. RecordInfo::DetermineTracingMethods() should return traceImpl() instead of trace() as trace_dispatch_method_. The plugin inspects the definition of trace_dispatch_method_ in order to detect missing or inappropriate delegation to subclasses. Therefore, the method should be something that has actual delegation calls in its body (i.e. traceImpl()) instead of a trampoline stub (i.e. trace()). 3. A silly typo in Config::GetTraceMethodType(). Really silly. Not sure why I did this way. This patch comes with updated tests for traceAfterDispatchImpl(). Now the tests are more closer to real cases, using traceImpl to implement trace dispatching. BUG=462511 R=kouhei@chromium.org CC=zerny@chromium.org, oilpan-reviews@chromium.org Committed: https://crrev.com/8747aee1f18410d8eaf0e2d98724c8e24477bdc2 Cr-Commit-Position: refs/heads/master@{#318673}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+144 lines, -36 lines) Patch
M tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp View 2 chunks +16 lines, -1 line 0 comments Download
M tools/clang/blink_gc_plugin/Config.h View 1 chunk +1 line, -1 line 0 comments Download
M tools/clang/blink_gc_plugin/RecordInfo.cpp View 1 chunk +22 lines, -11 lines 0 comments Download
M tools/clang/blink_gc_plugin/tests/trace_after_dispatch_impl.h View 4 chunks +17 lines, -1 line 0 comments Download
M tools/clang/blink_gc_plugin/tests/trace_after_dispatch_impl.cpp View 4 chunks +24 lines, -3 lines 0 comments Download
M tools/clang/blink_gc_plugin/tests/trace_after_dispatch_impl_error.h View 4 chunks +17 lines, -1 line 0 comments Download
M tools/clang/blink_gc_plugin/tests/trace_after_dispatch_impl_error.cpp View 3 chunks +30 lines, -7 lines 0 comments Download
M tools/clang/blink_gc_plugin/tests/trace_after_dispatch_impl_error.txt View 1 chunk +17 lines, -11 lines 0 comments Download

Messages

Total messages: 6 (1 generated)
Yuta Kitamura
Note: this is the last change needed to fix all known inlined-trace-related issues.
5 years, 9 months ago (2015-03-02 09:15:43 UTC) #1
kouhei (in TOK)
lgtm. Thanks a lot!
5 years, 9 months ago (2015-03-02 09:26:05 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/968143002/1
5 years, 9 months ago (2015-03-02 09:26:50 UTC) #4
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years, 9 months ago (2015-03-02 09:59:24 UTC) #5
commit-bot: I haz the power
5 years, 9 months ago (2015-03-02 10:00:15 UTC) #6
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/8747aee1f18410d8eaf0e2d98724c8e24477bdc2
Cr-Commit-Position: refs/heads/master@{#318673}

Powered by Google App Engine
This is Rietveld 408576698