960342 - Implement custom Rooteds with virtual trace() functions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

StackBaseShape::AutoRooter fixes hazards, but the analysis doesn't understand it yet. bz suggested making a Rooted instead. I kind of like that approach, since it's more clear what is protected. But it's sort of a pain to do one-offs, so I thought it'd be cool if we had a custom Rooted like with have the CustomAutoRooter.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Add a CustomRooted general class and use it for StackBaseShape::Rooted

The internal implementation is not pretty. Asking for feedback as to whether this seems like a viable approach. It looks pretty good from the user's point of view (as in, defining your own Rooted is pretty straightforward.)

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 8360760 [details] [diff] [review]
Add a CustomRooted general class and use it for StackBaseShape::Rooted

Review of attachment 8360760 [details] [diff] [review]:
-----------------------------------------------------------------

After discussion on IRC, we realized that this is basically identical to CustomAutoRooter as is. We decided on some changes to make it more like a Rooted.

1) Move the data declaration internal to CustomRooted, like other Rooted.
2) Add |CustomRooted<>::trace(JSTracer *trc) { data.trace(trc); }| to automatically forward tracing to the most relevant class, if possible.
3) Add |CustomRooted<T*>::trace(JSTracer *trc) { data->trace(trc); }| for cases where the rooter is rooting a reference to a thing for perforamnce, as in StackBaseShape.
4) Remove StackBaseShape::AutoRooter and replace with CustomRooted<StackBaseShape*>.
5) Move it all to RootingAPI.h.

::: js/src/gc/RootMarking.cpp
@@ +64,5 @@
>        case THING_ROOT_ID:          MarkIdRoot(trc, (jsid *)addr, "exact-id"); break;
>        case THING_ROOT_PROPERTY_ID: MarkIdRoot(trc, &((js::PropertyId *)addr)->asId(), "exact-propertyid"); break;
>        case THING_ROOT_BINDINGS:    ((Bindings *)addr)->trace(trc); break;
>        case THING_ROOT_PROPERTY_DESCRIPTOR: ((JSPropertyDescriptor *)addr)->trace(trc); break;
> +      case THING_ROOT_CUSTOM: ((CustomRootedBase *)addr)->trace(trc); break;

Align with the others. Which will be important when we re-implement PropertyDescriptor on top of this.

Comment 3

[Steve Fink [:sfink] [:s:]]

> > +      case THING_ROOT_CUSTOM: ((CustomRootedBase *)addr)->trace(trc); break;

Sadly, this doesn't work.

First, it's totally the wrong pointer to use. addr is the address of the 'ptr' field in a Rooted, whereas CustomRootedBase is a Rooted itself. So this is like 0x18 off already. But that's easily fixable using the local variable 'rooter' instead of 'addr'.

However, 'rooter' is a Rooted<void*> here. CustomRootedBase is a class with a vtable. The vtable and the Rooted data can't both be at offset 0x0. So this needs some fixup. For example, this would work:

          reinterpret_cast<CustomRootedBase*>(uintptr_t(rooter) - sizeof(void*))->trace(trc);

It would also make code readers cry. There are more graceful ways of doing that, though.

I'm looking into alternatives. One possibility is to munge CustomAutoRooter into what I want.

Comment 4

[Steve Fink [:sfink] [:s:]]

> However, 'rooter' is a Rooted<void*> here. CustomRootedBase is a class with
> a vtable. The vtable and the Rooted data can't both be at offset 0x0. So
> this needs some fixup. For example, this would work:
> 
>           reinterpret_cast<CustomRootedBase*>(uintptr_t(rooter) -
> sizeof(void*))->trace(trc);
> 
> It would also make code readers cry. There are more graceful ways of doing
> that, though.

This is a little better. Instead of inheriting from Rooted<GCType>, I put it as an element of CustomRooted, and then can do this:

          const size_t rooterOffset = offsetof(CustomRooted<void*>, rooter);
          reinterpret_cast<CustomRootedBase*>(uintptr_t(rooter) - rooterOffset)->trace(trc);

(In reply to Terrence Cole [:terrence] from comment #2)
> 1) Move the data declaration internal to CustomRooted, like other Rooted.

Actually, I already have storage space for this in the Rooted itself, so I'm now using that directly.

> 2) Add |CustomRooted<>::trace(JSTracer *trc) { data.trace(trc); }| to
> automatically forward tracing to the most relevant class, if possible.
> 3) Add |CustomRooted<T*>::trace(JSTracer *trc) { data->trace(trc); }| for
> cases where the rooter is rooting a reference to a thing for perforamnce, as
> in StackBaseShape.

Now I don't think I follow. But I'm ending up with the same thing, I think -- now you define a trace() on the actual class (eg StackBaseShape) and it will be called.

> 4) Remove StackBaseShape::AutoRooter and replace with
> CustomRooted<StackBaseShape*>.

Yep. That was already gone.

> 5) Move it all to RootingAPI.h.

Done.

I still think I'm going to hell for this code.

Comment 5

[Steve Fink [:sfink] [:s:]]

Attachment: Add a RootedCustom general class and use it for StackBaseShape::Rooted

Here's another attempt at the base functionality. Seems to work ok for pointer types.

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: Convert StackBaseShape from AutoRooter to RootedCustom

Here's a conversion of StackBaseShape using RootedCustom. Still waffling on the RootedCustom/CustomRooted name.

This one is pretty straightforward.

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Convert StackShape from AutoRooter to RootedCustom

Getting a bit messier, here's a replacement for StackShape::AutoRooter.

Comment 8

[Steve Fink [:sfink] [:s:]]

I took a stab at Rooted<JSPropertyDescriptor>. It's a lot more troublesome, mainly because it's not a pointer type and more would be required to handle it. I didn't manage to finish the patch.

Comment 9

[Terrence Cole [:terrence]]

Comment on attachment 8360900 [details] [diff] [review]
Add a RootedCustom general class and use it for StackBaseShape::Rooted

Review of attachment 8360900 [details] [diff] [review]:
-----------------------------------------------------------------

r=me With a different name. I don't care overly much between the options we've discussed.

::: js/public/RootingAPI.h
@@ +918,5 @@
>  
> +class RootedCustomBase {
> +  public:
> +    virtual void trace(JSTracer *trc) = 0;
> +};

Do we actually need or want a RootedCustomBase here? The purpose of RootedBase<T> is so that you can specialize on T to provide extra behaviors to the child. I guess we could add template <typename T>, but I'm not sure if it's worth the extra complexity yet.

@@ +937,5 @@
> + * from this pointer to find a vtable containing a type-appropriate trace()
> + * method.
> + */
> +template <typename GCType>
> +class JS_PUBLIC_API(RootedCustom) : public RootedCustomBase

I think we should go with GenericRooted, to distinguish it even more from CustomAutoRooter.

@@ +948,5 @@
> +        : rooter(cx), skip(cx, rooter.address())
> +    {
> +    }
> +
> +    RootedCustom(js::ContextFriendFields *cx, GCType initial)

I guess this should be |const GCType &| in case we make GCType something larger than a pointer? Will that add unsafe refs warnings everywhere we use this if we do that?

Comment 10

[Terrence Cole [:terrence]]

Comment on attachment 8360902 [details] [diff] [review]
Convert StackBaseShape from AutoRooter to RootedCustom

Review of attachment 8360902 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I think this is basically what we want. r=me

::: js/src/vm/Shape.h
@@ +846,5 @@
>      static inline HashNumber hash(const StackBaseShape *lookup);
>      static inline bool match(UnownedBaseShape *key, const StackBaseShape *lookup);
>  
> +    // For RootedCustom<StackBaseShape*>
> +    static inline js::ThingRootKind rootKind() { return js::THING_ROOT_CUSTOM; }

I still don't understand why we need this to be here and not on the CustomRooted, or even CustomRooted<StackBaseShape*>.

Comment 11

[Terrence Cole [:terrence]]

Comment on attachment 8360903 [details] [diff] [review]
Convert StackShape from AutoRooter to RootedCustom

Review of attachment 8360903 [details] [diff] [review]:
-----------------------------------------------------------------

r=me Great!

Comment 12

[Terrence Cole [:terrence]]

(In reply to Terrence Cole [:terrence] from comment #10)
> > +    // For RootedCustom<StackBaseShape*>
> > +    static inline js::ThingRootKind rootKind() { return js::THING_ROOT_CUSTOM; }
> 
> I still don't understand why we need this to be here and not on the
> CustomRooted, or even CustomRooted<StackBaseShape*>.

Right, because the Rooted is nested now and not a base any longer.

Comment 13

[Steve Fink [:sfink] [:s:]]

(In reply to Terrence Cole [:terrence] from comment #12)
> (In reply to Terrence Cole [:terrence] from comment #10)
> > > +    // For RootedCustom<StackBaseShape*>
> > > +    static inline js::ThingRootKind rootKind() { return js::THING_ROOT_CUSTOM; }
> > 
> > I still don't understand why we need this to be here and not on the
> > CustomRooted, or even CustomRooted<StackBaseShape*>.
> 
> Right, because the Rooted is nested now and not a base any longer.

It would still be needed if it were a base.

If you had class CustomRooted<T> : public Rooted<T> { ... }, then you need to be able to instantiate Rooted<T>. That requires either using the default GCMethods<T>::kind(), which looks for a T::rootKind(), or specializing GCMethods<T> to specify the kind() that way. CustomRooted is out of the picture at this point. I prefer defining T::rootKind() just because it lets you make a class CustomRoot-able (now named RootedGeneric) by defining rootKind() and trace() right next to each other.

Comment 14

[Steve Fink [:sfink] [:s:]]

So, the end result of this patch: this takes us from 20 unsafe references to... 20 unsafe references. :~(

The problem is that previously there'd be a StackShape passed in by reference, so no hazards would be reported whether or not you had a StackShape::AutoRooter involved. There would be an unsafe reference, but it's in the caller that is passing in a StackShape by reference -- and this patch doesn't change that.

This patch fixes this case:

   StackShape child;
   StackShape::AutoRooter root(&child);
   GC();
   child.foo();

because it's now

   StackShape unrootedChild;
   RootedGeneric<StackShape*> child(&unrootedChild);
   GC();
   child.foo();

so the use-after-GC is now of the rooted object. But those would have been reported as hazards, so we got rid of them already anyway.

So the question now is, how can we eliminate those unsafe ref reports?

Comment 15

[Steve Fink [:sfink] [:s:]]

One option would be to just ignore refs of unrooted StackShapes. Then we would not catch introductions of hazards like:

  StackShape child;
  ShapeShape *trouble = &child;
  canGCFunc();
  trouble->foo();

(a "caller hazard") and

  void func(StackShape *child)
  {
     GC();
     child.foo();
  }

(a "callee hazard"). The caller hazard seems pretty unlikely, so I think we can ignore it. For the callee hazard, what if we lied and said that StackShape* is a GCPointer? Then it would be reported as a hazard. The safe usage would be

  void func(StackShape *unrootedChild)
  {
     RootedGeneric<StackShape*> child(unrootedChild);
     GC();
     child->foo();
  }

which would not be reported as a hazard.

Comment 16

[Terrence Cole [:terrence]]

I really like the idea of making StackShape* a GCPointer. I guess those get treated differently than references to GCTypes in the analysis?

Comment 17

[Steve Fink [:sfink] [:s:]]

(In reply to Terrence Cole [:terrence] from comment #16)
> I really like the idea of making StackShape* a GCPointer. I guess those get
> treated differently than references to GCTypes in the analysis?

Not sure what you mean by "references" here. A StackShape& will be identical to a StackShape* from the point of view of the analysis. (The sixgill layer erases that distinction and passes them both through as a pointer to a StackShape.)

But the analysis knows about GCTypes (eg JSObject, JSString) and GCPointers (eg JSObject*, JSString*, Value, StackShape). StackShape is a GCPointer because it is a structure containing GCPointers. (If it contained direct GCTypes, it would be a GCType, but that doesn't happen. Well, maybe something like JSFunction, if it starts with a JSObject? Dunno.)

So the change is actually quite simple: treat StackShape as a GCType instead of a GCPointer. Only GCPointers change across GCs, not GCTypes, so this will result in the analysis ignoring cases where a StackShape is held live across a GC. But a StackShape* (which is really a double pointer to a GCType) will now be considered a pointer to a GCType, or in other words a GCPointer, and so the analysis will report hazards where one is kept live across a GC (as in comment 15).

A really, really nasty hack, but it seems to at least do the ignoring part correctly. See https://tbpl.mozilla.org/?tree=Try&rev=cd3c7b2e0c03 for a run that reports 24 unsafe refs and (still) 0 hazards.

I need to do a run where I intentionally hold a StackShape* live across a GC to verify that it gets picked up.

Comment 18

[Steve Fink [:sfink] [:s:]]

Attachment: Look for StackShape* hazards instead of StackShape hazards

Analysis hack for the StackShape and StackBaseShape stuff.

Comment 19

[Terrence Cole [:terrence]]

Comment on attachment 8362285 [details] [diff] [review]
Look for StackShape* hazards instead of StackShape hazards

Review of attachment 8362285 [details] [diff] [review]:
-----------------------------------------------------------------

Nasty, but I think it's the right solution here. r=me

Comment 20

[Steve Fink [:sfink] [:s:]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce9500f47c73
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7bee3ff046d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b74d292da1fa

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ce9500f47c73
https://hg.mozilla.org/mozilla-central/rev/c7bee3ff046d
https://hg.mozilla.org/mozilla-central/rev/b74d292da1fa