802319 - Various rooting fixes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

With this patch applied, js/src/tests (jstests) currently passes all tests.

Comment 1

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

Attachment: Various rooting fixes

If only I could push this to try test test rooting analysis there...

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 671996 [details] [diff] [review]
Various rooting fixes

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

That's a smaller set of changes to get jstests working than I expected.  We /really/ need to get these tested regularly on Try now.

::: js/src/shell/js.cpp
@@ +2035,5 @@
>      bool ok;
>  
> +    // Grab the depth param first, because theoretically JS_ValueToECMAUint32
> +    // can GC. Only it probably doesn't matter, since any way it could GC would
> +    // probably also result in it returning false.

Can't JS_ValueToString also GC?  I'm not sure I see why this move.  Wouldn't it be better/easier to just root everything in DumpHeap properly at all times?

@@ +2042,5 @@
> +        v = JS_ARGV(cx, vp)[3];
> +        if (!JSVAL_IS_NULL(v)) {
> +            uint32_t depth;
> +
> +            if (!JS_ValueToECMAUint32(cx, v, &depth))

Bill changed DumpHeap recently so that it's not callable from GC anymore: could we add an AssertCanGC() to the top of DumpHeap and JS_ValueToECMAUint32 since it seems like these could be troublesome in other cases as well?

::: js/src/vm/Stack.cpp
@@ +1517,5 @@
> +    fp_(other.fp_),
> +    calls_(other.calls_),
> +    seg_(other.seg_),
> +    pc_(other.pc_),
> +    script_(other.maybecx_ ? other.maybecx_->runtime : TlsRuntime.get(), other.script_),

On one hand, uhg, but on the other, this is what we added TlsRuntime to do: otherwise we would be totally out-of-luck here.

::: js/src/vm/Stack.h
@@ +1723,5 @@
>      CallArgsList *calls_;
>  
>      StackSegment *seg_;
>      jsbytecode   *pc_;
> +    RootedScript  script_;

Glad to see this worked out.

Comment 3

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

(In reply to Terrence Cole [:terrence] from comment #2)
> ::: js/src/shell/js.cpp
> @@ +2035,5 @@
> >      bool ok;
> >  
> > +    // Grab the depth param first, because theoretically JS_ValueToECMAUint32
> > +    // can GC. Only it probably doesn't matter, since any way it could GC would
> > +    // probably also result in it returning false.
> 
> Can't JS_ValueToString also GC?

Oh, didn't even notice that. Yes, it can, but it's right after the depth argument now so it doesn't matter. (The depth argument doesn't put any gcthings on the stack. Nor does the fileName argument, which is what calls JS_ValueToString, so I think I'll swap the order just to be a little more sensible.)

> I'm not sure I see why this move.  Wouldn't
> it be better/easier to just root everything in DumpHeap properly at all
> times?

I don't know how. DumpHeap calls JSVAL_IS_TRACEABLE(v) on two of its arguments, then stores them in void* variables. I don't know how to root those. I could make them gc::Cell* instead, but you still can't do a Rooted<gc::Cell*>.

Sorry, I'll beef up the comment like I should have done in the first place.

> @@ +2042,5 @@
> > +        v = JS_ARGV(cx, vp)[3];
> > +        if (!JSVAL_IS_NULL(v)) {
> > +            uint32_t depth;
> > +
> > +            if (!JS_ValueToECMAUint32(cx, v, &depth))
> 
> Bill changed DumpHeap recently so that it's not callable from GC anymore:
> could we add an AssertCanGC() to the top of DumpHeap and
> JS_ValueToECMAUint32 since it seems like these could be troublesome in other
> cases as well?

Um. DumpHeap, sure. JS_ValueToECMAUint32 I have mixed feelings about. I would need to verify this, but I think it's one of those things that can only GC if it also ends up returning false. Usually when something that takes a cx returns false then all of the stack-allocated variables are dead anyway because the caller is just going to return.

> 
> ::: js/src/vm/Stack.cpp
> @@ +1517,5 @@
> > +    fp_(other.fp_),
> > +    calls_(other.calls_),
> > +    seg_(other.seg_),
> > +    pc_(other.pc_),
> > +    script_(other.maybecx_ ? other.maybecx_->runtime : TlsRuntime.get(), other.script_),
> 
> On one hand, uhg, but on the other, this is what we added TlsRuntime to do:
> otherwise we would be totally out-of-luck here.
> 
> ::: js/src/vm/Stack.h
> @@ +1723,5 @@
> >      CallArgsList *calls_;
> >  
> >      StackSegment *seg_;
> >      jsbytecode   *pc_;
> > +    RootedScript  script_;
> 
> Glad to see this worked out.

I think it was the copy constructor thing that that scared me off earlier.

Comment 4

[Jeff Walden [:Waldo]]

(In reply to Steve Fink [:sfink] from comment #3)
> JS_ValueToECMAUint32 I have mixed feelings about. I would need to
> verify this, but I think it's one of those things that can only GC
> if it also ends up returning false.

Not so; for example, the value might be this:

  ({ valueOf: function() { gc(); return 12; } })

The result of the conversion would of course successfully be 12.

Comment 5

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

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> (In reply to Steve Fink [:sfink] from comment #3)
> > JS_ValueToECMAUint32 I have mixed feelings about. I would need to
> > verify this, but I think it's one of those things that can only GC
> > if it also ends up returning false.
> 
> Not so; for example, the value might be this:
> 
>   ({ valueOf: function() { gc(); return 12; } })
> 
> The result of the conversion would of course successfully be 12.

Ah. Clearly I've been hanging out too much in ToNumber() (with full knowledge that I am a primitive, even.)

I need to spend more time where you crazy people live, with recursive JS invocations and things.

Anyway, JS_ValueToECMAUint32 just calls ToUint32, which already has AssertCanGC(). (The test I was using never passed in the argument that caused it to be called.)

Comment 6

[Terrence Cole [:terrence]]

(In reply to Steve Fink [:sfink] from comment #3)
> > Bill changed DumpHeap recently so that it's not callable from GC anymore:
> > could we add an AssertCanGC() to the top of DumpHeap and
> > JS_ValueToECMAUint32 since it seems like these could be troublesome in other
> > cases as well?
> 
> Um. DumpHeap, sure. JS_ValueToECMAUint32 I have mixed feelings about. I
> would need to verify this, but I think it's one of those things that can
> only GC if it also ends up returning false. Usually when something that
> takes a cx returns false then all of the stack-allocated variables are dead
> anyway because the caller is just going to return.

I have repeatedly wanted to express exactly this concept.  We should find a way to make this expressable with C++ types.

Comment 7

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/ef8010af9fe6

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ef8010af9fe6

Comment 9

[Nicholas Nethercote [inactive]]

> If only I could push this to try test test rooting analysis there...

Just include the following patch in your pushed patch set:


diff --git a/configure.in b/configure.in
--- a/configure.in
+++ b/configure.in
@@ -1641,16 +1641,17 @@ fi

 dnl ========================================================
 dnl = Perform moving GC stack rooting analysis
 dnl ========================================================
 MOZ_ARG_ENABLE_BOOL(root-analysis,
 [  --enable-root-analysis  Enable moving GC stack root analysis],
     JSGC_ROOT_ANALYSIS=1,
     JSGC_ROOT_ANALYSIS= )
+JSGC_ROOT_ANALYSIS=1
 if test -n "$JSGC_ROOT_ANALYSIS"; then
     AC_DEFINE(JSGC_ROOT_ANALYSIS)
 fi

 dnl ========================================================
 dnl = Use exact stack rooting for GC
 dnl ========================================================
 MOZ_ARG_ENABLE_BOOL(exact-rooting,

Comment 10

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

Well, I was somewhat kidding, since I do know how to do a custom try push that would run the rooting analysis. In fact, I implemented common mozconfigs to make this easier -- you can push with a custom build/mozconfig.common that will get included by all platforms. I really should find a good place to document that. (Though in this case you could edit just the linux64-debug mozconfig and only select that platform.)

But note that your above solution is incomplete -- that would compile in the rooting analysis, but it wouldn't actually be active during the tests for a series of reasons:

1. the regular builds use --enable-threadsafe, which disables the actual checks
2. you need to set the environment variable JS_GC_ZEAL=6 during the tests
3. the JS shell will only run the jit-tests, not the jstests. The jstests are run as a separate test job under the browser, but the browser of course won't work if rooting analysis is on.

So... it would be much nicer if we could just request rootanalysis try builds. I have patches that I *think* will do that, awaiting review.