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

Issue 619149 link

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



Sign in to add a comment

Improve Blink GC plugin error reporting when tracing illegal fields

Reported by sigbjo...@opera.com, Jun 10 2016 
The clang Blink GC plugin detects if trace() methods are incomplete, failing to trace one or more fields that are "traceable" (heap references or part objects that contain heap references.) It does not report errors if tracing is over fields that aren't traceable, leaving that for compilation to otherwise fail.

We should try to improve on this; e.g., better, more succinct, error messages when attempting to trace smart pointers wrapping up non-heap objects are preferable to verbose static_assert()-generated error messages and other long compiler error backtraces. The GC plugin should instead detect & report trace() calls over fields that do not support tracing.

Comment 1 by sigbjo...@opera.com, Jun 10 2016

Owner: sigbjo...@opera.com
Status: Started (was: Untriaged)
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 15 2016 

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

commit 3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8
Author: sigbjornf <sigbjornf@opera.com>
Date: Wed Jun 15 08:35:48 2016

GC plugin: improve error reporting when tracing illegal fields.

Add detection of trace() calls over smart pointer types that either do not
wrap up references to heap objects, or are otherwise not meant to be traced
over. In particular, CrossThread(Weak)Persistent<T> fields are now detected
as being illegal to trace over. Also consider OwnPtr<T>, RefPtr<T> and
std::unique_ptr<T> as illegal to trace over & emit a more concise error
messages for these.

R=
BUG= 619149 

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

[modify] https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8/tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp
[modify] https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8/tools/clang/blink_gc_plugin/CheckFieldsVisitor.cpp
[modify] https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8/tools/clang/blink_gc_plugin/CheckFieldsVisitor.h
[modify] https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8/tools/clang/blink_gc_plugin/Config.h
[modify] https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8/tools/clang/blink_gc_plugin/DiagnosticsReporter.cpp
[modify] https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8/tools/clang/blink_gc_plugin/DiagnosticsReporter.h
[modify] https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8/tools/clang/blink_gc_plugin/Edge.cpp
[modify] https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8/tools/clang/blink_gc_plugin/Edge.h
[modify] https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8/tools/clang/blink_gc_plugin/RecordInfo.cpp
[modify] https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8/tools/clang/blink_gc_plugin/RecordInfo.h
[modify] https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8/tools/clang/blink_gc_plugin/TracingStatus.h
[add] https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8/tools/clang/blink_gc_plugin/tests/fields_illegal_tracing.cpp
[add] https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8/tools/clang/blink_gc_plugin/tests/fields_illegal_tracing.h
[add] https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8/tools/clang/blink_gc_plugin/tests/fields_illegal_tracing.txt
[modify] https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8/tools/clang/blink_gc_plugin/tests/heap/stubs.h
[modify] https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8/tools/clang/blink_gc_plugin/tests/persistent_field_in_gc_managed_class.h
[modify] https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8/tools/clang/blink_gc_plugin/tests/persistent_field_in_gc_managed_class.txt
[add] https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8/tools/clang/blink_gc_plugin/tests/persistent_no_trace.cpp
[add] https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8/tools/clang/blink_gc_plugin/tests/persistent_no_trace.h
[add] https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8/tools/clang/blink_gc_plugin/tests/persistent_no_trace.txt
[modify] https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8/tools/clang/blink_gc_plugin/tests/templated_class_with_local_class_requires_trace.txt
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 15 2016 

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

commit fd0eaf988b6b216d51e3eb9dde44b4df4f42c3a3
Author: thakis <thakis@chromium.org>
Date: Wed Jun 15 09:38:52 2016

Revert of GC plugin: improve error reporting when tracing illegal fields. (patchset #2 id:20001 of https://codereview.chromium.org/2060553002/ )

Reason for revert:
New test fails: https://build.chromium.org/p/chromium.fyi/builders/ClangToTMac%20%28dbg%29/builds/5481/steps/gclient%20runhooks/logs/stdio

Original issue's description:
> GC plugin: improve error reporting when tracing illegal fields.
>
> Add detection of trace() calls over smart pointer types that either do not
> wrap up references to heap objects, or are otherwise not meant to be traced
> over. In particular, CrossThread(Weak)Persistent<T> fields are now detected
> as being illegal to trace over. Also consider OwnPtr<T>, RefPtr<T> and
> std::unique_ptr<T> as illegal to trace over & emit a more concise error
> messages for these.
>
> R=
> BUG= 619149 
>
> Committed: https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8
> Cr-Commit-Position: refs/heads/master@{#399861}

TBR=oilpan-reviews@chromium.org,sigbjornf@opera.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 619149 

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

[modify] https://crrev.com/fd0eaf988b6b216d51e3eb9dde44b4df4f42c3a3/tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp
[modify] https://crrev.com/fd0eaf988b6b216d51e3eb9dde44b4df4f42c3a3/tools/clang/blink_gc_plugin/CheckFieldsVisitor.cpp
[modify] https://crrev.com/fd0eaf988b6b216d51e3eb9dde44b4df4f42c3a3/tools/clang/blink_gc_plugin/CheckFieldsVisitor.h
[modify] https://crrev.com/fd0eaf988b6b216d51e3eb9dde44b4df4f42c3a3/tools/clang/blink_gc_plugin/Config.h
[modify] https://crrev.com/fd0eaf988b6b216d51e3eb9dde44b4df4f42c3a3/tools/clang/blink_gc_plugin/DiagnosticsReporter.cpp
[modify] https://crrev.com/fd0eaf988b6b216d51e3eb9dde44b4df4f42c3a3/tools/clang/blink_gc_plugin/DiagnosticsReporter.h
[modify] https://crrev.com/fd0eaf988b6b216d51e3eb9dde44b4df4f42c3a3/tools/clang/blink_gc_plugin/Edge.cpp
[modify] https://crrev.com/fd0eaf988b6b216d51e3eb9dde44b4df4f42c3a3/tools/clang/blink_gc_plugin/Edge.h
[modify] https://crrev.com/fd0eaf988b6b216d51e3eb9dde44b4df4f42c3a3/tools/clang/blink_gc_plugin/RecordInfo.cpp
[modify] https://crrev.com/fd0eaf988b6b216d51e3eb9dde44b4df4f42c3a3/tools/clang/blink_gc_plugin/RecordInfo.h
[modify] https://crrev.com/fd0eaf988b6b216d51e3eb9dde44b4df4f42c3a3/tools/clang/blink_gc_plugin/TracingStatus.h
[delete] https://crrev.com/24bb735828d354a357bbf30cb70d4d5ec41cb939/tools/clang/blink_gc_plugin/tests/fields_illegal_tracing.cpp
[delete] https://crrev.com/24bb735828d354a357bbf30cb70d4d5ec41cb939/tools/clang/blink_gc_plugin/tests/fields_illegal_tracing.h
[delete] https://crrev.com/24bb735828d354a357bbf30cb70d4d5ec41cb939/tools/clang/blink_gc_plugin/tests/fields_illegal_tracing.txt
[modify] https://crrev.com/fd0eaf988b6b216d51e3eb9dde44b4df4f42c3a3/tools/clang/blink_gc_plugin/tests/heap/stubs.h
[modify] https://crrev.com/fd0eaf988b6b216d51e3eb9dde44b4df4f42c3a3/tools/clang/blink_gc_plugin/tests/persistent_field_in_gc_managed_class.h
[modify] https://crrev.com/fd0eaf988b6b216d51e3eb9dde44b4df4f42c3a3/tools/clang/blink_gc_plugin/tests/persistent_field_in_gc_managed_class.txt
[delete] https://crrev.com/24bb735828d354a357bbf30cb70d4d5ec41cb939/tools/clang/blink_gc_plugin/tests/persistent_no_trace.cpp
[delete] https://crrev.com/24bb735828d354a357bbf30cb70d4d5ec41cb939/tools/clang/blink_gc_plugin/tests/persistent_no_trace.h
[delete] https://crrev.com/24bb735828d354a357bbf30cb70d4d5ec41cb939/tools/clang/blink_gc_plugin/tests/persistent_no_trace.txt
[modify] https://crrev.com/fd0eaf988b6b216d51e3eb9dde44b4df4f42c3a3/tools/clang/blink_gc_plugin/tests/templated_class_with_local_class_requires_trace.txt
Project Member

Comment 4 by bugdroid1@chromium.org, Jun 20 2016 

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

commit 163954bcdf26d970e2166ec2dabadff3c71fd1c8
Author: sigbjornf <sigbjornf@opera.com>
Date: Mon Jun 20 11:24:57 2016

GC plugin: improve error reporting when tracing illegal fields.

Add detection of trace() calls over smart pointer types that either do not
wrap up references to heap objects, or are otherwise not meant to be traced
over. In particular, CrossThread(Weak)Persistent<T> fields are now detected
as being illegal to trace over. Also consider OwnPtr<T>, RefPtr<T> and
std::unique_ptr<T> as illegal to trace over & emit a more concise error
messages for these.

R=
BUG= 619149 

Committed: https://crrev.com/3ba6089cd6a901b62ff5a0d8f08a2bd818edcbe8
Review-Url: https://codereview.chromium.org/2060553002
Cr-Original-Commit-Position: refs/heads/master@{#399861}
Cr-Commit-Position: refs/heads/master@{#400653}

[modify] https://crrev.com/163954bcdf26d970e2166ec2dabadff3c71fd1c8/tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp
[modify] https://crrev.com/163954bcdf26d970e2166ec2dabadff3c71fd1c8/tools/clang/blink_gc_plugin/CheckFieldsVisitor.cpp
[modify] https://crrev.com/163954bcdf26d970e2166ec2dabadff3c71fd1c8/tools/clang/blink_gc_plugin/CheckFieldsVisitor.h
[modify] https://crrev.com/163954bcdf26d970e2166ec2dabadff3c71fd1c8/tools/clang/blink_gc_plugin/Config.h
[modify] https://crrev.com/163954bcdf26d970e2166ec2dabadff3c71fd1c8/tools/clang/blink_gc_plugin/DiagnosticsReporter.cpp
[modify] https://crrev.com/163954bcdf26d970e2166ec2dabadff3c71fd1c8/tools/clang/blink_gc_plugin/DiagnosticsReporter.h
[modify] https://crrev.com/163954bcdf26d970e2166ec2dabadff3c71fd1c8/tools/clang/blink_gc_plugin/Edge.cpp
[modify] https://crrev.com/163954bcdf26d970e2166ec2dabadff3c71fd1c8/tools/clang/blink_gc_plugin/Edge.h
[modify] https://crrev.com/163954bcdf26d970e2166ec2dabadff3c71fd1c8/tools/clang/blink_gc_plugin/RecordInfo.cpp
[modify] https://crrev.com/163954bcdf26d970e2166ec2dabadff3c71fd1c8/tools/clang/blink_gc_plugin/RecordInfo.h
[modify] https://crrev.com/163954bcdf26d970e2166ec2dabadff3c71fd1c8/tools/clang/blink_gc_plugin/TracingStatus.h
[add] https://crrev.com/163954bcdf26d970e2166ec2dabadff3c71fd1c8/tools/clang/blink_gc_plugin/tests/fields_illegal_tracing.cpp
[add] https://crrev.com/163954bcdf26d970e2166ec2dabadff3c71fd1c8/tools/clang/blink_gc_plugin/tests/fields_illegal_tracing.h
[add] https://crrev.com/163954bcdf26d970e2166ec2dabadff3c71fd1c8/tools/clang/blink_gc_plugin/tests/fields_illegal_tracing.txt
[modify] https://crrev.com/163954bcdf26d970e2166ec2dabadff3c71fd1c8/tools/clang/blink_gc_plugin/tests/heap/stubs.h
[modify] https://crrev.com/163954bcdf26d970e2166ec2dabadff3c71fd1c8/tools/clang/blink_gc_plugin/tests/persistent_field_in_gc_managed_class.h
[modify] https://crrev.com/163954bcdf26d970e2166ec2dabadff3c71fd1c8/tools/clang/blink_gc_plugin/tests/persistent_field_in_gc_managed_class.txt
[add] https://crrev.com/163954bcdf26d970e2166ec2dabadff3c71fd1c8/tools/clang/blink_gc_plugin/tests/persistent_no_trace.cpp
[add] https://crrev.com/163954bcdf26d970e2166ec2dabadff3c71fd1c8/tools/clang/blink_gc_plugin/tests/persistent_no_trace.h
[add] https://crrev.com/163954bcdf26d970e2166ec2dabadff3c71fd1c8/tools/clang/blink_gc_plugin/tests/persistent_no_trace.txt
[modify] https://crrev.com/163954bcdf26d970e2166ec2dabadff3c71fd1c8/tools/clang/blink_gc_plugin/tests/templated_class_with_local_class_requires_trace.txt

Comment 5 by sigbjo...@opera.com, Jan 2 2017

Status: Fixed (was: Started)
►

Sign in to add a comment