1519005 - Remove BytecodeEmitter.emitLevel and EmitLevelManager

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Tooru Fujisawa [:arai]]

Looks like it's just to perform updateSourceCoordNotes at the end of top-level script or function script.
we should do that in the caller, BytecodeEmitter::emitFunctionScript and BytecodeEmitter::emitScript.

This will simplify bug 1473796 patch.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Remove BytecodeEmitter.emitLevel and EmitLevelManager

Remove BytecodeEmitter.emitLevel and EmitLevelManager, for the following reasons:

BytecodeEmitter.emitLevel and EmitLevelManager are used only in the following code
that is, whether to call updateSourceCoordNotes or not on certain case.

https://searchfox.org/mozilla-central/rev/5053031ba7621fa8f63f42de4c204ab3561e4e59/js/src/frontend/BytecodeEmitter.cpp#9247-9252

bool BytecodeEmitter::emitTree(
    ParseNode* pn, ValueUsage valueUsage /* = ValueUsage::WantValue */,
    EmitLineNumberNote emitLineNote /* = EMIT_LINENOTE */) {
...
  EmitLevelManager elm(this);
...
  /* bce->emitLevel == 1 means we're last on the stack, so finish up. */
  if (emitLevel == 1) {
    if (!updateSourceCoordNotes(pn->pn_pos.end)) {
      return false;
    }
  }
  return true;
}

then, emitLevel == 1 condition becomes true only the following 3 callsites (emitTree is called inside emitLexicalScopeBody),
and updateSourceCoordNotes is already called for the same position.

https://searchfox.org/mozilla-central/rev/5053031ba7621fa8f63f42de4c204ab3561e4e59/js/src/frontend/BytecodeEmitter.cpp#2311-2321

bool BytecodeEmitter::emitScript(ParseNode* body) {
...
  if (sc->isEvalContext() && !sc->strict() && body->is<LexicalScopeNode>() &&
      !body->as<LexicalScopeNode>().isEmptyScope()) {
...
    if (!emitLexicalScopeBody(scope->scopeBody())) {  <--- HERE
      return false;
    }
...
  } else {
    if (!emitTree(body)) {  <--- HERE
      return false;
    }
  }

  if (!updateSourceCoordNotes(body->pn_pos.end)) { <-- HERE
    return false;
  }

and

https://searchfox.org/mozilla-central/rev/5053031ba7621fa8f63f42de4c204ab3561e4e59/js/src/frontend/BytecodeEmitter.cpp#2392-2394

bool BytecodeEmitter::emitFunctionScript(CodeNode* funNode,
                                         TopLevelFunction isTopLevel) {
...
  if (!emitTree(body)) { <--- HERE
    return false;
  }

  if (!updateSourceCoordNotes(body->pn_pos.end)) { <-- HERE
    return false;
  }

and calling updateSourceCoordNotes with same position twice has no effect.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 9035543 [details] [diff] [review]
Remove BytecodeEmitter.emitLevel and EmitLevelManager

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

The emitLexicalScopeBody call looks dodgy for this.  If that function gets called, why can't we enter the

```
  if (body->isKind(ParseNodeKind::StatementList) &&
      body->as<ListNode>().hasTopLevelFunctionDeclarations()) {
    // This block contains function statements whose definitions are
    // hoisted to the top of the block. Emit these as a separate pass
    // before the rest of the block.
    if (!emitHoistedFunctionsInList(&body->as<ListNode>())) {
      return false;
    }
  }
```

block, call eHFIL there, and then that does

```
  for (ParseNode* stmt : stmtList->contents()) {
    ParseNode* maybeFun = stmt;

    if (!sc->strict()) {
      while (maybeFun->isKind(ParseNodeKind::LabelStmt)) {
        maybeFun = maybeFun->as<LabeledStatement>().statement();
      }
    }

    if (maybeFun->isKind(ParseNodeKind::Function) &&
        maybeFun->as<CodeNode>().functionIsHoisted()) {
      if (!emitTree(maybeFun)) { // !!!
        return false;
      }
    }
  }
```

where it seems like we ought to be able to hit the !!! line without doing any updateSourceCoordNotes after each such node?

Given the screwy confluence of events that get us into this -- direct eval, nonstrict code, with top-level let declarations within it, with top-level function statements that must be hoisted -- it is conceivable we just don't have any tests of this.  Or the tests we do have that exercise this path, do not hinge in their correctness upon updateSourceCoordNotes happening.

I'm reading/reviewing this on a plane somewhat without Searchfox sorts of things, so maybe I'm missing something here, but this does not seem obviously right.  Please enlighten me!

Comment 3

[Tooru Fujisawa [:arai]]

About emitHoistedFunctionsInList, we're indeed creating SRC_SETLINEs for function nodes,
but those functions are hoisted and emitted inside the prologue section, that is not the target of source notes.
that results in setting those source notes to the first opcode in the main section.

I'll check some other cases to see whether those source notes are effectful or not.
If they're not, we can drop them.

Here's the example:

code

eval(`
let a;
function x() {
  return 20;
}
dis();
`);

output

loc     op
-----   --
00000:  uninitialized                   # UNINITIALIZED
00001:  initlexical 0                   # UNINITIALIZED
00005:  pop                             # 
00006:  lambda function x() {
  return 20;
} # FUN
00011:  deffun                          # 
main:
00012:  undefined                       # undefined
00013:  initlexical 0                   # undefined
00017:  pop                             # 
00018:  getgname "dis"                  # dis
00023:  gimplicitthis "dis"             # dis THIS
00028:  call 0                          # dis(...)
00031:  setrval                         # 
00032:  debugleavelexicalenv            # 
00033:  retrval                         # 

with calling updateSourceCoordNotes in emitHoistedFunctionsInList

Source notes:
 ofs line    pc  delta desc     args
---- ---- ----- ------ -------- ------
  0:    1    12 [  12] xdelta  
  1:    1    12 [   0] setline  lineno 5
  3:    5    12 [   0] colspan 1
  5:    5    12 [   0] setline  lineno 1
  7:    1    12 [   0] newline 
  8:    2    12 [   0] colspan 4
 10:    2    18 [   6] setline  lineno 6
 12:    6    32 [  14] xdelta  
 13:    6    32 [   0] newline 

Scope notes:
   index   parent    start      end
       1   (none)        0       33

without calling updateSourceCoordNotes in emitHoistedFunctionsInList

Source notes:
 ofs line    pc  delta desc     args
---- ---- ----- ------ -------- ------
  0:    1    12 [  12] xdelta  
  1:    1    12 [   0] newline 
  2:    2    12 [   0] colspan 4
  4:    2    18 [   6] setline  lineno 6
  6:    6    32 [  14] xdelta  
  7:    6    32 [   0] newline 

Scope notes:
   index   parent    start      end
       1   (none)        0       33

Comment 4

[Tooru Fujisawa [:arai]]

so far, I don't find any case that the existence of SRC_SETLINE for hoisted function affects debugger or coverage or anything.

SRC_SETLINE are used in the following functions.

[js::BytecodeRangeWithPosition::updatePosition]

Accumulates SRC_COLSPAN/SRC_SETLINE/SRC_NEWLINE up to given PC.
This means source notes in prologue are simply overwritten by source note
for the first opcode in the main section

[js::coverage::LCovSource::writeScript]

called by js::coverage::LCovRealm::collectCodeCoverageInfo, which does the almost same as above

[js::PCToLineNumber]

almost same as above

[js::LineNumberToPC]

almost same as above

[js::GetScriptLineExtent]

updateLineNumberNotes inside emitScript sets the largest line number,
and others don't affect.

Comment 5

[Jeff Walden [:Waldo]]

I was thinking maybe the absence of the source notes would affect line/column information on the error for if a hoisted function would conflict with an immutable binding of the same name in the enclosing environment. Something like

[jwalden@find-waldo-now after]$ cat /tmp/test.js 
const x = 3;
eval("let y = 4; function x() { return 2; }");
[jwalden@find-waldo-now after]$ dbg/js/src/js /tmp/test.js 
/tmp/test.js:1:20 SyntaxError: redeclaration of const x:
/tmp/test.js:1:20 let y = 4; function x() { return 2; }
/tmp/test.js:1:20 ....................^
Stack:
  @/tmp/test.js:2:1

where some of those numbers would change as a consequence. I rather doubt the absence of these notes truly affects nothing, but that's mostly instinct.

Comment 6

[Tooru Fujisawa [:arai]]

redeclaration error happens inside parser, and we don't use source notes there.

I'll check some more cases, and post updated patch if nothing found.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Remove BytecodeEmitter.emitLevel and EmitLevelManager

Bug 1284719 removed the source note in prologue,
and now we don't have to worry about the source note for hoisted functions.

Comment 8

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/52642bea80bae88ad486ce1b2e0d77c0376f86ac
Bug 1519005 - Remove BytecodeEmitter.emitLevel and EmitLevelManager. r=jwalden

Comment 9

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/52642bea80ba