Last Comment Bug 744670 - Port IonMonkey changes of StackIter to inbound.
: Port IonMonkey changes of StackIter to inbound.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 745360
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-11 18:04 PDT by Nicolas B. Pierron [:nbp]
Modified: 2012-04-14 06:41 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Port StackIter modification of IonMonkey. (14.41 KB, patch)
2012-04-12 11:45 PDT, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
Port StackIter modification of IonMonkey. (12.87 KB, patch)
2012-04-12 18:27 PDT, Nicolas B. Pierron [:nbp]
luke: review+
Details | Diff | Splinter Review

Description Nicolas B. Pierron [:nbp] 2012-04-11 18:04:42 PDT
Multiple modifications have been made to StackIter to make it compatible with IonFrames.  Bug 744253 needs to copy a subset of StackFrame interface to StackIter to abstract over the frame representation.  Bug 729886 and Bug 729833 are changing the interface of StackIter.

The goal of this bug is to extract pieces which are not related to IonMonkey and copy them to mozilla-central.
Comment 1 Nicolas B. Pierron [:nbp] 2012-04-12 11:45:21 PDT
Created attachment 614475 [details] [diff] [review]
Port StackIter modification of IonMonkey.

This fold 3 patches of IonMonkey and adapt to them to mozilla-central:

https://hg.mozilla.org/projects/ionmonkey/rev/751b7185683cebb25683b99f5726c42e10325df5
Add a function to dump JS stack for minimizing bugs. (Bug 729833, r=dvander)

https://hg.mozilla.org/projects/ionmonkey/rev/0ff7a7bd318f21f5f8737851095da8ae40b1d7cf
Make dumpStack compatible with IonFrames. (Bug 729886, r=dvander)

https://hg.mozilla.org/projects/ionmonkey/rev/2386dfe53a85d267cbc1883989fd262847d499e8
Rewrite fun_getProperty to handle Ion Frames (Bug 732853 part 2, r=dvander)
Comment 2 Luke Wagner [:luke] 2012-04-12 15:12:09 PDT
Comment on attachment 614475 [details] [diff] [review]
Port StackIter modification of IonMonkey.

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

::: js/src/jsfun.cpp
@@ +146,5 @@
> +        ArgumentsObject *argsobj;
> +        if (!fp) {
> +            JSObject &f = iter.callee();
> +            if (!&f)
> +                return false;

I'd take out the 'if (!fp)' branch

Also, regarding "if (!&f)"... I'm not sure that is even semantically well-defined.  Furthermore, we explicitly use references to mean non-null, so can you please change 'callee' to 'maybeCallee' and have it return a pointer (on IM).

@@ +163,5 @@
> +        return true;
> +    }
> +
> +    StackIter prev(iter);
> +    ++prev;

Can you push these two lines down into the callerAtom branch where it is used?

::: js/src/vm/ArgumentsObject.h
@@ +158,5 @@
>      /*
> +     * Cannot recover any fields of the arguments object, and replace it by
> +     * the poison value.
> +     */
> +    static ArgumentsObject *createPoison(JSContext *cx, uint32_t argc, JSObject &callee);

It seems like, for the poison case, we wouldn't add a createPoison to ArgumentsObject but instead do the exact same thing that fun_resolve does for the poisonPillProps.

::: js/src/vm/Stack.cpp
@@ +1249,5 @@
> +    JS_ASSERT(!done());
> +    switch (state_) {
> +      case SCRIPTED:
> +        return fp()->isFunctionFrame();
> +      default:

Rather than a 'default' label here and below, could you explicitly enumerate the cases that 'return true' (with JS_NOT_REACHED for DONE)?  That way, if we add a state the compiler will tell us if we forget about the switch.

::: js/src/vm/Stack.h
@@ +1836,5 @@
>  
>      bool operator==(const StackIter &rhs) const;
>      bool operator!=(const StackIter &rhs) const { return !(*this == rhs); }
>  
> +    // isInterpreterFrame

The rest of this file is all /* comments */.  Until we do some big change-all-of-SM-to-C++-style patch, can we keep it consistent per file?

@@ +1841,3 @@
>      bool isScript() const { JS_ASSERT(!done()); return state_ == SCRIPTED; }
> +    // A scripted frame may not provide an fp and an sp, such as IonMonkey frames.
> +    bool isScripted() const { JS_ASSERT(!done()); return state_ == SCRIPTED; }

I don't think we should have both isScript() and isScripted()
Comment 3 Nicolas B. Pierron [:nbp] 2012-04-12 18:27:33 PDT
Created attachment 614645 [details] [diff] [review]
Port StackIter modification of IonMonkey.

Apply nits, and remove paths only used by IonMonkey.
Comment 4 Luke Wagner [:luke] 2012-04-12 18:41:48 PDT
Comment on attachment 614645 [details] [diff] [review]
Port StackIter modification of IonMonkey.

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

::: js/src/jsfun.cpp
@@ +145,5 @@
> +
> +        ArgumentsObject *argsobj;
> +        argsobj = ArgumentsObject::createUnexpected(cx, fp);
> +
> +        if (!argsobj)

ArgsObject *argsobj = ArgumentsObject::createUnexpected(cx, fp);
if (!argsobj)
    return false;

@@ +180,3 @@
>  
> +        if (!prev.isFunctionFrame())
> +            return true;

if (prev.done() || !prev.isFunctionFrame())
    return true;

::: js/src/vm/Stack.h
@@ +1839,5 @@
>  
> +    /*
> +     * A script frame may not provide an fp and an sp, such as IonMonkey
> +     * frames. A script frame is garantee to provide a pc and a script.
> +     */

Can remove this comment for now.
Comment 5 Nicolas B. Pierron [:nbp] 2012-04-13 15:22:29 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/be00c204c582
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2012-04-14 06:40:55 PDT
https://hg.mozilla.org/mozilla-central/rev/be00c204c582
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2012-04-14 06:41:19 PDT
And please don't resolve the bug unless it makes it to m-c.

Note You need to log in before you can comment on or make changes to this bug.