The default bug view has changed. See this FAQ.

Port IonMonkey changes of StackIter to inbound.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

Other Branch
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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)
Attachment #614475 - Flags: feedback?(luke)

Comment 2

5 years ago
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()
Attachment #614475 - Flags: feedback?(luke)
(Assignee)

Updated

5 years ago
No longer blocks: 744253
(Assignee)

Comment 3

5 years ago
Created attachment 614645 [details] [diff] [review]
Port StackIter modification of IonMonkey.

Apply nits, and remove paths only used by IonMonkey.
Attachment #614475 - Attachment is obsolete: true
Attachment #614645 - Flags: review?(luke)

Comment 4

5 years ago
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.
Attachment #614645 - Flags: review?(luke) → review+
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/be00c204c582
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 745360
https://hg.mozilla.org/mozilla-central/rev/be00c204c582
And please don't resolve the bug unless it makes it to m-c.
You need to log in before you can comment on or make changes to this bug.