1477896 - Add SRC_DO_WHILE with 2 fields, replacing 2 SRC_WHILE notes for do-while

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

I don't see any reason to use 2 SRC_WHILE notes for do-while, which makes the code complicated.
we can add SRC_DO_WHILE with 2 fields (cond offset, back jump offset)

Comment 1

[Tooru Fujisawa [:arai]]

I'll fix this once we have DoWhileEmitter and SrcNote::DoWhile{1,2}, converting SrcNote::DoWhile{1,2} to SrcNote::DoWhile

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Add SRC_DO_WHILE replacing 2 SRC_WHILE notes for do-while.

here's the patch.
I'll ask review after bug 1456404 and bug 1477621 got resolved.

Comment 3

[Tooru Fujisawa [:arai]]

looks like it's problematic for breakpoint to remove JSOP_NOP there.
it expects some opcode for "do" part, in order to have separate breakpoints for "do" and body.
currently JSOP_NOP is for "do", and JSOP_LOOPHEAD is for body.
and surely "do" is nop.
I'll try applying the SRC_DO_WHILE change, while keeping JSOP_NOP there.

Comment 4

[Tooru Fujisawa [:arai]]

:jorendorff, just to confirm, is it preferred to have a breakpoint on "do" while it's nop ?

here's the places it stops when stepping over:

  var i = 0;
  do              // 1
  {
  console.log(i); // 2, 5, ...
  i++;            // 3, 6, ...
  }
  while
  (
  i < 10          // 4, 7, ...
  );

Comment 5

[Jason Orendorff [:jorendorff]]

My expectation as a user of the debugger is that I can set a breakpoint on any statement. Even if it's a NOP.

We already have some exceptions to this rule, and users do complain about them. In fact, I asked jlast, and it turns out that *at the moment I asked*, by complete coincidence, he was looking at a case where breakpoints were failing to hit on a line of code that says `if (true)`!

Comment 6

[Tooru Fujisawa [:arai]]

(In reply to Jason Orendorff [:jorendorff] from comment #5)
> We already have some exceptions to this rule, and users do complain about
> them. In fact, I asked jlast, and it turns out that *at the moment I asked*,
> by complete coincidence, he was looking at a case where breakpoints were
> failing to hit on a line of code that says `if (true)`!

sounds like, the real problem there is that we don't fallback to subsequent lines when the specified line doesn't have any column where breakpoint can be set.

now I feel, "whether users can *set* breakpoint to specific line" and "whether users can *stop* at the specific line when stepping" would be distinct issue.

both for "do" case and "if (true)" cases, setting breakpoint to the first possible line after the *nop* would make sense.
we don't have to do any kind of control flow analysis, but just trying the subsequent lines sequentially will work.

this should be what happens in other debuggers like lldb,
when I set a breakpoint on "do" line, the actual breakpoint is set to the first line in the body.

is there already a bug filed against it?

Comment 7

[Tooru Fujisawa [:arai]]

err, maybe I'm wrong.
I cannot set breakpoints for lines 1, 2, 4, 5, 7,
but trying setting to line 3 results in setting to line 6.
so some kind of fallback seems to already be performed, but not all cases.

  if   // 1
  (    // 2
  true // 3
  )    // 4
  {    // 5
  console.log(i * 2); // 6
  }    // 7

can I have more detail about the `if (true)` issue?

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Add SRC_DO_WHILE replacing 2 SRC_WHILE notes for do-while.

* Added SRC_DO_WHILE which has 2 slots (CondOffset, BackJumpOffset)
  * Replaced 2 SRC_WHILE for do-while with SRC_DO_WHILE,
    which is now added to JSOP_LOOPHEAD
  * JSOP_NOP at the top of loop is kept, in order to keep the same debugger behavior
    (make it possible to add breakpoint on `do`)

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8999575 [details] [diff] [review]
Add SRC_DO_WHILE replacing 2 SRC_WHILE notes for do-while.

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

This is much nicer, thanks.

Comment 10

[Jason Orendorff [:jorendorff]]

> can I have more detail about the `if (true)` issue?

Jason Laster knows more about this.

Comment 11

[Jason Laster [:jlast]]

I don't feel too strongly about pausing at a do.

With that said, here is the context for pausing at `if (true) {}`. The debugger uses pause points [1] to have some more fine grained control over stepping, which is handled here [2] in the thread actor.

Previously, the engine did not have column positions for control flow, so the server would pause at an IF and report the location as column 0. This was fixed here [3]. Before the fix, we specifically dis-allowed pausing at an IF because in some cases the server would pause at column 0 and then at the IF which was confusing. 

After [3] was landed we started skipping steps at IFs because we disallowed pausing there. I fixed that last week with this PR [4] that re-added pause points at IFs. 



[1] https://docs.google.com/document/d/1wL3If9cC-dpLCaIin4V-0y610WTQJovPtol3WidD8sg/edit
[2] https://searchfox.org/mozilla-central/source/devtools/server/actors/thread.js#537-578
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=1472291
[4] https://github.com/devtools-html/debugger.html/pull/6788/files

Comment 12

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/19dde1cd4e9f9f482fd4649c3bba0f7e2e71f6f3
Bug 1477896 - Add SRC_DO_WHILE replacing 2 SRC_WHILE notes for do-while. r=jandem

Comment 13

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/19dde1cd4e9f