1436407 - Rationalize how BytecodeEmitter generates column-number source notes

Status: NEW

(Core :: JavaScript Engine, enhancement, P5)

Description

[Jason Orendorff [:jorendorff]]

Last I looked, I couldn't make sense of how BytecodeEmitter emits the column half of location information. It is apparently sufficiently borked that the devtools have to work around:

https://github.com/devtools-html/debugger.html/pull/5322

So, at a guess, we should:

* Never update line number without updating column too
* Include column data in SN_SETLINE/SETNEWLINE notes
* Delete SN_COLSPAN

Comment 1

[Jason Laster [:jlast]]

So... the main case i've seen is pausing at control flow logic like IF and FOR.

Comment 2

[Jason Laster [:jlast]]

It looks like the biggest issue is IF conditions where the column information for IF conditions is always column 0.

https://dbg-conditionals.glitch.me/

> at {"source":{"actor":"server1.conn0.child1/source25","generatedUrl":null,"url":"https://puzzle-watch.glitch.me/client.js","isBlackBoxed":false,"isPrettyPrinted":false,"isSourceMapped":false,"sourceMapURL":null,"introductionUrl":null,"introductionType":"scriptElement"},"line":4,"column":0,"lastColumn":1}
> at {"source":{"actor":"server1.conn0.child1/source25","generatedUrl":null,"url":"https://puzzle-watch.glitch.me/client.js","isBlackBoxed":false,"isPrettyPrinted":false,"isSourceMapped":false,"sourceMapURL":null,"introductionUrl":null,"introductionType":"scriptElement"},"line":9,"column":0,"lastColumn":1}

Comment 3

[Logan Smyth [:loganfsmyth]]

We came up with another undesirable case during a discussion yesterday that is because of the way expressions are handled. Currently, expressions primarily rely on a parent container to set their start column, which can lead to undesirable behavior when there are prefixed operators, but the first entrypoint is at another locations:

```
dis(function(){
    typeof f();
//  ^          - column 4
//         ^   - column 11
});
```
results in
```
flags: LAMBDA CONSTRUCTOR
loc     op
-----   --
main:
00000:  getgname "f"                    # f
00005:  gimplicitthis "f"               # f THIS
00010:  call 0                          # f(...)
00013:  typeofexpr                      # (typeof f(...))
00014:  pop                             # 
00015:  retrval                         # 

Source notes:
 ofs line    pc  delta desc     args
---- ---- ----- ------ -------- ------
  0:    1     0 [   0] newline 
  1:    2     0 [   0] colspan 4
  3:    2    10 [  10] xdelta  
  4:    2    10 [   0] colspan 7
  6:    2    15 [   5] newline 
```

This makes column 4, and column 11 pause locations, but conceptually they are in the reverse order that most users would expect. Stepping to this statement will pause at opcode 0, which is the `f` lookup, but column 4, the 'typeof', is highlighted, because the parent expression generically set column 4 as the start of the overall expression, and only the actual call opcode changes that, meaning the 'getgname' and such opcodes run with the wrong column, and there is no column offset assigned for the typeof itself at offset 13.

Comment 4

[Jason Laster [:jlast]]

I believe this is now fixed.

Comment 5

[Logan Smyth [:loganfsmyth]]

I'm going to vote to leave this open because it does still influence the position information that we should for stack frames. We're dramatically better for stepping, and shouldn't trigger any position issues with stepping anymore, but poor position metadata could still bite us when inspecting the location of parent stack frames.

Comment 6

[Jason Laster [:jlast]]

With breakpoint positions, this is no longer a priority.