Rationalize how BytecodeEmitter generates column-number source notes

NEW
Unassigned

Status

()

enhancement
P5
normal
2 years ago
2 months ago

People

(Reporter: jorendorff, Unassigned)

Tracking

(Blocks 1 bug)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fix-optional)

Details

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
Priority: -- → P3
So... the main case i've seen is pausing at control flow logic like IF and FOR.
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}
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.

I believe this is now fixed.

Flags: needinfo?(lsmyth)
Blocks: dbg-control
No longer blocks: dbg-stepping

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.

Flags: needinfo?(lsmyth)

With breakpoint positions, this is no longer a priority.

Priority: P3 → P5
You need to log in before you can comment on or make changes to this bug.