Support ignorance of base class finalizers.

Support ignorance of base class finalizers.

When checking if a class needs a finalizer, allow some of its base
classes to be ignored, even if they do have a declared destructor.

This is done so as to handle WebCore::ScriptWrappable use better while
in transition. i.e., WebCore::ScriptWrappable's destructor does not
have to be invoked when sweeping, and when Oilpan will be on by
default, ScriptWrappable will not define one. However, until that
time, it still provides a user-defined destructor, and to allow the
class to be used as base in conjunction with GarbageCollected<>, we
will now look past it when checking the finalizer requirement.

R=zerny@chromium.org
BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281758

[sof, 2014-07-07 08:33:33]

Please take a look.

[zerny-chromium, 2014-07-07 09:04:33]

Thanks for having a look at this. It would be great to have support for this
exception in the plugin but it is a bit difficult to do so and not loose a lot
of potential bugs in the process. The only way I can see getting it in without
loosing errors is to manually (and recursively) inspect fields as well as the
bases.

An alternative is to use transition types to deal with this. The problematic
cases is a type that is GC'ed in both builds but with different finalization
requirements. In this case a GarbageCollectedFinalizedWillBeGarbageCollected<T>
type should do the trick?

Can you think of other issues that prevent us from using the transition types?

https://codereview.chromium.org/374593002/diff/1/tools/clang/blink_gc_plugin/...
File tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp (right):

https://codereview.chromium.org/374593002/diff/1/tools/clang/blink_gc_plugin/...
tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp:588:
options_.ignored_base_class_finalizers.insert("ScriptWrappable::WebCore");
Lets swap this so it is ns::record

https://codereview.chromium.org/374593002/diff/1/tools/clang/blink_gc_plugin/...
tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp:879: return;
This will allow erroneous code to pass the plugin, since a class declaring
fields that need finalization will not necessarily cause the class to need
finalization anymore.

class Foo : public GarbageCollected<Foo>, public ScriptWrappable {
  // ... and no user declared dtor.
private:
  String m_str;
};

I think this definition of Foo will not give a compile error despite the String
requiring Foo to have finalization support. Also, say Foo did not have the
string, then still a subclass would still require finalization since we only
ignore on the direct bases:

class Foo : public GarbageCollected<Foo>, public ScriptWrappable { };
class Bar : public Foo { };

Ie, this definition of Bar will give an error I think.

[sof, 2014-07-07 09:11:52]

On 2014/07/07 09:04:33, zerny-chromium wrote:
> Thanks for having a look at this. It would be great to have support for this
> exception in the plugin but it is a bit difficult to do so and not loose a lot
> of potential bugs in the process. The only way I can see getting it in without
> loosing errors is to manually (and recursively) inspect fields as well as the
> bases.
> 
> An alternative is to use transition types to deal with this. The problematic
> cases is a type that is GC'ed in both builds but with different finalization
> requirements. In this case a
GarbageCollectedFinalizedWillBeGarbageCollected<T>
> type should do the trick?
> 

If you want to see that approach up close,
https://codereview.chromium.org/367113005/

[sof, 2014-07-07 18:49:22]

Thanks for the feedback, shallowly performing the check when reporting the state
of the finalization-needing class is not going to work.

Reworked the RecordInfo::NeedsFinalization() predicate instead to take
'safely-ignorable' classes into account.

[zerny-chromium, 2014-07-08 07:45:03]

This recursive check will do the trick. Looks mostly good with a few comments.

https://codereview.chromium.org/374593002/diff/20001/tools/clang/blink_gc_plu...
File tools/clang/blink_gc_plugin/RecordInfo.cpp (right):

https://codereview.chromium.org/374593002/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/RecordInfo.cpp:412: if (does_need_finalization_) {
Add early exit instead:

if (!does_need_finalization_)
  return does_need_finalization_;

https://codereview.chromium.org/374593002/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/RecordInfo.cpp:417: does_need_finalization_ =
kFalse;
Do an early return in this block as well and avoid the following conditional.

https://codereview.chromium.org/374593002/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/RecordInfo.cpp:426: is_candidate = false;
This can just return does_need_finalization_ which is true here.

https://codereview.chromium.org/374593002/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/RecordInfo.cpp:434: if (ns &&
This check is not be needed here. The ignored case already happens in the
base-case of an ignored class.

https://codereview.chromium.org/374593002/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/RecordInfo.cpp:439: is_candidate = false;
Just return does_need_finalization_

https://codereview.chromium.org/374593002/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/RecordInfo.cpp:441: if (is_candidate)
Here we can unconditionally set does_need_finalization_ = kFalse.

https://codereview.chromium.org/374593002/diff/20001/tools/clang/blink_gc_plu...
File tools/clang/blink_gc_plugin/tests/class_requires_finalization_base.h
(right):

https://codereview.chromium.org/374593002/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/tests/class_requires_finalization_base.h:26: class
ScriptWrappable {
Define ScriptWrappable in heap/stubs.h

That should avoid some of the line:column changes and make it easier to simply
remove the ScriptWrappable destructor when we remove the ignore code again.

https://codereview.chromium.org/374593002/diff/20001/tools/clang/blink_gc_plu...
File tools/clang/blink_gc_plugin/tests/class_requires_finalization_mixin.h
(right):

https://codereview.chromium.org/374593002/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/tests/class_requires_finalization_mixin.h:59: :
public GarbageCollected<HasFinalizer>
GarbageCollected<NeedsNoFinalization>

[sof, 2014-07-08 08:47:06]

https://codereview.chromium.org/374593002/diff/20001/tools/clang/blink_gc_plu...
File tools/clang/blink_gc_plugin/RecordInfo.cpp (right):

https://codereview.chromium.org/374593002/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/RecordInfo.cpp:412: if (does_need_finalization_) {
On 2014/07/08 07:45:03, zerny-chromium wrote:
> Add early exit instead:
> 
> if (!does_need_finalization_)
>   return does_need_finalization_;

Rephrased to use early returns instead of fallthrough.

https://codereview.chromium.org/374593002/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/RecordInfo.cpp:434: if (ns &&
On 2014/07/08 07:45:03, zerny-chromium wrote:
> This check is not be needed here. The ignored case already happens in the
> base-case of an ignored class.

I was hesitant to remove it, in case the complete type of the ignorable classes
wasn't in case. But in that case (and if that's supported), the forward decl
must be, so it will have been processed already. Hence, ok to do so. Removed.

https://codereview.chromium.org/374593002/diff/20001/tools/clang/blink_gc_plu...
File tools/clang/blink_gc_plugin/tests/class_requires_finalization_base.h
(right):

https://codereview.chromium.org/374593002/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/tests/class_requires_finalization_base.h:26: class
ScriptWrappable {
On 2014/07/08 07:45:03, zerny-chromium wrote:
> Define ScriptWrappable in heap/stubs.h
> 
> That should avoid some of the line:column changes and make it easier to simply
> remove the ScriptWrappable destructor when we remove the ignore code again.

Done.

https://codereview.chromium.org/374593002/diff/20001/tools/clang/blink_gc_plu...
File tools/clang/blink_gc_plugin/tests/class_requires_finalization_mixin.h
(right):

https://codereview.chromium.org/374593002/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/tests/class_requires_finalization_mixin.h:59: :
public GarbageCollected<HasFinalizer>
On 2014/07/08 07:45:03, zerny-chromium wrote:
> GarbageCollected<NeedsNoFinalization>

Done.

[sof, 2014-07-08 08:48:49]

https://codereview.chromium.org/374593002/diff/20001/tools/clang/blink_gc_plu...
File tools/clang/blink_gc_plugin/RecordInfo.cpp (right):

https://codereview.chromium.org/374593002/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/RecordInfo.cpp:434: if (ns &&
On 2014/07/08 08:47:05, sof wrote:
> On 2014/07/08 07:45:03, zerny-chromium wrote:
> > This check is not be needed here. The ignored case already happens in the
> > base-case of an ignored class.
> 
> I was hesitant to remove it, in case the complete type of the ignorable
classes
> wasn't in case. But in that case (and if that's supported), the forward decl
> must be, so it will have been processed already. Hence, ok to do so. Removed.

s/in case/in scope/. Need more coffee.

[zerny-chromium, 2014-07-08 11:09:38]

lgtm

[sof, 2014-07-08 11:21:36]

Thanks for the review (and the code for this static analysis pass, easy to work
with.)

[sof, 2014-07-08 11:21:47]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-07-08 11:25:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/374593002/40001

[commit-bot: I haz the power, 2014-07-08 15:33:41]

Change committed as 281758

[Nico, 2014-07-08 15:41:56]

(sof: In case you aren't aware: The plugin is bundled with the clang binaries,
so this change won't have an effect until we build new binaries. This isn't hard
to do, but if you have more changes planned it might make sense to rebuild them
after they're all done (assuming they're all relatively small). If this is your
only change, let us know if it's urgent; else it'll be part of the "regular"
clang refresh eventually.)

[sof, 2014-07-08 15:46:28]

On 2014/07/08 15:41:56, Nico (away) wrote:
> (sof: In case you aren't aware: The plugin is bundled with the clang binaries,
> so this change won't have an effect until we build new binaries. This isn't
hard
> to do, but if you have more changes planned it might make sense to rebuild
them
> after they're all done (assuming they're all relatively small). If this is
your
> only change, let us know if it's urgent; else it'll be part of the "regular"
> clang refresh eventually.)

thanks, nothing urgent about this. No other CLs planned.

[sof, 2014-07-18 12:58:34]

On 2014/07/08 15:46:28, sof wrote:
> On 2014/07/08 15:41:56, Nico (away) wrote:
> > (sof: In case you aren't aware: The plugin is bundled with the clang
binaries,
> > so this change won't have an effect until we build new binaries. This isn't
> hard
> > to do, but if you have more changes planned it might make sense to rebuild
> them
> > after they're all done (assuming they're all relatively small). If this is
> your
> > only change, let us know if it's urgent; else it'll be part of the "regular"
> > clang refresh eventually.)
> 
> thanks, nothing urgent about this. No other CLs planned.

The rename from WebCore to blink necessitated an update to blink_gc_plugin and a
new clang roll, it seems --

 http://commondatastorage.googleapis.com/chromium-browser-clang/index.html

Do we have to wait for that to deploy on the bots? (The CL that depends on it
fails still, https://codereview.chromium.org/367113005/ )

[Nico, 2014-07-18 15:57:45]

On 2014/07/18 12:58:34, sof wrote:
> On 2014/07/08 15:46:28, sof wrote:
> > On 2014/07/08 15:41:56, Nico (away) wrote:
> > > (sof: In case you aren't aware: The plugin is bundled with the clang
> binaries,
> > > so this change won't have an effect until we build new binaries. This
isn't
> > hard
> > > to do, but if you have more changes planned it might make sense to rebuild
> > them
> > > after they're all done (assuming they're all relatively small). If this is
> > your
> > > only change, let us know if it's urgent; else it'll be part of the
"regular"
> > > clang refresh eventually.)
> > 
> > thanks, nothing urgent about this. No other CLs planned.
> 
> The rename from WebCore to blink necessitated an update to blink_gc_plugin and
a
> new clang roll, it seems --
> 
>  http://commondatastorage.googleapis.com/chromium-browser-clang/index.html

That's independent of the blink rename; we just roll clang every now and then
(we're shooting for every 1-2 weeks normally; there was a longer break this time
due to vacations and http://crbug.com/381910)

> Do we have to wait for that to deploy on the bots? (The CL that depends on it
> fails still, https://codereview.chromium.org/367113005/ )

commondatastorage is where bots downloads their compilers from, but just
uploading a binary there doesn't make it active – we need to change the clang
download script to download the new version.
https://codereview.chromium.org/397243003/ is the change that does that, but due
to its tryjobs not even starting it hasn't landed yet. (There are a few red
bots, but the whitespace cl linked from
https://code.google.com/p/chromium/issues/detail?id=381489#c10 has them too.)

[sof, 2014-07-19 06:23:47]

On 2014/07/18 15:57:45, Nico (away) wrote:
> On 2014/07/18 12:58:34, sof wrote:
> > On 2014/07/08 15:46:28, sof wrote:
> > > On 2014/07/08 15:41:56, Nico (away) wrote:
> > > > (sof: In case you aren't aware: The plugin is bundled with the clang
> > binaries,
> > > > so this change won't have an effect until we build new binaries. This
> isn't
> > > hard
> > > > to do, but if you have more changes planned it might make sense to
rebuild
> > > them
> > > > after they're all done (assuming they're all relatively small). If this
is
> > > your
> > > > only change, let us know if it's urgent; else it'll be part of the
> "regular"
> > > > clang refresh eventually.)
> > > 
> > > thanks, nothing urgent about this. No other CLs planned.
> > 
> > The rename from WebCore to blink necessitated an update to blink_gc_plugin
and
> a
> > new clang roll, it seems --
> > 
> >  http://commondatastorage.googleapis.com/chromium-browser-clang/index.html
> 
> That's independent of the blink rename; we just roll clang every now and then
> (we're shooting for every 1-2 weeks normally; there was a longer break this
time
> due to vacations and http://crbug.com/381910)
> 
> > Do we have to wait for that to deploy on the bots? (The CL that depends on
it
> > fails still, https://codereview.chromium.org/367113005/ )
> 
> commondatastorage is where bots downloads their compilers from, but just
> uploading a binary there doesn't make it active – we need to change the
clang
> download script to download the new version.
> https://codereview.chromium.org/397243003/ is the change that does that, but
due
> to its tryjobs not even starting it hasn't landed yet. (There are a few red
> bots, but the whitespace cl linked from
> https://code.google.com/p/chromium/issues/detail?id=381489#c10 has them too.)

Thanks much for explaining; will check in on the status of that CL & roll.