1186723 - Make BytecodeEmitter::emitDupAt take a reasonable offset.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

At present, emitDupAt() takes an offset from the bottom of the stack, which is...not all that useful, except in the one place for which it was written. In practice, when you want a JSOP_DUPAT, you know the offset down from the top.

The JSOP_DUPAT has a sane operand, but the emitter function is nuts.

Comment 1

[Tooru Fujisawa [:arai]]

This should be a good first bug :)

Required code changes are following:
  * Make BytecodeEmitter::emitDupAt [1] take the offset down from the top of the stack (the value of slotFromTop inside the method), instead of the offset from the bottom of the stack
  * Rewrite all BytecodeEmitter::emitDupAt callsites to follow the change

You'll need basic knowledge of C++, and be able to build and test Firefox [2] or SpiderMonkey [3].

A skilled first-time contributor should be able to finish this in a month.  Leave comments / questions here, or ask me (:arai) in IRC #introduction and #jsapi channels.

[1] https://hg.mozilla.org/mozilla-central/file/2ddec2dedced/js/src/frontend/BytecodeEmitter.cpp#l334
(note that I linked to specific revision of the file to point the method, it may not be the latest one)
[2] https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
[3] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation

Comment 2

[Tyler Steiman]

Hi Tooru,

I would love to work on this bug, I've assigned it to myself. 

Is the problem here how slotFromTop is being set and evaluated? 

>343: unsigned slotFromTop = stackDepth - 1 - slot;
https://dxr.mozilla.org/mozilla-central/source/js/src/frontend/BytecodeEmitter.cpp#343

Or what exactly is the problem? I'm still learning and would appreciate any insight, thanks!

Comment 3

[Tooru Fujisawa [:arai]]

Hi Muhsin, thank you for taking this :D

The problem is that, emitDupAt takes the offset from the bottom of the stack (slot argument), but we want to pass the offset from the top of the stack, because of following reasons:

1. almost all callsites calculate the value of "the offset from the bottom of the stack" from "the offset from the top of the stack" value [1], like following (there, 2 is the offset from the top of the stack)

> if (!emitDupAt(this->stackDepth - 1 - 2))
>     return false;

2. emitDupAt calculates the value of "the offset from the top of the stack" (slotFromTop variable) from "the offset from the bottom of the stack" (slot argument)

> unsigned slotFromTop = stackDepth - 1 - slot;

those 2 steps are just redundant.  we don't have to calculate the `slot` value in callsite.
in former example, we should just pass 2, and take it as slotFromTop in emitDupAt, without any other unnecessary calculation with stack depth.

3. JSOP_DUPAT opcode has the offset from the top of stack as its operand [2] (the slotFromTop value), not from the bottom, it's confusing when writing bytecode emitter and reading emitted bytecode

so, if we make emitDupAt take the offset from the top of the stack as its argument, everything will become simpler.

does this make sense?

[1] https://dxr.mozilla.org/mozilla-central/search?q=emitDupAt%28&redirect=false&case=true&limit=76&offset=0
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Internals/Bytecode (search JSOP_DUPAT)

Comment 4

[Tyler Steiman]

Thanks for the reply Tooru. I understand however I have one question. If we are to just pass the value on callsite as such:

> if (!emitDupAt(2))
>     return false;

The logic inside emitDupAt: 

> unsigned slotFromTop = stackDepth - 1 - slot;

Still stands valid, right? If slotFromTop's logic still stands true the only thing that needs to change is the arguments on callsite, right? 

Or should slotFromTop automatically take its value from the argument (2)? The reason I ask is because you stated:

>we should just pass 2, and take it as slotFromTop in emitDupAt 

I'm just confused on this part.

Comment 5

[Tooru Fujisawa [:arai]]

Sorry, I forgot to note the important thing.
This is a code cleanup, so no behavior should be changed.

(In reply to Muhsin A. Steiman from comment #4)
> Thanks for the reply Tooru. I understand however I have one question. If we
> are to just pass the value on callsite as such:
> 
> > if (!emitDupAt(2))
> >     return false;
> 
> The logic inside emitDupAt: 
> 
> > unsigned slotFromTop = stackDepth - 1 - slot;
> 
> Still stands valid, right?

No. If it takes 2 as `slot` argument, and calculate `slotFromTop` from it, the value will become different, right?
We need to keep the output bytecode same, so the value of `slotFromTop` should keep same between the change.


for example, in that case

> if (!emitDupAt(this->stackDepth - 1 - 2))
>     return false;

if stack depth is 10, `emitDupAt` will be called with 7.

> unsigned slotFromTop = stackDepth - 1 - slot;

and here, `slot` argument is 7, and `sloFromTop` will become 2.  that's the number directly written in the callsite.
The value of `slot` is used only in the calculation.
We don't need to calculate the value of `slot` (7), but just pass 2 and take it as `slotFromTop`, right?


> Or should slotFromTop automatically take its value from the argument (2)?

yes :)

Comment 6

[Tyler Steiman]

Attachment: bug1186723_emitDupAt.diff

This patch is what I have so far.

>340     MOZ_ASSERT(slotFromTop < unsigned(stackDepth));

I'm running into a runtime error here, it's telling me I'm having an assertion failure however I'm not exactly sure why. Shouldn't slotFromTop indeed be < stackDepth?

I changed slot to slotFromTop just for clarity purposes. 

I hope I'm understanding the problem properly, if you could give me some feedback I would appreciate it, thank you Tooru.

Comment 7

[Tooru Fujisawa [:arai]]

Comment on attachment 8645492 [details] [diff] [review]
bug1186723_emitDupAt.diff

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

Thank you for your patch :D
Yeah, this patch does what we need here.  It's almost there.

(In reply to Muhsin A. Steiman from comment #6)
> >340     MOZ_ASSERT(slotFromTop < unsigned(stackDepth));
> 
> I'm running into a runtime error here, it's telling me I'm having an
> assertion failure however I'm not exactly sure why. Shouldn't slotFromTop
> indeed be < stackDepth?

There are two places where you're passing wrong value, see following.


https://dxr.mozilla.org/mozilla-central/rev/d6ea652c579992daa9041cc9718bb7c6abefbc91/js/src/frontend/BytecodeEmitter.cpp#7892
>         if (!emitDupAt(arrayCompDepth))
>             return false;

arrayCompDepth is the offset from the bottom of the stack.  This is the only one place where we know the offset from the bottom, not from top.  Please change that line to pass the offset from the top of the stack.


https://dxr.mozilla.org/mozilla-central/rev/d6ea652c579992daa9041cc9718bb7c6abefbc91/js/src/frontend/BytecodeEmitter.h#391
>     // Dup the var in operand stack slot "slot". The first item on the operand
>     // stack is one slot past the last fixed slot. The last (most recent) item is
>     // slot bce->stackDepth - 1.
>     //
>     // The instruction that is written (JSOP_DUPAT) switches the depth around so
>     // that it is addressed from the sp instead of from the fp. This is useful when
>     // you don't know the size of the fixed stack segment (nfixed), as is the case
>     // when compiling scripts (because each statement is parsed and compiled
>     // separately, but they all together form one script with one fixed stack
>     // frame).
>     bool emitDupAt(unsigned slot);

The emitDupAt method is declared here.  Please change the argument name and comment match to the definition in BytecodeEmitter.cpp.  I guess most of those description can be just removed, as the emitDupAt method is no more complicated as before.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +341,2 @@
>  
>      // The slot's position on the operand stack, measured from the top.

This comment was for slotFromTop variable, so it should be removed or moved to BytecodeEmitter.h.

@@ +2900,5 @@
> +    if (!emitDupAt(2))       // KEY THIS OBJ KEY
> +        return false;
> +    if (!emitDupAt(2))       // KEY THIS OBJ KEY THIS
> +        return false;
> +    if (!emitDupAt(2))       // KEY THIS OBJ KEY THIS OBJ

Please align the comments to other lines ;)

@@ +5374,5 @@
>              return false;
>          if (!emit1(JSOP_DUP))                             // ITER ITER
>              return false;
>      } else {
> +        if (!emitDupAt(2))         // ITER ARR I ITER

Please align the comment here too.

@@ +6738,5 @@
>          }
>  
>          if (isNewOp) {
>              // Repush the callee as new.target
> +            if (!emitDupAt(1 - (argc + 1)))

The offset from the top of stack is `argc + 1`.

Comment 8

[Tyler Steiman]

Attachment: bug1186723_emitDupAt.diff

Tooru, I have updated the patch. Please let me know if I missed something here :)

Comment 9

[Tooru Fujisawa [:arai]]

Comment on attachment 8645636 [details] [diff] [review]
bug1186723_emitDupAt.diff

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

Thank you again!
Code change looks good :)
I think the comment in BytecodeEmitter.h needs some more brushup, but not sure what the best description here is (and I'm still not good at english, sorry :P

forwarding to luke, who reviewed the emitDupAt patch before.

Comment 10

[Eric Faust [:efaust]]

Comment on attachment 8645636 [details] [diff] [review]
bug1186723_emitDupAt.diff

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

This is so much better than it used to be! Thanks for the cleanup :)

::: js/src/frontend/BytecodeEmitter.h
@@ +377,5 @@
>  
>      // Emit three bytecodes, an opcode with two bytes of immediate operands.
>      bool emit3(JSOp op, jsbytecode op1, jsbytecode op2);
>  
> +    // The slot's position on the operand stack, measured from the top.

How about something like

// Helper to emit JSOP_DUPAT. The argument is the value's depth on the
// JS stack, as measured from the top.

Comment 11

[Tyler Steiman]

Attachment: bug1186723_emitDupAt.diff

I've updated the comment in the header file. I've never done the push to test, so if a test is needed I need some help... thanks for the help!

Comment 12

[Tooru Fujisawa [:arai]]

Pushed to try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d276b671ddd
It'll finish within 2 hours

Comment 13

[Tooru Fujisawa [:arai]]

Comment on attachment 8646182 [details] [diff] [review]
bug1186723_emitDupAt.diff

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

It passed the try run :D

Comment 14

[Tyler Steiman]

Great! Thanks for the help Tooru I learned a lot and enjoyed working on this!

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/493068dc26dc

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/493068dc26dc

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/493068dc26dc