832373 - Refactor eval-in-frame to use AbstractFramePtr

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1: Use AbstractFramePtr

For eval(-in-frame) the caller StackFrame is passed down to the frontend and various other parts. This should be an AbstractFramePtr so that it works with the baseline compiler.

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 703948 [details] [diff] [review]
Part 1: Use AbstractFramePtr

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

Nice!

::: js/src/builtin/Eval.cpp
@@ +317,5 @@
>      if (!WarnOnTooManyArgs(cx, args))
>          return false;
>  
>      Rooted<GlobalObject*> global(cx, &args.callee().global());
> +    return EvalKernel(cx, args, INDIRECT_EVAL, AbstractFramePtr(), global);

I think it'd be slightly more readable to have a NullFramePtr() where, perhaps NullFramePtr was a derived class and the AbstractFramePtr() default constructor was protected.

::: js/src/vm/Stack.h
@@ +227,5 @@
> +
> +    AbstractFramePtr(StackFrame *fp)
> +        : ptr_(fp ? uintptr_t(fp) | 0x1 : 0)
> +    {
> +    }

Could you JS_ASSERT((uintptr_t(fp) & 1) == 0) here and below?

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 2: initExecuteFrame, m-c part

If eval-in-frame is used with a baseline JIT frame, set the frame's prev_ link to its entry frame.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 3: Support eval-in-frame with baseline JIT frames

Patch for the baseline JIT branch. With these patches the frame.eval() tests pass.

Comment 5

[Jan de Mooij [:jandem]]

Part 1 with nits addressed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1392d241c137

(In reply to Luke Wagner [:luke] from comment #2)
> I think it'd be slightly more readable to have a NullFramePtr() where,
> perhaps NullFramePtr was a derived class and the AbstractFramePtr() default
> constructor was protected.

Great suggestion, I agree NullFramePtr() looks much nicer.

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 704500 [details] [diff] [review]
Part 2: initExecuteFrame, m-c part

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

Very nice

::: js/src/vm/Stack.cpp
@@ +1080,4 @@
>       */
>      CallArgsList *evalInFrameCalls = NULL;  /* quell overwarning */
>      MaybeExtend extend;
> +    StackFrame *prevLink = NULL;

I tend to leave off initialization when GCC warnings don't force it since it indicates "I'm declaring this to initialize it on the branches below".

::: js/src/vm/Stack.h
@@ +301,5 @@
>      inline bool prevUpToDate() const;
>      inline void setPrevUpToDate() const;
>      inline AbstractFramePtr evalPrev() const;
>  
> +    inline void *annotation() const;

die!

Comment 7

[Jan de Mooij [:jandem]]

Part 2, with nit addressed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/cec0e9df1d2c

Comment 8

[Jan de Mooij [:jandem]]

Part 3:

https://hg.mozilla.org/projects/ionmonkey/rev/7e34528bc2b7

Comment 9

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/1392d241c137

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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