767141 - moar rooting 1

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

I'm running the rooting analysis, and it's failing all over the place during initialization. I manually went through all of jsapi.cpp, CTypes.cpp, and frontend/BytecodeCompiler.cpp and fixed them up. I also went through many other places that were affected or that I happened to hit in my simple test case. I originally intended to fix up everything necessary for my test file to run under |-m -n|, but eventually gave up on doing JM incrementally.

So anyway, this bug is for an arbitrary slice of the engine, with 100% coverage of the above mentioned 3 files.

Comment 1

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

Attachment: Implement AssertRootingUnnecessary guard

This RAII guard is used to mark lexical regions that are believed to never trigger GC. It is mostly intended for documentation, but is also dynamically checked.

Comment 2

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

Attachment: Add a handle(i) accessor to AutoVectorRooter

Given that AutoVectorRooter roots a vector of gcthings, it seems convenient to be able to access elements for passing into Handle-accepting functions without re-rooting.

Comment 3

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

Attachment: moar rooting

Believed to be complete rooting for jsapi.cpp, ctypes/CTypes.cpp, and frontend/BytecodeCompiler.cpp, and lots of rooting for additional things I encountered along the way

Comment 4

[Brian Hackett [Laid off!]]

Comment on attachment 635462 [details] [diff] [review]
Implement AssertRootingUnnecessary guard

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

::: js/src/gc/Root.h
@@ +319,5 @@
>  
> +#if defined(JSGC_ROOT_ANALYSIS) && defined(DEBUG) && !defined(JS_THREADSAFE)
> +bool IsRootingUnnecessaryForContext(JSContext *cx);
> +void SetRootingUnnecessaryForContext(JSContext *cx, bool value);
> +class AssertRootingUnnecessary {

I think it would be better if the class definition was outside the #ifdef, so it only has to appear once, and you can use the GUARD_OBJECT_NOTIFIER stuff in all cases without duplicate boilerplate.

I also think you can just make the contents of this class #ifdef DEBUG, and assert !cx->rootingUnnecessary within MaybeCheckStackRoots in any debug build.

This class is a great idea and it would be good to push the assertion checking even further.  r+ as is, but it would be great if you could make these changes and post an updated patch.

Comment 5

[Brian Hackett [Laid off!]]

Comment on attachment 635466 [details] [diff] [review]
moar rooting

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

Thanks!

::: js/src/ctypes/CTypes.cpp
@@ +2362,5 @@
>  // Convert jsval 'val' to a C binary representation of CType 'targetType',
>  // storing the result in 'buffer'. This function is more forceful than
>  // ImplicitConvert.
>  JSBool
> +ExplicitConvert(JSContext* cx, jsval val, JSObject* targetType_, void* buffer)

Is there a reason for taking a bare JSObject* here and not a handle?  Is this used as a function pointer (can also change the function pointer signature)?

Generally these should be replaced with handles and rooting within the callee avoided, mostly so that C++ will type check the caller to see that it uses roots too.

Ditto for other instances of this below.

jsapi.cpp does this a lot but that will change pretty soon once the handle based API is done.

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 635466 [details] [diff] [review]
moar rooting

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

::: js/src/frontend/BytecodeCompiler.h
@@ +13,5 @@
>  namespace js {
>  namespace frontend {
>  
>  bool
> +CompileFunctionBody(JSContext *cx, HandleFunction fun,

There was some quasi-consensus on the js-internals mailing list that the Handle<JSFunction*> form was preferable...

::: js/src/jsscript.h
@@ +567,5 @@
>    public:
>      static JSScript *Create(JSContext *cx, bool savedCallerFun,
>                              JSPrincipals *principals, JSPrincipals *originPrincipals,
>                              bool compileAndGo, bool noScriptRval,
> +                            js::GlobalObject* globalObject, JSVersion version,

ORLY?  No need to change this, plz.

Comment 7

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

(In reply to Nicholas Nethercote [:njn] from comment #6)
> Comment on attachment 635466 [details] [diff] [review]
> moar rooting
> 
> Review of attachment 635466 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/frontend/BytecodeCompiler.h
> @@ +13,5 @@
> >  namespace js {
> >  namespace frontend {
> >  
> >  bool
> > +CompileFunctionBody(JSContext *cx, HandleFunction fun,
> 
> There was some quasi-consensus on the js-internals mailing list that the
> Handle<JSFunction*> form was preferable...

I'll raise this in the GC meeting today, but I'm kind of expecting a full trivial rewrite pass of some sort once the naming bikeshedding is complete, so I don't much care. And the discussion on the mailing list wasn't close enough to a consensus yet (as in, there were fairly strong opinions for both sides).

> ::: js/src/jsscript.h
> @@ +567,5 @@
> >    public:
> >      static JSScript *Create(JSContext *cx, bool savedCallerFun,
> >                              JSPrincipals *principals, JSPrincipals *originPrincipals,
> >                              bool compileAndGo, bool noScriptRval,
> > +                            js::GlobalObject* globalObject, JSVersion version,
> 
> ORLY?  No need to change this, plz.

Doh! Thanks. I think the ctypes convention is the opposite and that got stuck in my head. Though I have a suspicion that current-day JS developers may actually prefer |type* var|, and the Handle-ification of the API wouldn't be the worst possible time for a flag day... nah, never mind.

Comment 8

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

(In reply to Brian Hackett (:bhackett) from comment #5)
> Comment on attachment 635466 [details] [diff] [review]
> moar rooting
> 
> Review of attachment 635466 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!
> 
> ::: js/src/ctypes/CTypes.cpp
> @@ +2362,5 @@
> >  // Convert jsval 'val' to a C binary representation of CType 'targetType',
> >  // storing the result in 'buffer'. This function is more forceful than
> >  // ImplicitConvert.
> >  JSBool
> > +ExplicitConvert(JSContext* cx, jsval val, JSObject* targetType_, void* buffer)
> 
> Is there a reason for taking a bare JSObject* here and not a handle?  Is
> this used as a function pointer (can also change the function pointer
> signature)?

Because I wasn't changing public API, and ExplicitConvert is defined in CTypes.h so I assume that it is part of the API.

On the other hand, I also didn't convert a bunch of the static functions defined in a collection of namespaces at the top of CTypes.cpp, and there's no reason not to.

> Generally these should be replaced with handles and rooting within the
> callee avoided, mostly so that C++ will type check the caller to see that it
> uses roots too.
> 
> Ditto for other instances of this below.
> 
> jsapi.cpp does this a lot but that will change pretty soon once the handle
> based API is done.

I think I'll land this now and do the APIs together. (Or almost "this" -- I also switched the static functions to handles, too.)

Comment 9

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

Attachment: nasty Type rooting

Still trying to get this through the try server.

This patch does some especially ugly juggling to root a Type used as a key. Please give me a simple alternative to avoid this. :)

Comment 10

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

Attachment: null check for AutoGCVector

I'm not sure about the perf implications of this patch, so I wanted to run it by billm.

The problem is when I use an auto vector rooter, the vector can contain NULLs, and those get passed into MarkInternal, which barfs on them. Sorry; I should have kept a stack trace. I don't remember the exact names. But it's a vector of JSObject* and it's doing something like MarkRange, which just goes over every entry and passes in the address of each. The addresses are of course non-NULL, but the thing=*thingp can be NULL.

Comment 11

[Brian Hackett [Laid off!]]

Comment on attachment 637745 [details] [diff] [review]
nasty Type rooting

Eh, this looks fine to me.  Bug 767223 makes GCs during AutoEnterTypeInference impossible, which would remove the need for this root, but I suspect there are other places Types may need to be rooted even with that change.

Comment 12

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d87f9f5d3c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/54bb503183eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dd9559f15ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f15e352d4b4

Comment 13

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

Comment on attachment 637746 [details] [diff] [review]
null check for AutoGCVector

Talked to billm over IRC; moved the null check to the range rooter and folded into the rooting patch.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0d87f9f5d3c6
https://hg.mozilla.org/mozilla-central/rev/54bb503183eb
https://hg.mozilla.org/mozilla-central/rev/0dd9559f15ff
https://hg.mozilla.org/mozilla-central/rev/8f15e352d4b4

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

The change at <https://hg.mozilla.org/mozilla-central/rev/0d87f9f5d3c6#l4.1> seems to be bogus.

Comment 16

[Ed Morley [:emorley]]

In addition to comment 15, this turned mozilla-inbound_linux64-debug_spidermonkey-rootanalysis from orange (https://tbpl.mozilla.org/php/getParsedLog.php?id=13122160&tree=Mozilla-Inbound) to red (https://tbpl.mozilla.org/php/getParsedLog.php?id=13124032&tree=Mozilla-Inbound). It is hidden on TBPL, so not something that anything would be backed out over - but just thought I'd mention :-)

Comment 17

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

Attachment: followup - remove extra return statement.

It compiled successfully after I rebased; how could it possibly be wrong? :)

Comment 18

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4cd38116f593

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4cd38116f593