Closed Bug 793577 Opened 7 years ago Closed 7 years ago

Create a new type for unrooted return values

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: njn, Assigned: terrence)

References

Details

(Whiteboard: [js:t])

Attachments

(1 file, 5 obsolete files)

We want to eventually remove all uses of naked GC pointer types, such as |JSObject *|.  So we need to do something with return values.  sfink doesn't want to use |Raw<T>| for return values (bug 793086 comment 3), so we should create a new type.  That type would start out just being a typedef for the underlying type, but it could eventually become a wrapper class that might do some checking or prevent dangerous type-coercions or similar.

Any suggestions for names?  My best idea so far is simply |Return<T>|.
There's also the question of whether this would be an internal-only name, or if we could use it in API functions as well.  I'd prefer the latter, just so that grepping for e.g. |JSObject *| is a useful gauge of how far we have to go before exact rooting is complete.
This should be internal only.  We are going to remove all direct rooting concerns from API users by using HandleScopes.  This will allow us to return Handle<T> directly from the API, possibly renamed to Local<T> to better match V8's naming and make the correct usage clearer.
The error we are hitting is:
Assertion failure: TlsRuntime.get()->gcAssertNoGCDepth >= 0, at e:/builds/moz2_slave/try-w32-dbg/build/js/src/jsapi.cpp:699

This would indicate that the guard object destructor is firing too many times in Windows?!?
Assignee: general → terrence
Status: NEW → ASSIGNED
Attached patch v0: Implementation of Return<T> (obsolete) — Splinter Review
I'm splitting this review in two parts.  Part 1 is for the Return<T> class implementation alone, to get some focused GC and C++ integration feedback.
Attachment #669660 - Flags: review?(wmccloskey)
Attached patch v0: green on try (obsolete) — Splinter Review
Part 2 is the usage of Return<T> to fix the currently-broken HandleScript return out of JSFunction and the frame.
Attachment #669179 - Attachment is obsolete: true
Attachment #669662 - Flags: review?(n.nethercote)
Comment on attachment 669662 [details] [diff] [review]
v0: green on try

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

I've only skimmed the patch so far... there are some things I'm uncertain about.

You mentioned this in bug 794667 comment 28 the distinction between operator->() and get().  I'd really like a comment about that.  Actually, they look like they're identical -- am I missing something?

Are all the new assertions required to avoid runtime failures?  Some of the AutoAssertNoGCs seem unnecessary.

::: js/src/jsapi.cpp
@@ +685,5 @@
>  JS_FRIEND_API(void)
>  EnterAssertNoGCScope()
>  {
> +    ++cnt;
> +    //fprintf(stderr, "UP: %lu\n", cnt);

Debugging code -- remove?

::: js/src/jsfun.h
@@ +131,5 @@
>  
>      static inline size_t offsetOfEnvironment() { return offsetof(JSFunction, u.i.env_); }
>      static inline size_t offsetOfAtom() { return offsetof(JSFunction, atom_); }
>  
> +    js::Return<JSScript*> script() const {

You should create a ReturnScript typedef, and probably a few others (ReturnObject, etc), and use them throughout.

::: js/src/vm/Stack.h
@@ +327,5 @@
> +    }
> +    Value *actuals() const {
> +        AutoAssertNoGC nogc;
> +        return formals() - (flags_ & OVERFLOW_ARGS ? 2 + u.nactual : 0);
> +    }

This is an example of an AutoAssertNoGC that seems unnecessary -- it's in a function with no arguments, in a class with no |cx| or |rt| member.  Is it required because of the return value somehow?
(In reply to Nicholas Nethercote [:njn] from comment #7)
> 
> I've only skimmed the patch so far... there are some things I'm uncertain
> about.

Thanks for taking the review!  There is a fair bit of subtlety here, so I really appreciate you taking the time to understand it all.
 
> You mentioned this in bug 794667 comment 28 the distinction between
> operator->() and get().  I'd really like a comment about that.  Actually,
> they look like they're identical -- am I missing something?

|operator->| and |get| make the same assertion, but they handle different use cases, as does |operator T|.  Consider the following snippets:

{
    AutoAssertNoGC nogc;
    RawScript script = fun->script(); // Whatevah, I can do what a want!
    // The implicit |operator T| here won't outlive the |nogc| even if 
    // we store it into an unguarded form. (Assuming no RawScript are
    // returned in the rest of the program.)
}

{
    RawScript script = fun->script(); // KABOOM!
    // We have no guarantee that script will be safe after it escapes Return<T>.
}

{
    RawScript script = fun->script().get(); // I know and will take care.
    // If only we could make gcc hand the user a contract at this point....
    // Currently, we don't do this anywhere and hopefully we won't have to.
}

{
    JS_ASSERT(myScript == fun->script().get()); // A more practical example.
    // I added |operator==| because these are so common, so a better way
    // to write this would be:
    JS_ASSERT(fun->script() == myScript);
}

{
    bool isEval = fun->script()->isForEval(); // This is also safe.
    // |isForEval| will not GC, so the RawScript that |operator->| puts on the
    // stack will not be used dangerously.
}

{
    fun->script()->bindings = myBindings->clone(cx, ...);  // KABOOM!
    // Our assertion saves us from a rare crasher in this case:
    // |fun->script()::operator->| could run before |clone|, be corrupted
    // on the stack by a GC under |clone|, then the lookup for
    // |RawScript::bindings| on the corrupt RawScript would crash.
}

{
    bool status = fun->script()->ensureRanAnalysis(cx); // ASSERT!
    // This is basically safe.  |ensureRanAnalysis| may GC, but the script
    // is not used again, so this is okayish.  I chose to assert zealously,
    // since C++ does not give us the tools to be more subtle here.
}

{
    RootedScript fscript(cx, fun->script());
    bool status = fscript->ensureRanAnalysis(cx); // The right way.
    // This avoids the assertion by properly rooting the script on the stack.
}

There are probably some other cases, but I think these are the common and important ones.  I tried to draw a fair line between safety and usability and I think this roughly hits the mark while staying within the lines of well-defined C++.  As a bonus, these tools are all relatively efficient in C++, so we don't have to worry too much about aiming for good coverage.

** Caveat: these examples all assume good coverage of SpiderMonkey with AssertCanGC() to trigger the temporary scoped guards.

> Are all the new assertions required to avoid runtime failures?  Some of the
> AutoAssertNoGCs seem unnecessary.
> 
> ::: js/src/jsapi.cpp
> @@ +685,5 @@
> >  JS_FRIEND_API(void)
> >  EnterAssertNoGCScope()
> >  {
> > +    ++cnt;
> > +    //fprintf(stderr, "UP: %lu\n", cnt);
> 
> Debugging code -- remove?

Doh!  It was very helpful, but it certainly shouldn't be in here now!

> ::: js/src/jsfun.h
> @@ +131,5 @@
> >  
> >      static inline size_t offsetOfEnvironment() { return offsetof(JSFunction, u.i.env_); }
> >      static inline size_t offsetOfAtom() { return offsetof(JSFunction, atom_); }
> >  
> > +    js::Return<JSScript*> script() const {
> 
> You should create a ReturnScript typedef, and probably a few others
> (ReturnObject, etc), and use them throughout.

Fair enough.

> ::: js/src/vm/Stack.h
> @@ +327,5 @@
> > +    }
> > +    Value *actuals() const {
> > +        AutoAssertNoGC nogc;
> > +        return formals() - (flags_ & OVERFLOW_ARGS ? 2 + u.nactual : 0);
> > +    }
> 
> This is an example of an AutoAssertNoGC that seems unnecessary -- it's in a
> function with no arguments, in a class with no |cx| or |rt| member.  Is it
> required because of the return value somehow?

I may have been a bit over-zealous with my new tools.  Feel free to point out places that are over-the-top and I will roll it back a bit.
Thanks for the detailed examples, that helps a lot.  I haven't yet gone through the patch, but here are some high level suggestions that would clarify things.


We need a better comment on AssertCanGC().  In particular the fact that it's
present in cx->malloc() and GC() (and similar places) and so will catch any
case where those are triggered.  And therefore it's worth adding more of 
them to functions like ToNumber() where allocation/GC are rare -- that will
convert rare assertion failures (e.g. if you call ToNumber() in a NoGC
scope) into common assertion failures.


Reorder the operations in Return<T>, and add comments explaining the following.

- |operator T| is the safest.  It will assert if not called in a NoGC context,
  such as this:

    RawScript script = fun->script();

- |operator->| is fairly safe.  Its result cannot be stored in a local
  variable, and it will assert if a CanGC context is encountered before the
  next |;|.  For example:

    fun->script()->bindings = myBindings->clone(cx, ...);

  This could crash in the following case: |fun->script()::operator->| could
  run before |clone|, be corrupted on the stack by a GC under |clone|, then
  the lookup for |RawScript::bindings| on the corrupt RawScript would crash.
  But, assuming that the call to clone() hits an AssertCanGC(), this will
  assert.

  The right way to write this is:

    RootedScript clone = myBindings->clone(cx, ...);
    fun->script()->bindings = clone;

- get() is mostly unsafe.  Its result can be stored in a local variable, and
  it has similar checking to |operator->| but that's unlikely to help much
  in practice.

- operator== is completely safe.  It allows this common pattern:

    JS_ASSERT(myScript == fun->script().get()); 

  To instead be written like this:

    JS_ASSERT(fun->script() == myScript);


(Please let me know if I've got any of the above wrong.)
Just a note: cx->malloc cannot GC.
> Just a note: cx->malloc cannot GC.

Yes it can.  JSContext::malloc_ calls JSRuntime::malloc_ which calls JSRuntime::updateMallocCounter() which calls JSCompartment::updateMallocCounter() which can call JSCompartment::onTooMuchMalloc() which calls TriggerCompartmentGC().
TriggerCompartmentGC() schedules a GC to happen in the future via the operation callback. It doesn't actually do a GC.
(In reply to Nicholas Nethercote [:njn] from comment #9)
> ...snip... 
> (Please let me know if I've got any of the above wrong.)

Other than cx->malloc triggering GC, that's all right on.
Ok, so I was loose with my language.  But from the point of view of someone who cares about rooting and the possibility of a GC occurring and moving GC things around, "schedules a GC to happen in the future" is just as dangerous as "triggers a GC right now", correct?
I don't think I follow.  MaybeGC is the only place that checks |rt->gcIsNeeded|, so I don't think setting it actually changes our rooting concerns: we already need to be rooting-safe anywhere that would call MaybeGC.
Bill explained to me on IRC that a scheduled GC can't happen without calling into the interpreter or the JIT.
Comment on attachment 669662 [details] [diff] [review]
v0: green on try

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

Nice job.  I think you've struck a nice balance between additional safety and convenience.  I'll be interested to try converting some existing return types to Return<T> to see how it feels in practice.

I'm looking forward to seeing the expanded comments on Return when this lands :)

It would have been nice to have this split in two:  one part containing the script() return type change, plus all the additional asserts that were required;  and the second part containing optional additional asserts.  Something to consider for next time.

::: js/src/gc/Root.h
@@ +387,5 @@
> +        return ptr_;
> +    }
> +
> +    /*
> +     * Note: we do not disallow operator== because there is no [useful] way to

Nit: square brackets are odd.

::: js/src/ion/Bailouts.cpp
@@ +74,5 @@
>      }
>  }
>  
>  static JSScript*
>  GetBailedJSScript(JSContext *cx)

I'd be tempted to change this so you pass in |cx->runtime->ionTop| instead of |cx|.

::: js/src/ion/CodeGenerator.cpp
@@ +307,5 @@
>      u.s.flags = fun->flags & ~JSFUN_EXTENDED;
>  
>      JS_STATIC_ASSERT(offsetof(JSFunction, flags) == offsetof(JSFunction, nargs) + 2);
>      masm.store32(Imm32(u.word), Address(output, offsetof(JSFunction, nargs)));
> +    masm.storePtr(ImmGCPtr(fun->script().get()), Address(output, JSFunction::offsetOfNativeOrScript()));

Hmm, that looks dangerous -- embedding a GC thing point into generated code :(

::: js/src/ion/Ion.cpp
@@ +910,5 @@
>  
>  void
>  AttachFinishedCompilations(JSContext *cx)
>  {
> +    AssertCanGC();

I assume it's safe to put AssertCanGC() in a function that cannot trigger GC?  A lot of the functions where you're adding them I can't quite see why they can GC, but if it's safe then overpromising isn't a problem...

@@ +1559,5 @@
>          // See if the frame has already been invalidated.
>          if (it.checkInvalidation())
>              continue;
>  
> +        RawScript script = it.script();

it.script() is used in various places higher up this function;  you could move the creation of |script| to the top of the for-loop if you wanted.

::: js/src/ion/IonFrames-inl.h
@@ +83,5 @@
>  // Returns the JSScript associated with the topmost Ion frame.
>  inline JSScript *
>  GetTopIonJSScript(JSContext *cx, const SafepointIndex **safepointIndexOut, void **returnAddrOut)
>  {
> +    AutoAssertNoGC nogc;

Again, could pass in |cx->runtime->ionTop| instead of |cx| here.

::: js/src/ion/IonFrames.cpp
@@ +33,5 @@
>      switch (GetCalleeTokenTag(token)) {
>        case CalleeToken_Script:
>          return CalleeTokenToScript(token);
>        case CalleeToken_Function:
> +        return CalleeTokenToFunction(token)->script().get();

Should you also change the return type to ReturnScript?

@@ +171,5 @@
>  
>  JSScript *
>  IonFrameIterator::script() const
>  {
> +    AutoAssertNoGC nogc;

Ditto here?

@@ +337,5 @@
>                  // When profiling, each frame popped needs a notification that
>                  // the function has exited, so invoke the probe that a function
>                  // is exiting.
> +                AutoAssertNoGC nogc;
> +                RawScript script = frames.script();

At the top of this function you have AssertCanGC().  Surely it's not allowed to nest AutoAssertNoGC within that?

::: js/src/ion/shared/CodeGenerator-shared.h
@@ +509,5 @@
>  template <class ArgSeq, class StoreOutputTo>
>  bool
>  CodeGeneratorShared::visitOutOfLineCallVM(OutOfLineCallVM<ArgSeq, StoreOutputTo> *ool)
>  {
> +    AssertCanGC();

I can't see how this can GC, though I may have missed something.

::: js/src/jscntxt.cpp
@@ +554,5 @@
>   */
>  void
>  js_ReportOutOfMemory(JSContext *cx)
>  {
> +    AutoAssertNoGC nogc;

Is this wrong?  I think the JSDebugErrorHook might be able to GC.

::: js/src/jsgc.cpp
@@ +4493,5 @@
>   */
>  static JS_NEVER_INLINE void
>  GCCycle(JSRuntime *rt, bool incremental, int64_t budget, JSGCInvocationKind gckind, gcreason::Reason reason)
>  {
> +    AutoAssertNoGC nogc;

Um... I'm not sure how to interpret this.  Doesn't GCCycle invoke the GC?

::: js/src/jsinterp.cpp
@@ +2474,5 @@
>          goto error;
>  
>      InitialFrameFlags initial = construct ? INITIAL_CONSTRUCT : INITIAL_NONE;
>      bool newType = cx->typeInferenceEnabled() && UseNewType(cx, script, regs.pc);
> +    RawScript newScript = fun->script().unsafeGet();

Just inline the one occurrence?

::: js/src/jsinterp.h
@@ +269,5 @@
>  
>  class TryNoteIter
>  {
>      const FrameRegs &regs;
> +    RootedScript script;

Comment that this is safe because objects of this class are always stack-allocated.

::: js/src/jsobj.cpp
@@ +5539,3 @@
>              sprinter.printf("#%d %14p   %s:%d (%p @ %d)\n",
>                              depth, (i.isIon() ? 0 : i.fp()), filename, line,
> +                            script, i.pc() - i.script()->code);

Change the second |i.script()| to |script|.

::: js/src/jsxml.cpp
@@ +1752,5 @@
>      if (!i.done()) {
>          op = (JSOp) *i.pc();
>          if (op == JSOP_TOXML || op == JSOP_TOXMLLIST) {
> +            filename = i.script().unsafeGet()->filename;
> +            lineno = PCToLineNumber(i.script().unsafeGet(), i.pc());

LOL.  Can we remove jsxml.cpp yet?

::: js/src/methodjit/MethodJIT.cpp
@@ +1115,5 @@
>  #if JS_TRACE_LOGGING
>      AutoTraceLog logger(TraceLogging::defaultLogger(),
>                          TraceLogging::JM_START,
>                          TraceLogging::JM_STOP,
> +                        fp->script().unsafeGet());

Why not get()?

@@ +1128,5 @@
>  #if JS_TRACE_LOGGING
>      AutoTraceLog logger(TraceLogging::defaultLogger(),
>                          TraceLogging::JM_SAFEPOINT_START,
>                          TraceLogging::JM_SAFEPOINT_STOP,
> +                        cx->fp()->script().unsafeGet());

Ditto.

::: js/src/methodjit/MonoIC.cpp
@@ +103,5 @@
>  DisabledSetGlobal(VMFrame &f, ic::SetGlobalNameIC *ic)
>  {
> +    AssertCanGC();
> +    RootedPropertyName name(f.cx, f.script()->getName(GET_UINT32_INDEX(f.pc())));
> +    stubs::SetName(f, name);

I don't see why you had to introduce the |name| local here.

::: js/src/shell/js.cpp
@@ +1382,5 @@
>      ScriptFrameIter iter(cx);
>      JS_ASSERT(!iter.done());
>  
>      JSStackFrame *caller = Jsvalify(iter.fp());
> +    JSScript *script = iter.script().get();

Change to RawScript.

@@ +1765,5 @@
>              JSObject *obj = objects->vector[i];
>              if (obj->isFunction()) {
>                  Sprint(sp, "\n");
> +                RootedFunction fun(cx, obj->toFunction());
> +                RootedScript nested(cx, fun->maybeScript());

Why root these?  They can obviously be Raw.

::: js/src/vm/GlobalObject.cpp
@@ +295,4 @@
>                                           NULL, 0, JSFUN_INTERPRETED, self, NullPtr());
>          if (!proto)
>              return NULL;
>          JS_ASSERT(proto == functionProto);

Might be worth giving these four lines their own scope, to make the lifetime of |proto| clearer?  There are lots of lines after this point in this function.  You could even do the same with |functionProto| higher up.

@@ +424,5 @@
>          return NULL;
>      }
>  
>      /* Add the global Function and Object properties now. */
> +    RawId objectId = NameToId(cx->names().Object);

I usually just inline one-shot locals like this, to save having to think about the local variable type.

@@ +429,3 @@
>      if (!self->addDataProperty(cx, objectId, JSProto_Object + JSProto_LIMIT * 2, 0))
>          return NULL;
> +    RawId functionId = NameToId(cx->names().Function);

Ditto.

@@ +434,5 @@
>  
>      /* Heavy lifting done, but lingering tasks remain. */
>  
>      /* ES5 15.1.2.1. */
>      RootedId id(cx, NameToId(cx->names().eval));

Ditto (I realize this one precedes your patch;  the rooting is unnecessary).

@@ +473,5 @@
>       * Notify any debuggers about the creation of the script for
>       * |Function.prototype| -- after all initialization, for simplicity.
>       */
> +    RootedScript functionProtoScript(cx, functionProto->script());
> +    js_CallNewScriptHook(cx, functionProtoScript, functionProto);

I would have expected you to just change the 2nd arg to |functionProto->script().get()|.

::: js/src/vm/Stack.cpp
@@ +243,5 @@
>                  JS_ASSERT(i.block() == scope->asClonedBlock().staticBlock());
>                  scope = &scope->asClonedBlock().enclosingScope();
>                  break;
>                case StaticScopeIter::FUNCTION:
> +                JS_ASSERT(i.funScript() == scope->asCall().callee().script().get());

Doesn't the operator== avoid the need for the |.get()| here?

::: js/src/vm/Stack.h
@@ +315,5 @@
>    public:
> +    Value *slots() const {
> +        AutoAssertNoGC nogc;
> +        return (Value *)(this + 1);
> +    }

The NoGC seems overzealous.

@@ +323,5 @@
> +    }
> +    Value *formals() const {
> +        AutoAssertNoGC nogc;
> +        return (Value *)this - fun()->nargs;
> +    }

Ditto.

@@ +327,5 @@
> +    }
> +    Value *actuals() const {
> +        AutoAssertNoGC nogc;
> +        return formals() - (flags_ & OVERFLOW_ARGS ? 2 + u.nactual : 0);
> +    }

Ditto.

@@ +625,4 @@
>          return isFunctionFrame()
>                 ? isEvalFrame()
> +                 ? u.evalScript
> +                 : fun()->script().unsafeGet()

Why not get()?
Attachment #669662 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #17)
> Nice job.  I think you've struck a nice balance between additional safety
> and convenience.  I'll be interested to try converting some existing return
> types to Return<T> to see how it feels in practice.

\o/  Thanks for the thorough review: excellent comments all around!
 
> ::: js/src/ion/CodeGenerator.cpp
> @@ +307,5 @@
> >      u.s.flags = fun->flags & ~JSFUN_EXTENDED;
> >  
> >      JS_STATIC_ASSERT(offsetof(JSFunction, flags) == offsetof(JSFunction, nargs) + 2);
> >      masm.store32(Imm32(u.word), Address(output, offsetof(JSFunction, nargs)));
> > +    masm.storePtr(ImmGCPtr(fun->script().get()), Address(output, JSFunction::offsetOfNativeOrScript()));
> 
> Hmm, that looks dangerous -- embedding a GC thing point into generated code
> :(

^^ This.  It is vital for performance to be able to bake into generated code basically everything that hangs off the script.  This is why we have no near-term plans to be able to move scripts and when we do, we will almost certainly still have to flush all compiled code to do so.
 
> ::: js/src/ion/Ion.cpp
> @@ +910,5 @@
> >  
> >  void
> >  AttachFinishedCompilations(JSContext *cx)
> >  {
> > +    AssertCanGC();
> 
> I assume it's safe to put AssertCanGC() in a function that cannot trigger
> GC?  A lot of the functions where you're adding them I can't quite see why
> they can GC, but if it's safe then overpromising isn't a problem...

Yes, and I took exactly the opposite approach when writing this patch.  :-)  If you can put an AutoAssertNoGC at the top of a method, you don't have to touch anything else, so this /seems/ like the "easy" approach.

Unfortunately, this resulted in my spending ~4 tedious hours locally and ~8 try runs finding spots where I was over-protective with this assertion and rolling it back.  For each assertion failure you have to grab a stacktrace, replace the AssertNoGC with an AssertCanGC, fix up all local uses of script(), then try to find all callers and repeat the process for each of them.  All of the AssertCanGC() -- in particular the ones that are not obviously necessary -- are there because I found them the hard way.

It would have been much easier to just splat AssertCanGC everywhere, but this process ends up getting a much more accurate fit of what can and cannot GC.  I'll actually be a bit sad if we make exception and error reporting GC-less because it will invalidate much of this work.  Maybe I'll work on that next so that we can minimize our later pain.

> ::: js/src/ion/IonFrames.cpp
> @@ +33,5 @@
> >      switch (GetCalleeTokenTag(token)) {
> >        case CalleeToken_Script:
> >          return CalleeTokenToScript(token);
> >        case CalleeToken_Function:
> > +        return CalleeTokenToFunction(token)->script().get();
> 
> Should you also change the return type to ReturnScript?
>
> @@ +171,5 @@
> >  
> >  JSScript *
> >  IonFrameIterator::script() const
> >  {
> > +    AutoAssertNoGC nogc;
> 
> Ditto here?

I considered it, but you have to stop somewhere.  I'm not really sure how much work it would be, but given that this is already a 200k patch....
 
> @@ +337,5 @@
> >                  // When profiling, each frame popped needs a notification that
> >                  // the function has exited, so invoke the probe that a function
> >                  // is exiting.
> > +                AutoAssertNoGC nogc;
> > +                RawScript script = frames.script();
> 
> At the top of this function you have AssertCanGC().  Surely it's not allowed
> to nest AutoAssertNoGC within that?

Nope, its exactly the opposite way.  At the top of every Invoke or Evaluate there is an AssertCanGC.  Within the context of the running script, there are (generally small) sub-regions of execution which we know statically cannot GC.  This particular block is one of those regions.  I suppose that most NoGC regions are their own function, but there is no real reason for that other than convenience.

On the other hand, if you are in an AutoAssertNoGC region, then doing a GC would be super bad.  Thus, if you enter a CanGC scope while you are in a NoGC scope, then that is transitively just as bad as triggering a full GC on the spot.
 
[Aside: Naturally there are many, many more blocks of C++ which staticly could GC but in practice never will for dynamic reasons.  E.g. in str_contains, if we never get passed a non-int32 position then dynamically it could effectively be an AutoAssertNoGC block.]

> ::: js/src/ion/shared/CodeGenerator-shared.h
> @@ +509,5 @@
> >  template <class ArgSeq, class StoreOutputTo>
> >  bool
> >  CodeGeneratorShared::visitOutOfLineCallVM(OutOfLineCallVM<ArgSeq, StoreOutputTo> *ool)
> >  {
> > +    AssertCanGC();
> 
> I can't see how this can GC, though I may have missed something.

I couldn't either initially, but the test suite found this call path:
CodeGeneratorShared::visitOutOfLineCallVM
CodeGeneratorShared::callVM
IonCompartment::generateVMWrapper
Linker::newCode
IonCode::New
gc::newGCThing<IonCode>
 
> ::: js/src/jscntxt.cpp
> @@ +554,5 @@
> >   */
> >  void
> >  js_ReportOutOfMemory(JSContext *cx)
> >  {
> > +    AutoAssertNoGC nogc;
> 
> Is this wrong?  I think the JSDebugErrorHook might be able to GC.

There are no uses of this hook by the browser.  I think that making this function able to GC would be extremely annoying, because it would effectively make cx/rt->malloc able to GC where they are not currently.  If anything I think we should remove the cx arg from the hook.

> ::: js/src/jsgc.cpp
> @@ +4493,5 @@
> >   */
> >  static JS_NEVER_INLINE void
> >  GCCycle(JSRuntime *rt, bool incremental, int64_t budget, JSGCInvocationKind gckind, gcreason::Reason reason)
> >  {
> > +    AutoAssertNoGC nogc;
> 
> Um... I'm not sure how to interpret this.  Doesn't GCCycle invoke the GC?

GCCycle /is/ the GC.  If we enter a CanGC block once we are inside the GC you would be triggering a GC from inside the GC.  This would be bad, so we put an AutoAssertNoGC block here to make sure that doesn't happen.

> ::: js/src/jsxml.cpp
> @@ +1752,5 @@
> >      if (!i.done()) {
> >          op = (JSOp) *i.pc();
> >          if (op == JSOP_TOXML || op == JSOP_TOXMLLIST) {
> > +            filename = i.script().unsafeGet()->filename;
> > +            lineno = PCToLineNumber(i.script().unsafeGet(), i.pc());
> 
> LOL.  Can we remove jsxml.cpp yet?

Almost!
 
> ::: js/src/methodjit/MethodJIT.cpp
> @@ +1115,5 @@
> >  #if JS_TRACE_LOGGING
> >      AutoTraceLog logger(TraceLogging::defaultLogger(),
> >                          TraceLogging::JM_START,
> >                          TraceLogging::JM_STOP,
> > +                        fp->script().unsafeGet());

Because figuring out whether its safe or not is brain-cycles I will never get back.  It's E4X, so as long as it still compiles for another few weeks we're golden.
 
> ::: js/src/methodjit/MonoIC.cpp
> @@ +103,5 @@
> >  DisabledSetGlobal(VMFrame &f, ic::SetGlobalNameIC *ic)
> >  {
> > +    AssertCanGC();
> > +    RootedPropertyName name(f.cx, f.script()->getName(GET_UINT32_INDEX(f.pc())));
> > +    stubs::SetName(f, name);
> 
> I don't see why you had to introduce the |name| local here.

stubs::SetName can GC.  I found this out because the temporary created by |operator->| caused an assertion when we tried to allocate.  This is one of the few cases I found where it would be safe to use |unsafeGet|: C++ actually guarantees eager argument evaluation.

> ::: js/src/shell/js.cpp
> @@ +1382,5 @@
> >      ScriptFrameIter iter(cx);
> >      JS_ASSERT(!iter.done());
> >  
> >      JSStackFrame *caller = Jsvalify(iter.fp());
> > +    JSScript *script = iter.script().get();
> 
> Change to RawScript.

Ah, good catch.

> @@ +1765,5 @@
> >              JSObject *obj = objects->vector[i];
> >              if (obj->isFunction()) {
> >                  Sprint(sp, "\n");
> > +                RootedFunction fun(cx, obj->toFunction());
> > +                RootedScript nested(cx, fun->maybeScript());
> 
> Why root these?  They can obviously be Raw.

Well, someone rooted a script at the top of this method and this block recurses.  Now that I'm looking more closely, it looks as if this function ends up in the old disassembler, so I wonder if this code is even reachable anymore?

> ::: js/src/vm/GlobalObject.cpp
> @@ +295,4 @@
> >                                           NULL, 0, JSFUN_INTERPRETED, self, NullPtr());
> >          if (!proto)
> >              return NULL;
> >          JS_ASSERT(proto == functionProto);
> 
> Might be worth giving these four lines their own scope, to make the lifetime
> of |proto| clearer?  There are lots of lines after this point in this
> function.  You could even do the same with |functionProto| higher up.

Good idea!

> @@ +434,5 @@
> >  
> >      /* Heavy lifting done, but lingering tasks remain. */
> >  
> >      /* ES5 15.1.2.1. */
> >      RootedId id(cx, NameToId(cx->names().eval));
> 
> Ditto (I realize this one precedes your patch;  the rooting is unnecessary).
> 
> @@ +473,5 @@
> >       * Notify any debuggers about the creation of the script for
> >       * |Function.prototype| -- after all initialization, for simplicity.
> >       */
> > +    RootedScript functionProtoScript(cx, functionProto->script());
> > +    js_CallNewScriptHook(cx, functionProtoScript, functionProto);
> 
> I would have expected you to just change the 2nd arg to
> |functionProto->script().get()|.

js_CallNewScriptHook can get into arbitrary code, right?  If so, then having the temporary returned by |get| still live would not be safe.
 
> ::: js/src/vm/Stack.h
> @@ +315,5 @@
> >    public:
> > +    Value *slots() const {
> > +        AutoAssertNoGC nogc;
> > +        return (Value *)(this + 1);
> > +    }
> 
> The NoGC seems overzealous.

Yup.  I'll pull these out.
 
> @@ +625,4 @@
> >          return isFunctionFrame()
> >                 ? isEvalFrame()
> > +                 ? u.evalScript
> > +                 : fun()->script().unsafeGet()
> 
> Why not get()?

g++ was not able to properly extract the type through the IntermediateNoGC indirection in this case.  I'll try |get()| with a manual cast.
Attachment #669660 - Attachment is obsolete: true
Attachment #669660 - Flags: review?(wmccloskey)
Attachment #670484 - Flags: review?(wmccloskey)
Attachment #670484 - Flags: feedback?(n.nethercote)
Comment on attachment 670484 [details] [diff] [review]
v1: gc/Root.h implementation of Return<T> alone

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

Have you tried all the examples given in the documentation to ensure that they actually assert?

::: js/src/gc/Root.h
@@ +342,5 @@
> +};
> +#endif
> +
> +/*
> + * Return<T> wraps GC-Pointers that are returned from accessor methods.  The

GC-Pointers -> GC things

@@ +352,5 @@
> + *     class Foo {
> + *         HeapPtrScript script_;
> + *         ...
> + *       public:
> + *          ReturnScript script() { return script_; }

In the context of the example, Return<JSScript *> seems clearer to me.

@@ +355,5 @@
> + *       public:
> + *          ReturnScript script() { return script_; }
> + *     };
> + *
> + * Example Usage of method:

Capitalization is weird.

@@ +358,5 @@
> + *
> + * Example Usage of method:
> + *
> + *     Foo foo(...);
> + *     RootedScript script(cx, foo->script());

You might want to provide an example of an error that would be caught by this class.

@@ +427,5 @@
> +     * reason, it should only be used when it would be incorrect or absurd to
> +     * create a new Rooted for its use: e.g. for assertions.
> +     */
> +#ifdef DEBUG
> +    IntermediateNoGC<T> get() const {

I'd prefer this be called unsafeGet. That's the name we use for a number of other such methods where you're saying "it's okay, I know what I'm doing."
Attachment #670484 - Flags: review?(wmccloskey) → review+
> ^^ This.  It is vital for performance to be able to bake into generated code
> basically everything that hangs off the script.  This is why we have no
> near-term plans to be able to move scripts and when we do, we will almost
> certainly still have to flush all compiled code to do so.

So scripts will have to go straight into the tenured generation once we have GenGC?  And then they won't be able to be compacted?  Hmm.


> > Should you also change the return type to ReturnScript?
> 
> I considered it, but you have to stop somewhere.  I'm not really sure how
> much work it would be, but given that this is already a 200k patch....

Yeah, fair enough.


> > Is this wrong?  I think the JSDebugErrorHook might be able to GC.
> 
> There are no uses of this hook by the browser.  I think that making this
> function able to GC would be extremely annoying, because it would
> effectively make cx/rt->malloc able to GC where they are not currently.  If
> anything I think we should remove the cx arg from the hook.

I'm all for that.  Follow-up bug?


> > Um... I'm not sure how to interpret this.  Doesn't GCCycle invoke the GC?
> 
> GCCycle /is/ the GC.  If we enter a CanGC block once we are inside the GC
> you would be triggering a GC from inside the GC.  This would be bad, so we
> put an AutoAssertNoGC block here to make sure that doesn't happen.

Ok -- a comment explaining this would be nice :)


> > ::: js/src/methodjit/MethodJIT.cpp
> > @@ +1115,5 @@
> > >  #if JS_TRACE_LOGGING
> > >      AutoTraceLog logger(TraceLogging::defaultLogger(),
> > >                          TraceLogging::JM_START,
> > >                          TraceLogging::JM_STOP,
> > > +                        fp->script().unsafeGet());
> 
> Because figuring out whether its safe or not is brain-cycles I will never
> get back.  It's E4X, so as long as it still compiles for another few weeks
> we're golden.

I don't think that's E4X...


> > @@ +473,5 @@
> > >       * Notify any debuggers about the creation of the script for
> > >       * |Function.prototype| -- after all initialization, for simplicity.
> > >       */
> > > +    RootedScript functionProtoScript(cx, functionProto->script());
> > > +    js_CallNewScriptHook(cx, functionProtoScript, functionProto);
> > 
> > I would have expected you to just change the 2nd arg to
> > |functionProto->script().get()|.
> 
> js_CallNewScriptHook can get into arbitrary code, right?  If so, then having
> the temporary returned by |get| still live would not be safe.

I don't understand.  |functionProto->script().get()| would fully evaluate before js_CallNewScriptHook would be called.  Or is this a case where the checking is a bit overzealous and we have to work around it?
> I'd prefer this be called unsafeGet. That's the name we use for a number of
> other such methods where you're saying "it's okay, I know what I'm doing."

And the existing unsafeGet can become reallyUnsafeGet :P
Comment on attachment 670484 [details] [diff] [review]
v1: gc/Root.h implementation of Return<T> alone

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

::: js/src/gc/Root.h
@@ +378,5 @@
> +    Return(NullPtr) : ptr_(NULL) {}
> +
> +    /*
> +     * |operator const T &| is the safest way to access a Return<T>; however, it
> +     * can only be used when in a NoGC scope.

It's the safest way to access a Return<T> *without rooting it*.

Also, maybe mention that it'll assert if not used in a NoGC scope?

@@ +441,5 @@
> +     * |operator==| is safe to use in any context.  It is present to allow:
> +     *     JS_ASSERT(myScript == fun->script().get());
> +     *
> +     * To be rewritten as:
> +     *     JS_ASSERT(fun->script() == myScript);

My initial thought was "why'd you change the operand order in the second example?"  Now I see why, but it might be worth explaining briefly afterwards?
Attachment #670484 - Flags: feedback?(n.nethercote) → feedback+
Attached patch v2 (obsolete) — Splinter Review
Gary, this is going to need some fuzzing before I'm confident it's ready for checkin.

(In reply to Bill McCloskey (:billm) from comment #20)
> Have you tried all the examples given in the documentation to ensure that
> they actually assert?

Oh my, yes!  This patch required a whole bunch of testing to get all the AssertCanGC/AutoAssertNoGC guards pushed into exactly the right places.

(In reply to Nicholas Nethercote [:njn] from comment #23)
> Comment on attachment 670484 [details] [diff] [review]
> ::: js/src/gc/Root.h
> @@ +378,5 @@
> > +    Return(NullPtr) : ptr_(NULL) {}
> > +
> > +    /*
> > +     * |operator const T &| is the safest way to access a Return<T>; however, it
> > +     * can only be used when in a NoGC scope.
> 
> It's the safest way to access a Return<T> *without rooting it*.
> 
> Also, maybe mention that it'll assert if not used in a NoGC scope?

Good catch!
 
> @@ +441,5 @@
> > +     * |operator==| is safe to use in any context.  It is present to allow:
> > +     *     JS_ASSERT(myScript == fun->script().get());
> > +     *
> > +     * To be rewritten as:
> > +     *     JS_ASSERT(fun->script() == myScript);
> 
> My initial thought was "why'd you change the operand order in the second
> example?"  Now I see why, but it might be worth explaining briefly
> afterwards?

Excellent.

(In reply to Nicholas Nethercote [:njn] from comment #22)

The first plan is to not move JSScripts, but I'm sure we'll get there eventually.

> > I'd prefer this be called unsafeGet. That's the name we use for a number of
> > other such methods where you're saying "it's okay, I know what I'm doing."
> 
> And the existing unsafeGet can become reallyUnsafeGet :P

The only place we really needed to be using |reallyUnsafeGet| were in Rooted<T> so I made it a friend of Return<T> and made those accesses use |ptr_| directly.(In reply to 

Nicholas Nethercote [:njn] from comment #21)
> > > Is this wrong?  I think the JSDebugErrorHook might be able to GC.
> > 
> > There are no uses of this hook by the browser.  I think that making this
> > function able to GC would be extremely annoying, because it would
> > effectively make cx/rt->malloc able to GC where they are not currently.  If
> > anything I think we should remove the cx arg from the hook.
> 
> I'm all for that.  Follow-up bug?

Bug 800631.

> > > ::: js/src/methodjit/MethodJIT.cpp
> > > @@ +1115,5 @@
> > > >  #if JS_TRACE_LOGGING
> > > >      AutoTraceLog logger(TraceLogging::defaultLogger(),
> > > >                          TraceLogging::JM_START,
> > > >                          TraceLogging::JM_STOP,
> > > > +                        fp->script().unsafeGet());
> > 
> > Because figuring out whether its safe or not is brain-cycles I will never
> > get back.  It's E4X, so as long as it still compiles for another few weeks
> > we're golden.
> 
> I don't think that's E4X...

Doh!  You are right, I've changed it, then changed it back to |unsafeGet| again with the |get| -> |unsafeGet| rename :-). 
 
> > > @@ +473,5 @@
> > > >       * Notify any debuggers about the creation of the script for
> > > >       * |Function.prototype| -- after all initialization, for simplicity.
> > > >       */
> > > > +    RootedScript functionProtoScript(cx, functionProto->script());
> > > > +    js_CallNewScriptHook(cx, functionProtoScript, functionProto);
> > > 
> > > I would have expected you to just change the 2nd arg to
> > > |functionProto->script().get()|.
> > 
> > js_CallNewScriptHook can get into arbitrary code, right?  If so, then having
> > the temporary returned by |get| still live would not be safe.
> 
> I don't understand.  |functionProto->script().get()| would fully evaluate
> before js_CallNewScriptHook would be called.  Or is this a case where the
> checking is a bit overzealous and we have to work around it?

You are correct.  I was surprised that this case is problematic: technically there is a sequence point /at/ the call so I was expecting argument evaluation to include destructing the temporaries that are created.  The wording of the spec (as reflected on Wikipedia) is extremely vague, so I'm not sure this is wrong since all compilers I have access to seem to do it the same way.
Attachment #669662 - Attachment is obsolete: true
Attachment #670484 - Attachment is obsolete: true
Attachment #670893 - Flags: review+
Attachment #670893 - Flags: feedback?(gary)
Comment on attachment 670893 [details] [diff] [review]
v2

decoder's expertise is needed too!
Attachment #670893 - Flags: feedback?(choller)
Thx, I'm on it now.
Assertion failure: InNoGCScope(), at ../../gc/Root.h:417

function f(code) {
    code = code.replace(/\/\*DUPTRY\d+\*\//, function(k) {});
    disassemble("-r", f)
}
f("switch(''){default:break;/*DUPTRY525*/}")
Attached patch v3Splinter Review
Nice: I did not expect to see *that*.  Looks like I messed up when applying feedback.
Attachment #670893 - Attachment is obsolete: true
Attachment #670893 - Flags: feedback?(gary)
Attachment #670893 - Flags: feedback?(choller)
Attachment #670932 - Flags: review+
Attachment #670932 - Flags: feedback?(gary)
Attachment #670932 - Flags: feedback?(choller)
Comment on attachment 670932 [details] [diff] [review]
v3

I didn't find anything terribly wrong after a weekend of fuzzing. feedback+ from me.
Attachment #670932 - Flags: feedback?(gary) → feedback+
Comment on attachment 670932 [details] [diff] [review]
v3

Same here :)
Attachment #670932 - Flags: feedback?(choller) → feedback+
Christian, Gary: thanks for the fuzzing!

Green try run (modulo normal Androinsanity) at:
https://tbpl.mozilla.org/?tree=Try&rev=25320b8ab48c

Pushed at:
https://hg.mozilla.org/integration/mozilla-inbound/rev/741fb7f8e5cb
Blocks: 802319
https://hg.mozilla.org/mozilla-central/rev/741fb7f8e5cb
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Depends on: 805300
Depends on: 810588
Blocks: 810588
No longer depends on: 810588
You need to log in before you can comment on or make changes to this bug.