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

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
11 months ago

People

(Reporter: arai, Assigned: arai)

Tracking

Trunk
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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)
I'll fix this once we have DoWhileEmitter and SrcNote::DoWhile{1,2}, converting SrcNote::DoWhile{1,2} to SrcNote::DoWhile
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Depends on: 1456404
here's the patch.
I'll ask review after bug 1456404 and bug 1477621 got resolved.
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.
: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, ...
  );
Flags: needinfo?(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)`!
Flags: needinfo?(jorendorff)
(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?
Flags: needinfo?(jorendorff)
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?
* 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`)
Attachment #8994410 - Attachment is obsolete: true
Attachment #8999575 - Flags: review?(jdemooij)
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.
Attachment #8999575 - Flags: review?(jdemooij) → review+
> can I have more detail about the `if (true)` issue?

Jason Laster knows more about this.
Flags: needinfo?(jorendorff) → needinfo?(jlaster)
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
Flags: needinfo?(jlaster)
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
https://hg.mozilla.org/mozilla-central/rev/19dde1cd4e9f
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.