1341943 - Do not emit JSOp::RetRval for arrow function with expression body

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Tooru Fujisawa [:arai]]

discovered by bug 1322019.

js> dis(() => (x && y)())
flags: LAMBDA EXPRESSION_CLOSURE ARROW
loc     op
-----   --
main:
00000:  getgname "x"                    # x
00005:  and 17 (+12)                    # x
00010:  jumptarget                      # x
00011:  pop                             # 
00012:  getgname "y"                    # y

# from and @ 00005
00017:  jumptarget                      # merged<x>
00018:  undefined                       # merged<x> undefined
00019:  call 0                          # merged<x>(...)
00022:  return                          # 
00023:  retrval                         # !!! UNREACHABLE !!!

the last JSOP_RETRVAL is always unreachable.
if it's not used elsewhere in optimization or something, we should stop emitting it.

Comment 1

[Tooru Fujisawa [:arai]]

looks like it's intentional.

https://dxr.mozilla.org/mozilla-central/rev/7ef1e9abd296a8edc39b7efc8d637767ba2f77ed/js/src/frontend/BytecodeEmitter.cpp#4978-4982
>     // Always end the script with a JSOP_RETRVAL. Some other parts of the
>     // codebase depend on this opcode,
>     // e.g. InterpreterRegs::setToEndOfScript.
>     if (!emit1(JSOP_RETRVAL))
>         return false;

not sure how InterpreterRegs::setToEndOfScript uses it tho...

https://dxr.mozilla.org/mozilla-central/rev/7ef1e9abd296a8edc39b7efc8d637767ba2f77ed/js/src/vm/Stack.cpp#436-442
> // Unlike the other methods of this class, this method is defined here so that
> // we don't have to #include jsautooplen.h in vm/Stack.h.
> void
> InterpreterRegs::setToEndOfScript()
> {
>     sp = fp()->base();
> }

at least jsautooplen.h doesn't exist.

Comment 2

[Tooru Fujisawa [:arai]]

(the opcode are renamed and now it's JSOp::RetRval)

Things to do here:

• Verify there's no code that expects the unreachable JSOp::RetRval at the end of script
• Locate the code path that emits JSOp::RetVal at the end of arrow function
• Modify the code not to emit it, if it's arrow function with expression body

JSOp::RetRval itself is emitted in the following function, in BytecodeEmitter class.

https://searchfox.org/mozilla-central/rev/4a15041348e08fb0d6f5726015c32861e663fbfe/js/src/frontend/BytecodeEmitter.h#1023

struct MOZ_STACK_CLASS BytecodeEmitter {
...
  [[nodiscard]] bool emitReturnRval() { return emit1(JSOp::RetRval); }

Opcodes.h contains document for each opcode.

https://searchfox.org/mozilla-central/rev/4a15041348e08fb0d6f5726015c32861e663fbfe/js/src/vm/Opcodes.h#2506-2522

/*
 * Stop execution and return the current stack frame's `returnValue`. If no
 * `JSOp::SetRval` instruction has been executed in this stack frame, this
 * is `undefined`.
 *
 * Also emitted at end of every script so consumers don't need to worry
 * about running off the end.
 *
 * If the current script is a derived class constructor, `returnValue` must
 * be an object. The script can use `JSOp::CheckReturn` to ensure this.
 *
 *   Category: Control flow
 *   Type: Return
 *   Operands:
 *   Stack: =>
 */ \
MACRO(RetRval, ret_rval, NULL, 1, 0, 0, JOF_BYTE) \

If you have any question, feel free to ask here, or in https://chat.mozilla.org/#/room/#spidermonkey:mozilla.org

Comment 3

[Mohamed Atef]

I moved

if (!bce_->emitReturnRval()) {
      return false;
    }

inside this condition if (!funbox_->hasExprBody())
https://searchfox.org/mozilla-central/source/js/src/frontend/FunctionEmitter.cpp#628
it works for arrow functions but for a test case like this:
function foo(){var x = 5; return x;} it doesn't work, and produce the unreachable RetRval.
but if we remove this part completely it removes the RetRval from the empty arrow functions.
Am I on the right path?

Comment 4

[Mohamed Atef]

(In reply to Mohamed Atef from comment #3)

I moved

if (!bce_->emitReturnRval()) {
      return false;
    }

what about deleting this part?
I think It works fine.

inside this condition if (!funbox_->hasExprBody())
https://searchfox.org/mozilla-central/source/js/src/frontend/FunctionEmitter.cpp#628
it works for arrow functions but for a test case like this:
function foo(){var x = 5; return x;} it doesn't work, and produce the unreachable RetRval.
but if we remove this part completely it removes the RetRval from the empty arrow functions.
Am I on the right path?

Comment 5

[Mohamed Atef]

(In reply to Mohamed Atef from comment #4)

(In reply to Mohamed Atef from comment #3)

I moved

if (!bce_->emitReturnRval()) {
      return false;
    }

what about deleting this part?
I think It works fine.
sorry for that.
We should emit RetRval if it's an empty function, right?

inside this condition if (!funbox_->hasExprBody())
https://searchfox.org/mozilla-central/source/js/src/frontend/FunctionEmitter.cpp#628
it works for arrow functions but for a test case like this:
function foo(){var x = 5; return x;} it doesn't work, and produce the unreachable RetRval.
but if we remove this part completely it removes the RetRval from the empty arrow functions.
Am I on the right path?

Comment 6

[Mohamed Atef]

(In reply to Mohamed Atef from comment #3)

I moved

if (!bce_->emitReturnRval()) {
      return false;
    }

inside this condition if (!funbox_->hasExprBody())
https://searchfox.org/mozilla-central/source/js/src/frontend/FunctionEmitter.cpp#628
it works for arrow functions but for a test case like this:
function foo(){var x = 5; return x;} it doesn't work, and produce the unreachable RetRval.
from trying several test cases ()=>{var x; return x;} doesn't have an expression body.
I think the solution mentioned above is what we want.
but what if the function doesn't have an expression body?
but if we remove this part completely it removes the RetRval from the empty arrow functions.
Am I on the right path?

Comment 7

[Tooru Fujisawa [:arai]]

Good point.

Currently, JSOp::RetRval is always emitted at the end of function to catch all code paths that doesn't have explicit return statement (JSOp::Return), so that the function returns undefined.

Ultimately we should emit JSOp::RetRval only if it's reachable, but so far this bug focuses to arrow function with expression body case, as a first step, given arrow function with expression body is known to always have JSOp::Return, and it doesn't require any code flow analysis or whatever.
And the fix you've done sounds good.

So, don't worry about function foo(){var x = 5; return x;} case or ()=>{var x; return x;} case. We can look into it in followup bug.

Comment 8

[Mohamed Atef]

Attachment: Bug 1341943: Do not emit JSOp::RetRval for arrow function with expression body. r=arai

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a49b05fcf23
Do not emit JSOp::RetRval for arrow function with expression body. r=arai

Comment 10

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/0a49b05fcf23