Open Bug 1436407 Opened 3 years ago Updated 2 years ago

Rationalize how BytecodeEmitter generates column-number source notes


(Core :: JavaScript Engine, enhancement, P5)




Tracking Status
firefox60 --- fix-optional


(Reporter: jorendorff, Unassigned)


(Blocks 1 open bug)


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:

So, at a guess, we should:

* Never update line number without updating column too
* Include column data in SN_SETLINE/SETNEWLINE notes
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.

> at {"source":{"actor":"server1.conn0.child1/source25","generatedUrl":null,"url":"","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":"","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:

    typeof f();
//  ^          - column 4
//         ^   - column 11
results in
loc     op
-----   --
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.