798638 - GC: exact rooting for TryNoteIter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: v0

TryNoteIter stores a JSScript* and lives on the stack.  This switches it to a RootedScript.

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 668664 [details] [diff] [review]
v0

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

::: js/src/jsinterp.cpp
@@ +3977,5 @@
>      gc::MaybeVerifyBarriers(cx, true);
>      return interpReturnOK ? Interpret_Ok : Interpret_Error;
> +
> +  exit_error_to_len:
> +    DO_NEXT_OP(len);

Why is this necessary?  Each weird goto makes reasoning about Interpret that much more confusing, so it would be really good to avoid this.

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 668664 [details] [diff] [review]
v0

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

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

Is it worth commenting that this is safe because objects of this class are always stack-allocated?

Comment 3

[Terrence Cole [:terrence]]

Attachment: v1: Longer, with fewer inexplicable branches.

(In reply to Nicholas Nethercote [:njn] from comment #2)
> Is it worth commenting that this is safe because objects of this class are
> always stack-allocated?

Good idea!  Done.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 669723 [details] [diff] [review]
v1: Longer, with fewer inexplicable branches.

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

::: js/src/jsinterp.cpp
@@ +820,5 @@
> +      case JSTRAP_RETURN:
> +        cx->clearPendingException();
> +        regs.fp()->setReturnValue(rval);
> +      case JSTRAP_THROW:
> +        cx->setPendingException(rval);

These cases seem to be missing break statements.

@@ +3938,3 @@
>          }
>  
> +        switch(UnwindForCatchableException(cx, regs, script)) {

I'm not sure that this factoring of CallThrowHook/UnwindForCatchableException really makes anything simpler: it doesn't simplify control flow or add any real abstractions, it just kinda pulls code apart.

IIRC, removing indirect goto would solve these problems; can we just do that (as a prerequesite bug)?

Comment 5

[Terrence Cole [:terrence]]

Attachment: v2

Now that the interpreter loop simplification has stuck, this is thoroughly trivial to implement.

Comment 6

[Terrence Cole [:terrence]]

Green try run at:
https://tbpl.mozilla.org/?tree=Try&rev=0579d07256bf

Pushed at:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0893be94207

Comment 7

[Ed Morley [:emorley]]

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