Oilpan: Added gyp flag to dump points-to graph information from the Blink GC plugin.

Oilpan: Added gyp flag to dump points-to graph information from the Blink GC plugin.

Enabling the blink_gc_plugin_dump_graph flag will cause the Blink GC plugin for clang to generate files with points-to information for each compilation unit. The output files can be used to statically analyze the complete points-to graph for cycles and other correctness criteria.

R=ager@chromium.org
BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170142

[zerny-chromium, 2014-03-21 09:51:13]

Plugin side CL: https://codereview.chromium.org/207913002/

[haraken, 2014-03-21 10:31:40]

https://codereview.chromium.org/207393003/diff/1/Source/core/xml/XSLImportRule.h
File Source/core/xml/XSLImportRule.h (right):

https://codereview.chromium.org/207393003/diff/1/Source/core/xml/XSLImportRul...
Source/core/xml/XSLImportRule.h:69: GC_PLUGIN_IGNORE_CYCLE

It's a bit inconsistent that we have a bug number on GC_PLUGIN_IGNORE but we
have a FIXME on GC_PLUGIN_IGNORE_CYCLE. (I'd prefer a FIXME though :-)

https://codereview.chromium.org/207393003/diff/1/Source/modules/webdatabase/I...
File Source/modules/webdatabase/InspectorDatabaseAgent.h (right):

https://codereview.chromium.org/207393003/diff/1/Source/modules/webdatabase/I...
Source/modules/webdatabase/InspectorDatabaseAgent.h:79: GC_PLUGIN_IGNORE_CYCLE

Do we need to add this? I mean, isn't it enough to add the macro to only
persistent handles that cause the leak? We want to limit the place where
mysterious macros are added to the code base.

[zerny-chromium, 2014-03-21 10:46:18]

Thanks for the review Haraken.

https://codereview.chromium.org/207393003/diff/1/Source/core/xml/XSLImportRule.h
File Source/core/xml/XSLImportRule.h (right):

https://codereview.chromium.org/207393003/diff/1/Source/core/xml/XSLImportRul...
Source/core/xml/XSLImportRule.h:69: GC_PLUGIN_IGNORE_CYCLE
On 2014/03/21 10:31:40, haraken wrote:
> 
> It's a bit inconsistent that we have a bug number on GC_PLUGIN_IGNORE but we
> have a FIXME on GC_PLUGIN_IGNORE_CYCLE. (I'd prefer a FIXME though :-)

This only ignores the field with respect to the cycle detection code, so it has
limited impact (It does not signal a code issue or a plugin bug, just silences a
false positive). The other ignore is much more serious as simply acts as if the
field does not exist. Hopefully the use of GC_PLUGIN_IGNORE will be more
uncommon and the issue quickly resolved.

https://codereview.chromium.org/207393003/diff/1/Source/modules/webdatabase/I...
File Source/modules/webdatabase/InspectorDatabaseAgent.h (right):

https://codereview.chromium.org/207393003/diff/1/Source/modules/webdatabase/I...
Source/modules/webdatabase/InspectorDatabaseAgent.h:79: GC_PLUGIN_IGNORE_CYCLE
> Do we need to add this? I mean, isn't it enough to add the macro to only
> persistent handles that cause the leak? We want to limit the place where
> mysterious macros are added to the code base.

This is the persistent handle that causes the leak. We can't add it to the
typedef because that is not the field of an object.

[zerny-chromium, 2014-03-21 10:47:25]

> Source/modules/webdatabase/InspectorDatabaseAgent.h:79: GC_PLUGIN_IGNORE_CYCLE
> > Do we need to add this? I mean, isn't it enough to add the macro to only
> > persistent handles that cause the leak? We want to limit the place where
> > mysterious macros are added to the code base.
> 
> This is the persistent handle that causes the leak. We can't add it to the
> typedef because that is not the field of an object.

That should be "that causes the cycle" which is not an actual leak.

[haraken, 2014-03-21 11:07:48]

Makes sense, LGTM.

On 2014/03/21 10:47:25, zerny-chromium wrote:
> > Source/modules/webdatabase/InspectorDatabaseAgent.h:79:
GC_PLUGIN_IGNORE_CYCLE
> > > Do we need to add this? I mean, isn't it enough to add the macro to only
> > > persistent handles that cause the leak? We want to limit the place where
> > > mysterious macros are added to the code base.
> > 
> > This is the persistent handle that causes the leak. We can't add it to the
> > typedef because that is not the field of an object.
> 
> That should be "that causes the cycle" which is not an actual leak.

Shall we add a FIXME to all GC_PLUGIN_IGNORE_CYCLE about how it causes a cycle?
(I guess that GC_PLUGIN_IGNORE_CYCLE won't be common so the FIXME won't be a
mess.) The comment is helpful for us to understand the cycle and also helpful
for other developers to understand what the mysterious macro means.

[Mads Ager (chromium), 2014-03-21 11:34:34]

LGTM

[zerny-chromium, 2014-03-21 11:46:43]

Adam could you do the owners review?

[abarth-chromium, 2014-03-21 13:06:16]

This seems difficult to use.  When the cycle detector fire, how will we know
whether it's a false positive?

[zerny-chromium, 2014-03-21 13:13:01]

On 2014/03/21 13:06:16, abarth wrote:
> This seems difficult to use.  When the cycle detector fire, how will we know
> whether it's a false positive?

The cycle detector is a debugging tool meant to be manually run following a full
compilation where special graph files have been computed. It will not become
default enabled as part of standard builds and programmers are not expected to
run it. It would be nice to enable it on a buildbot at some point, but for now
we will be running it offline from time to time to keep track of potential leaks
in the transition period.

So in short, it will only fire on us, and we will then manually inspect if it is
a false positive or not.

[zerny-chromium, 2014-03-24 12:15:31]

On 2014/03/21 13:13:01, zerny-chromium wrote:
> On 2014/03/21 13:06:16, abarth wrote:
> > This seems difficult to use.  When the cycle detector fire, how will we know
> > whether it's a false positive?
> 
> The cycle detector is a debugging tool meant to be manually run following a
full
> compilation where special graph files have been computed. It will not become
> default enabled as part of standard builds and programmers are not expected to
> run it. It would be nice to enable it on a buildbot at some point, but for now
> we will be running it offline from time to time to keep track of potential
leaks
> in the transition period.
> 
> So in short, it will only fire on us, and we will then manually inspect if it
is
> a false positive or not.

Hi Adam,

An alternative to adding the GC_PLUGIN_IGNORE_CYCLE macro/annotation is that we
maintain an exception file on the side. That way, none of the cycle detection
related stuff would be "visible" to the rest of the blink devs. I'd prefer to
keep the current solution, as it fits in nicely with the existing code, but am
OK with taking a whitelist file approach. If that sounds preferable, where
should we place such a whitelist file?

[abarth-chromium, 2014-03-24 16:38:12]

Assuming we can't remove all of the false positives from the cycle detector, can
we make the cycle detector smart enough that we're able to refactor the shipping
code to avoid tripping the false positives?  I don't think any sort of whitelist
to ignore the cycle detect is a good thing for the long-term health of the code.
 Very few people are going to be expert enough to use the whitelist
appropriately, which means folks will either contort their code to avoid the
warning or whitelist structures that are actually cycles.  Instead, if the cycle
detector was smart enough, we'd only have to contort the code a tiny amount, and
we'd get full value from avoiding cycles.

[abarth-chromium, 2014-03-24 16:38:23]

not lgtm on the whitelist approach

[Mads Ager (chromium), 2014-03-24 18:47:37]

It seems to me that there is a bit of confusion about what this cycle detector
is. It is a tool to help Oilpan developers avoid cycles that mix Members,
RefPtrs and Persistent handles in a way that leads to cycles that cannot be
collected. This is mainly for the transition period - the usual way to solve
such cycles is to move more to the heap so we get rid of the Persistents and
RefPtrs and just have traced pointers between heap objects which can be
collected. The tools analyzes the static structure based on the type of fields.
Therefore, it cannot be precise, it does not have knowledge about the actual
objects involved.

This code review makes the cycle detector available as a tool for the Oilpan
developers if they provide a flag at gyp-time to enable it. It is not meant to
be used by other people.

Adam, what you are objecting to is mainly the idea of a whitelist or annotations
in the source code that people will not know how to deal with? We can avoid
annotations and whitelists and keep them in a document separately so that we do
not pollute the code base with neither annotation nor whitelists. Would that
address your concerns?

We would like to have this tool in the clang plugin code and have a gyp-time
flag to enable it. I believe that this tool will help us avoid uncollectable
cycles in the transition period and it should not influence anyone but the
Oilpan developers.

Am I understanding your concerns or is there something more fundamental that I'm
missing?

[abarth-chromium, 2014-03-24 19:50:30]

On 2014/03/24 18:47:37, Mads Ager (chromium) wrote:
> Adam, what you are objecting to is mainly the idea of a whitelist or
annotations
> in the source code that people will not know how to deal with?

Yes, exactly.

> We can avoid
> annotations and whitelists and keep them in a document separately so that we
do
> not pollute the code base with neither annotation nor whitelists. Would that
> address your concerns?

Yes, thank you.

> Am I understanding your concerns or is there something more fundamental that
I'm
> missing?

Nope, it's the mysterious annotations that I'm worried about.

Thanks!

[zerny-chromium, 2014-03-25 07:21:06]

Thanks for the discussion. In summary, I'll change the process-graph.py script
(which is always manually run) to take an additional file of cycles to ignore.
Adam: Is it correct that you would prefer that we maintain that file outside of
the Blink repository?

I've updated this CL to simply add a GYP flag to enable generation of the
points-to information as part of the compilation.

PTAL.

[zerny-chromium, 2014-03-25 07:41:54]

On 2014/03/25 07:21:06, zerny-chromium wrote:
> Thanks for the discussion. In summary, I'll change the process-graph.py script
> (which is always manually run) to take an additional file of cycles to ignore.
> Adam: Is it correct that you would prefer that we maintain that file outside
of
> the Blink repository?
> 
> I've updated this CL to simply add a GYP flag to enable generation of the
> points-to information as part of the compilation.
> 
> PTAL.

Plugin side change at: https://codereview.chromium.org/210803002/

[Mads Ager (chromium), 2014-03-26 07:47:44]

LGTM

Adam, the updated changelist just adds the gyp flag so we can start using this
locally. Could you have another look?

[abarth-chromium, 2014-03-26 16:29:57]

lgtm

[abarth-chromium, 2014-03-26 16:30:01]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-03-26 16:30:05]

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

[commit-bot: I haz the power, 2014-03-26 16:56:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-26 16:56:22]

Try jobs failed on following builders:
  tryserver.blink on win_blink_compile_dbg

[zerny-chromium, 2014-03-27 04:59:15]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-03-27 04:59:22]

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

[commit-bot: I haz the power, 2014-03-27 06:04:35]

Change committed as 170142