Refactor eval-in-frame to use AbstractFramePtr

RESOLVED FIXED in mozilla21

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

No description provided.
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.
Attachment #703948 - Flags: review?(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?
Attachment #703948 - Flags: review?(luke) → review+
If eval-in-frame is used with a baseline JIT frame, set the frame's prev_ link to its entry frame.
Attachment #704500 - Flags: review?(luke)
Patch for the baseline JIT branch. With these patches the frame.eval() tests pass.
Attachment #704519 - Flags: review?(luke)
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.
Whiteboard: [leave open]
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!
Attachment #704500 - Flags: review?(luke) → review+
Attachment #704519 - Flags: review?(luke) → review+
Part 3:

https://hg.mozilla.org/projects/ionmonkey/rev/7e34528bc2b7
Status: NEW → ASSIGNED
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/1392d241c137
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.