Closed Bug 1003554 Opened 11 years ago Closed 9 years ago

Strange behavior when stepping through try statement

Categories

(DevTools :: Debugger, defect, P1)

Product:
DevTools
Component:
Debugger
Version:
28 Branch
Platform:
x86_64
Linux
Type:
defect

Tracking

(firefox44 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 44
Tracking Flags:
Tracking Status
firefox44 --- fixed

People

(Reporter: skybrian, Assigned: tromey)

References

(Blocks 1 open bug)

Details

(Whiteboard: [polish-backlog][difficulty=medium])

Attachments

(1 file, 15 obsolete files)

make entry points correspond to entries in the line table
19.68 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1847.132 Safari/537.36 Steps to reproduce: Using this script: <script> var at = null; function runTry() { at = 4; try { at = 6; throw "something"; } catch (e) { at = 9; } finally { at = 11; } at = 13; } </script> <button onClick="runTry()"/>Try</button> Set a breakpoint at line 4 and single-step though the script. Actual results: The debugger visits Line 9 twice, before and after the finally block. (However, it isn't executed the second time.) Expected results: Line 9 shouldn't be visited after the finally block.
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Component: JavaScript Engine → Developer Tools: Debugger
Product: Core → Firefox
I've put the sample code here for easier testing: http://htmlpad.org/stepping-bug/
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: dbg-breakpoint
Summary: strange behavior when stepping through try statement → Strange behavior when stepping through try statement
I've appended the bytecode for this example. If you set a breakpoint at line 11 ("a = 11" in the finally clause), this corresponds to PC=0085. Now if you step, the interpreter will execute 'int8 11', 'setgname "at"', 'pop', and then 'retsub' -- stopping after this instruction at PC=0038 ('goto 95'). This stop is why the debugger shows that the step moves to line 9 again. I am not sure what an appropriate fix would be. One idea is that since "retsub" is only used when generating a "finally", the interpreter could special case this for the debugger. That is, when single-stepping, don't let a "retsub" stop the single step, but instead keep stepping until the line number changes again. However, my current understanding is that the line-number logic is currently in actors/script.js, so this would be pretty ugly. It would work ok if script.js can inspect the bytecode, but I don't know if that is possible. There are lots of ways to do this in the abstract (mark some lines as artificial, change the Debugger API to handle this automatically, pass more information to the onStep callback, etc); but I don't have the context to know what would be the most in keeping with the overall direction. js> dis(runTry) flags: loc op ----- -- main: 00000: bindgname "at" 00005: int8 4 00007: setgname "at" 00012: pop 00013: try 43 (+30) 00014: bindgname "at" 00019: int8 6 00021: setgname "at" 00026: pop 00027: string "something" 00032: throw 00033: gosub 79 (+46) 00038: goto 95 (+57) 00043: uninitialized 00044: initlexical 0 00048: pop 00049: exception 00050: initlexical 0 00054: pop 00055: bindname "at" 00060: int8 9 00062: setname "at" 00067: pop 00068: debugleaveblock 00069: gosub 79 (+10) 00074: goto 95 (+21) 00079: finally 00080: bindgname "at" 00085: int8 11 00087: setgname "at" 00092: pop 00093: retsub 00094: nop 00095: bindgname "at" 00100: int8 13 00102: setgname "at" 00107: pop 00108: retrval Source notes: ofs line pc delta desc args ---- ---- ----- ------ -------- ------ 0: 3 0 [ 0] newline 1: 4 0 [ 0] colspan 1 3: 4 13 [ 13] xdelta 4: 4 13 [ 0] newline 5: 5 13 [ 0] try offset to jump 25 7: 5 14 [ 1] newline 8: 6 14 [ 0] colspan 2 10: 6 27 [ 13] xdelta 11: 6 27 [ 0] newline 12: 7 27 [ 0] colspan 2 14: 7 43 [ 16] xdelta 15: 7 43 [ 0] newline 16: 8 55 [ 12] xdelta 17: 8 55 [ 0] newline 18: 9 55 [ 0] colspan 2 20: 9 79 [ 24] xdelta 21: 9 79 [ 0] newline 22: 10 79 [ 0] colspan 11 24: 10 80 [ 1] newline 25: 11 80 [ 0] colspan 2 27: 11 95 [ 15] xdelta 28: 11 95 [ 0] setline lineno 13 30: 13 95 [ 0] colspan 1 32: 13 108 [ 13] xdelta 33: 13 108 [ 0] colspan 8 Exception table: kind stack start end catch 0 14 43 finally 0 14 79
Thanks for the detailed analysis Tom! I think handling this in the Debugger API or even lower inside SpiderMonkey would be preferred. Jim, what do you think?
Flags: needinfo?(jimb)
I talked to Jim yesterday and he suggested modifying the bytecode emitter to use a different location for the "goto". In particular it could be set to the line at the end of the finally block.
Flags: needinfo?(jimb)
(In reply to Tom Tromey :tromey from comment #4) > I talked to Jim yesterday and he suggested modifying the bytecode emitter > to use a different location for the "goto". In particular it could be > set to the line at the end of the finally block. Could I convince you to work on this?
(In reply to Eddy Bruel [:ejpbruel] from comment #5) > Could I convince you to work on this? Sure!
Assignee: nobody → ttromey
This is what I'm going to test.
(In reply to Tom Tromey :tromey from comment #4) > I talked to Jim yesterday and he suggested modifying the bytecode emitter > to use a different location for the "goto". In particular it could be > set to the line at the end of the finally block. After implementing this and trying it out on a variety of test cases, I think it is not the best approach. I applied this to various artificial instructions generated by the bytecode emitter, and it does make stepping behavior more natural. However, because it changes line numbers for some instructions, it also changes the notion of what is an entry point. E.g., in the example in this bug, if you set a breakpoint on the first line of the "finally" clause, you will see two stops there. I think a better approach might be to introduce an "artificial" note into the line table, and then have the debugger APIs know to ignore artificial instructions. This will make stepping work properly (the debugger will step over artificial instructions), while not changing the reported entry points.
Comment on attachment 8532678 [details] [diff] [review] fix source location of "goto" to end of a "try" Trying another approach; but this one was mildly wrong anyhow, as it should have used UpdateSourceCoordNotes at least, and was missing some pieces (reported in other bugs) as well.
Attachment #8532678 - Attachment is obsolete: true
Plan B for this bug (and bug 1013219 et al) was to add a SRC_ARTIFICIAL source note. I added this as a "gettable" source note type and modified various spots to either add it or to examine it. However, this approach had an issue. In some cases the bytecode emitter may emit a number of artificial instructions, sometimes indirectly via other calls -- meaning that the resulting patch would require adding a number of parameters in various spots in the emitter. There's also a potential issue that an instruction may only have a single gettable note, so if any such artificial note also required another note, we would be stuck. It seemed simpler, and not any less clean, to instead treat SRC_ARTIFICIAL as a kind of line number note, meaning "no line number". I'll try this approach next.
This patch implements "Plan C" -- adding a new SRC_ARTIFICIAL source note which acts a bit like a line number note. In this approach, SRC_ARTIFICIAL does not change the computation of the "source line" at all. However, it does set a separate "artificial" flag when walking the line table. This patch then marks up various spots in BytecodeEmitter with SRC_ARTIFICIAL notes. Finally it changes the debugger to properly handle artificial instructions: a stepping operation will never stop on an artificial instruction, and an artificial instruction will never be reported as an "entry point". The new test cases exercise this code. Note there is one spot I did not update -- the code generated to terminate a function that doesn't have an explicit "return" or "yield". This is bug 1013219; I separated this part out into another patch because it may warrant different treatment.
(In reply to Tom Tromey :tromey from comment #8) > However, because it changes line numbers for some instructions, it also > changes > the notion of what is an entry point. One thing I realized recently is that one way this happens is because jsopcode.h:FlowsIntoNext returns true for JSOP_GOSUB. This means that for a sequence: gosub <finally> ; line 3 goto <after-try> ; line 7 ... FlowGraphSummary will think there is an edge from line 3 to line 7. So, the gosub will appear to be an entry point. However, this can't actually happen -- it is a spot where the flow graph doesn't fully model the underlying code. One argument for trying harder with the line-number approach is that it can mean an extra stop when a scope is popped. This would let a user investigate locals after all modifications but before they are removed. This current patch works by changing the internals to avoid stops at artificial locations. It occurred to me, though, that if we wanted to support (js-)instruction-level single-stepping, then this decision would have to be delegated to the debugger API user. In this case one way to go would be to extend the API proposed in bug 863089 to also report whether a given instruction is artificial.
This is a tiny update from the previous patch. In the earlier patch, BytecodeEmitter.cpp:UpdateLineNumberNotes always emitted a SRC_SETLINE note when switching out of the artificial state. However, this was only needed when the line number also remained the same. This version of the patch makes the line-number resetting a bit more efficient by special-casing the SRC_SETLINE behavior.
Attachment #8535785 - Attachment is obsolete: true
Attachment #8536702 - Flags: review?(jimb)
Thank you for working on this Tom. You do great work on fixing these source note issues!
Bytecodes associated with no particular source location introduce a new case that has to be handled higher up in the toolchain; I'd like to avoid that if possible. Would it be possible to associate the 'goto' with the line of its target instruction? Or would that entail rewriting prior source notes, and be a pain? (Don't we rewrite prior source notes already?)
Comment on attachment 8536702 [details] [diff] [review] mark compiler-generated instructions as SRC_ARTIFICIAL Clearing review to get this off my plate, but I'd readily believe there is more to the situation than I understand, so please find me on IRC or needinfo me if you don't think my prior suggestion is a good idea to pursue.
Attachment #8536702 - Flags: review?(jimb)
(In reply to Jim Blandy :jimb from comment #19) > Bytecodes associated with no particular source location introduce a new case > that has to be handled higher up in the toolchain; I'd like to avoid that if > possible. > > Would it be possible to associate the 'goto' with the line of its target > instruction? Or would that entail rewriting prior source notes, and be a > pain? (Don't we rewrite prior source notes already?) I will give it a try. I think this will still require fixing FlowGraphSummary per comment#14. Should be doable though. There are two potential issues, both related. One is that the current patch sets the artificial flag when leaving a nested scope. Simplest here would be to mark this exit as having the location of the scope's closing brace -- but that means an extra stop. (But as you note, we can look at back-patching the source notes, I'll see about that.) The other issue is patching a retsub. In this case there is no single following line, so I think we'll have to live with the extra stop on the closing brace.
When I wrote comment #14, I thought that the gosub was the only spot introducing new entry points and thus causing weird behavior. However, now I think that is incorrect. Consider the case of a "goto" at the end of the "then" part of an "if" statement. I think if we implement the current plan and give this "goto" the location of the statement following the "if", then the "goto" instruction will appear to be an entry point of that following line. It seems to me that this will mean double stops on such lines -- once on the goto, and once on the "real" entry point. Maybe it would be possible to make the flow graph notice when one entry point for a line post-dominates the other entry points. Then the post-dominating entry point would be the canonical entry point. I suspect that the artificial notes approach would be simpler than this.
Wouldn't it be better to give the goto the same source location as the last statement of the 'then' block?
Having discussed this a bit more, it seems to me that instructions like a then block's final goto, a try block's gosub, and the goto after that gosub, should generally be given the same source location as their predecessors in the flow graph. If we can correct FlowGraphSummary's understanding of gosub, so that it sees an edge from the gosub to its target, and edges from the retsub to the successors of all the gosubs, then it it doesn't seem to me that that should introduce any surprising stops.
(In reply to Jim Blandy :jimb from comment #23) > Having discussed this a bit more, it seems to me that instructions like a > then block's final goto, a try block's gosub, and the goto after that gosub, > should generally be given the same source location as their predecessors in > the flow graph. It seems to me that an instruction can have multiple predecessors but just a single location. For example in a nested if: function f() { if (a) { if (b) { c(); } else { d(); } } else { e(); } print("x"); } Here the "goto" at the end of the outermost "then" has two predecessors -- each branch of the inner "if". There isn't a consistent location to give this goto, at least not one based on its predecessors, I think. One idea might be to teach FlowGraphSummary about this somehow, but then there's the problem of identifying which instructions need this special treatment. I couldn't think of a way to do this without reintroducing the artificiality concept. It is possible to just use the location of the end of the "if" (or whatever) as the location of these artificial instructions. This means extra stops. In the case of blocks the stops will be on the closing brace, which isn't so bad; but I think for single-statement "if"s the user may see weird results. > If we can correct FlowGraphSummary's understanding of gosub, so that it sees > an edge from the gosub to its target, and edges from the retsub to the > successors of all the gosubs, then it it doesn't seem to me that that should > introduce any surprising stops. I suppose we can change FlowGraphSummary to simply never consider retsub instructions as possible entry points. That would solve this issue; though if the block also requires scope-popping then the user will still see a stop on the closing brace.
(In reply to Tom Tromey :tromey from comment #25) > I suppose we can change FlowGraphSummary to simply never consider retsub > instructions > as possible entry points. That would solve this issue; though if the block > also > requires scope-popping then the user will still see a stop on the closing > brace. On irc Jim suggested having FlowGraphSummary notice instructions that cannot produce side effects, and use this information to trim the set of entry points. <jimb> getAllOffsets should report the smallest set of bytecode offsets that dominate all the line's side effects. How's that? <jimb> In other words, here's where you must set breakpoints to catch execution before this line does anything.
We just discussed this on IRC; let me try to summarize our conclusions. Let's put the example from comment 25 in pseudo-bytecode: 1: ... a ... 2: IFEQ 9 3: ... b ... 4: IFEQ 7 5: ... c() ... 6: GOTO 8 7: ... d() ... 8: GOTO 10 9: ... e() ... 10: ... print(x) ... There are two goals here: to make stepping work, and to make breakpoints work. Stepping is driven by source locations: we single-step until the source location changes, and then we pause. Line entry points aren't relevant to stepping. So simply assigning one bytecode the same source line as another ensures that single-stepping from the first to the second will not pause. In that light, we can see that assigning the GOTO at offset 6 the source position of either "c()" (its predecessor) or "print(x)" (its eventual successor), and assigning the GOTO at offset 8 the source position of "print(x)" would cause no unexpected pauses at GOTO instructions while stepping, because every branch shares a source location with its immediate predecessor or successor. Unlike stepping, breakpoints are driven by entry points: Debugger.Script.prototype.getOffsets returns the offsets at which control might enter a given line. And here we have a problem: offset 10 is a successor to both offsets 8 and 9, which have different line numbers. So setting a breakpoint on "print(x)" would get us two pauses after executing "d()". And if we assign offset 6 the source position of "print(x)", then offset 6 becomes a *third* entry point to "print(x)", so we would get *three* breakpoint hits after executing "c()". This all follows directly from the way getOffsets is specified. But I think the problem is the specification, not the source position assignments. Rather, getOffsets should return a smallest set of offsets that dominate the given line's side effects. For "print(x)", the unique smallest set is { 10 }, which is exactly where we'd want to set our sole breakpoint. Debugger.Script.prototype.getAllOffsets and getAllColumnOffsets work on the same principle, and would also need to be changed. The easiest way to implement this, or an acceptable approximation, would be to note that JOF_JUMP opcodes never have side effects, and hence it should always be safe for FlowGraphSummary to slide across such instructions, never including them in a line's set of entry points. (Well, and RETSUB too, which is not JOF_JUMP; perhaps we want a new JOF_ bit for side-effect-free opcodes?)
(In reply to Jim Blandy :jimb from comment #27) > Rather, getOffsets should return a smallest set of offsets that dominate the > given line's side effects. Well, "dominates" isn't the right term here, without some qualification. Also, side effects like leaving block scopes are visible, but not very interesting. I should have said: "getOffsets should return the smallest set of offsets that dominate the given line's interesting side effects, when coming from any other source line." and we should add POPBLOCKSCOPE and DEBUGLEAVEBLOCK to the set of opcodes with our new JOF_ bit, because they're not interesting. Note that this formulation also works with 'for' loop heads, which generate bytecode looking like this: for (a; b; c) d; 1: ... a ... 2: GOTO 5 3: ... d ... 4: ... c ... 5: ... b ... 6: IFNE 3 Here, everything but offset 3 is attributed to the source line "for(...)". If the user sets a breakpoint on "for(...)", we would like to insert breakpoints at offsets 1 and 4. And indeed, { 1, 4 } is "a smallest set of offsets that dominates the given line's interesting side effects, when coming from any other source line". This shows why the "from any other source line" qualification is needed: offset 1 definitely dominates 4; but because control enters 3, which is a different source line, we need to include 4 in our set.
I guess JOF_UNINTERESTING is my preferred form of Tom's SRC_ARTIFICIAL.
In the end, I think "dominates" is just not what we want to say here. In the 'for' loop example, there's no sense in which offset 4 dominates "... a ...". "getOffsets should return the smallest set of offsets at which breakpoints must be set to guarantee a hit after any other source line's interesting side effects and this line's interesting side effects."
(In reply to Jim Blandy :jimb from comment #27) > Stepping is driven by source locations: we single-step until the source > location changes, and then we pause. Line entry points aren't relevant to > stepping. So simply assigning one bytecode the same source line as another > ensures that single-stepping from the first to the second will not pause. I belatedly remembered why I was thinking about stepping in this context: if stepping and breakpoints disagree about stopping points, then users will see double stops when stepping encounters a breakpoint. Like, suppose a "goto" is marked as line N, and the user also sets a breakpoint on line N. The "goto" won't be considered as an entry point, so the breakpoint will be set on the "goto"s target. Stepping will stop on the "goto" (due to its line number) and then a subsequent step will stop again on the goto's target, due to the breakpoint -- but this will appear to the user as a stop on the same line. I thought at first that maybe ignoring JOF_UNINTERESTING opcodes during stepping would work. But "break" and "continue" use JSOP_GOTO and are interesting. So then it looks like maybe looking at the line notes to see the kind of goto would work. But then maybe we're in comment #19 territory again?
That's a really good point. Stepping to a line should definitely land you at the same place where a breakpoint would have been set. Once we reach a new line, it ought to always be safe to continue stepping until one reaches one of the offsets returned by getAllOffsets. I wasn't thinking that stepping should ignore JOF_UNINTERESTING opcodes. Rather, I was imagining that FlowGraphSummary could skip JOF_UNINTERESTING opcodes in an effort to reduce the set of entry points. If the successor of a JOF_UNINTERESTING opcode was on a different line, then you wouldn't do the slide.
(In reply to Jim Blandy :jimb from comment #19) > Would it be possible to associate the 'goto' with the line of its target > instruction? Or would that entail rewriting prior source notes, and be a > pain? (Don't we rewrite prior source notes already?) I spent some time on this yesterday. It turns out that such rewriting is uglier than I expected. There are two issues. First, at various spots we need to make a source note to be filled in later. However, sometimes we need to do this while some other to-be-filled-in source note is also pending. If this "outer" pending source note is filled in, it may grow, invalidating the first index. I'm solving this with a couple of lists of source note markers that are updated as other notes are filled in. The other issue is that source note users scan the table linearly. In order to give one of these instructions the location of its referent, we must add space in the table for new line and column markers and also reset the state so that the next emitted note avoids space optimizations (like SRC_NEWLINE). That is, the line notes table will be somewhat bigger than one might expect. I couldn't think of a decent way to deal with this, I think we'll just have to live with it.
I've implemented the bytecode emitter changes and have moved on to the flow graph changes. One oddity here is that because EmitCatch can emit an artificial JSOP_IFNE, the sliding algorithm must handle instructions with multiple successors. I don't think this will cause real problems, but it does mean that we'll be seeing somewhat more surprising entry point results. For example something like "a = b ? c : d", will have two entry points, not one. I've also patched script.js to only stop when the current offset is an entry point for the line. This seems expensive to me unless we memoize the entry points somewhere. I haven't decided where or how to do that yet. Perhaps they can be stuck on the frame or the script.
Some notes from irc: <jimb> I don't think JSOP_IFNE can ever be an entry point of a statement. <jimb> tromey: because statements don't leave values on the stack, unless you're entering some binding construct. <jimb> As you emit bytecode, along with the source notes you're building, you also keep a vector of offsets within those source notes that need updating, and the values to store in them. <jimb> You never update the source notes while building. <jimb> So the offsets in the vector stay correct. [09:08] <jimb> And the indices in the vector are stable, so you can tell where to put the final values when you find them. <jimb> indices *of* the vector elements, sorry <jimb> or *into* <jimb> anyway bytecode offsets in this patch vector are strictly increasing, either by construction if possible, or by sorting <jimb> Then you walk it from end to front, and drop in all the final values in one pass. [09:10] <jimb> The only reason to consider this is if it'd be simpler overall than what you're doing. <jimb> But it does offer indices that remain stable (into the vector, instead of into the source notes) during bytecode generation. [...] <jimb> Right; all the SetSrcNoteOffset callers would change to use this new patch vector.
(In reply to Tom Tromey :tromey from comment #34) > One oddity here is that because EmitCatch can emit an artificial JSOP_IFNE, > the sliding algorithm must handle instructions with multiple successors. I looked at this again and I convinced myself that we can mark these instructions with the location of the catch body, and so the sliding shouldn't need to handle multiple successors after all. It would be great if somebody else could think about this as well.
I think I have it mostly working (but not polished, I added a helper function rather than JOF_UNINTERESTING and I have to add or rewrite comments). I find it hard to be confident in this patch as it is quite complicated to reason about. I added a hack to FlowGraphSummary to treat gosub and retsub specially to avoid the extra stop in a finally. The retsub treatment in particular is a big hack. Script-getLineOffsets-03.js shows a problem that I haven't figured out how to solve yet. It examines the script "while (i < 5) i++;", expecting a single stop. However, the disassembly is: 00013: goto 43 (+30) 00018: loophead 00019: bindgname "i" 00024: getgname "i" 00029: pos 00030: dup 00031: one 00032: add 00033: pick 2 00035: swap 00036: setgname "i" 00041: pop 00042: pop 00043: loopentry 129 00045: getgname "i" 00050: int8 5 00052: lt 00053: ifne 18 (-35) The issue here is that the initial "goto" is an uninteresting instruction, so entry point computation skips it. Instead it chooses the "loopentry" instruction. However, this instruction is executed for each iteration of the loop. I am not sure yet but maybe this can be handled by making FlowGraphSummary smarter somehow.
(In reply to Tom Tromey :tromey from comment #37) > I am not sure yet but maybe this can be handled by making FlowGraphSummary > smarter somehow. So far I could only think of two ways to handle this. One is the hack of making the sliding dependent on the successor instruction -- that is, ruling out this case by noticing that the target of the "goto" is a "loopentry". This seems like a bad approach to me, as it makes flow graph computation dependent on particulars of bytecode generation. Another approach is to only do sliding if there are multiple possible entry points for a line. This seems promising, but I haven't totally convinced myself it is correct in all cases.
(In reply to Tom Tromey :tromey from comment #37) > The issue here is that the initial "goto" is an uninteresting instruction, > so entry point computation skips it. Instead it chooses the "loopentry" > instruction. However, this instruction is executed for each iteration of > the loop. Why is this a problem? I would like stepping to stop on the condition each time through the loop; isn't that expected?
(In reply to Jim Blandy :jimb from comment #39) > Why is this a problem? I would like stepping to stop on the condition each > time through the loop; isn't that expected? The comment there says: // getLineOffsets treats one-line compound statements as having only one entry-point. // (A breakpoint on a line that only executes once will only hit once.) So basically, I think it's just because this behavior was intended. One weird thing about just deleting the test is that all the other tests in that file pass with the patches in place. That is, a one-line "while" statement will stop on every iteration, but a one-line "for" or "do" loop will stop just once. It seems to me that it would be fine to stop on each iteration. However if we were to do that for "while", then I think it would be best to do it for all kinds of loops. As an aside, as a long term goal it seems like it would be better for the debugger to be indifferent to whether or not statements are collected onto a single line. That is, single-stepping should take me to the next statement, whether it is on the same line or a different line. I'm not sure this can be done with the information currently emitted by the bytecode generator, though.
Attached patch P (obsolete) — — Splinter Review
Here's the WIP patch. Jim said it may be helpful to attach it. It's unclean in various ways, but it does mostly work.
(In reply to Tom Tromey :tromey from comment #40) > As an aside, as a long term goal it seems like it would be better for the > debugger to > be indifferent to whether or not statements are collected onto a single > line. That is, > single-stepping should take me to the next statement, whether it is on the > same line or > a different line. I'm not sure this can be done with the information > currently emitted > by the bytecode generator, though. Definitely. We want to become more statement-oriented, not line-oriented. That would play better with minimized code.
> Definitely. We want to become more statement-oriented, not line-oriented. > That would play better with minimized code. One thought I had was to make FlowGraphSummary only consider a PC as an entry point if it appears in the line table. This would automatically eliminate from consideration any PC corresponding to an "artificial" instruction without requiring changes to the bytecode emitter -- because a line note is never emitted for such instructions. The only wrinkle I can think of offhand here is that to preserve the current line-based behavior, FlowGraphSummary would need a special case for "for" loops. That is, to get the per-line entry points, ignoring columns as it does now, it would have to read the "for" source note to avoid eliminating the update entry point.
We should make sure to include the test case from bug 1129813 in this bug.
(In reply to Jim Blandy :jimb from comment #45) > We should make sure to include the test case from bug 1129813 in this bug. My current patch doesn't affect the behavior of that test at all. I don't think it is the same problem really -- I am going to undup it and put some more info there.
Here is "Plan E", as discussed earlier this week. The basic idea is to limit the set of possible entry points to exactly those PC offsets which have an explicit mention in the line table. These correspond to "interesting" places to stop, according to BytecodeEmitter. Then, script.js is changed so that stepping will only allow a stop at one of these entry points. This eliminates "artificial" instructions from consideration, since such instructions never have an explicit line table entry. A few notes on the patch: * Script-getAllColumnOffsets-06.js only passed due to the bug. It expects a stop on "new Error", but this does not occur because print returns undefined. Instead the stop occurred later, at an instruction that inherited this same column number. * I snuck an obviously missing "else" into BytecodeRangeWithPosition::updatePosition. * It seems nearly possible to remove FlowGraphSummary entirely. I need to think about it a bit more; if you have any thoughts on the topic, let me know. * It was unclear to me whether the script.js change should try to memoize the result of getLineOffsets; or whether this change should be implemented instead inside the vm.
It occurs to me as well that this needs a comment somewhere explaining the reason for checking the line table. I was also unsure if it needs a comment in Debugger.Frame.md explaining the correct way to implement stepping.
(In reply to Tom Tromey :tromey from comment #47) > * It seems nearly possible to remove FlowGraphSummary entirely. I don't think this can be done without adding a special hack for "for" loops somewhere. Otherwise it is difficult to differentiate between: something; something; something; ... which should have a single entry point for the line, and for (something; something; something) ... which should have two.
Added the missing comment.
Attachment #8536702 - Attachment is obsolete: true
Attachment #8556676 - Attachment is obsolete: true
Attachment #8562378 - Attachment is obsolete: true
Blocks: 1139235
Blocks: 1013219
Depends on: 1134198
Blocks: 1129813
Note that bug 1147067 will affect the viability of Plan E. If more source notes are emitted, under Plan E, this will change the set of possible entry points.
Rebased and updated for intervening changes to the parser.
Attachment #8565512 - Attachment is obsolete: true
> Rebased and updated for intervening changes to the parser. Of course I meant "bytecode emitter" here.
(In reply to Tom Tromey :tromey from comment #53) > Note that bug 1147067 will affect the viability of Plan E. > If more source notes are emitted, under Plan E, this will change the > set of possible entry points. After a long discussion with shu, we concluded that there is no problem here. 1147067 may introduce new columns, but should not introduce new line notes. So, the set of entry points should not change.
Blocks: 1151106
Comment on attachment 8588039 [details] [diff] [review] make entry points correspond to entries in the line table I think the whole "Plan E" approach is ready for review now. This first patch lays the groundwork for all the others. However, I think you will probably want to examine them as a group. A couple of the later patches are maybe a bit ugly.
Attachment #8588039 - Flags: review?(jimb)
Comment on attachment 8588039 [details] [diff] [review] make entry points correspond to entries in the line table Review of attachment 8588039 [details] [diff] [review]: ----------------------------------------------------------------- Still reviewing; some comments as I go: ::: js/src/jit-test/tests/debug/Frame-onStep-11.js @@ +2,5 @@ > +// step backward to some earlier statement. > + > +var g = newGlobal(); > +g.eval("function f() {\n" + > + " var x = 0;\n" + Use template strings here; they're much more legible. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/template_strings @@ +31,5 @@ > + } > + }; > +}; > + > +g.eval("f();\n"); You can just say: g.f(); @@ +33,5 @@ > +}; > + > +g.eval("f();\n"); > + > +assertEq(foundLine > debugLine, true); We should be able to tighten this test up a great deal. First, we can use g.evaluate instead of g.eval and pass the 'lineNumber' option, to ensure that our code starts at a fixed line number. Then, we can put the 'debugger' at the top of the function and step through the entire rest of the function's execution, recording each new line number we encounter. Then, we can check that we got the series of line numbers that we expected. If any of those change, we want to know. There's an arraysEqual utility function defined here: https://hg.mozilla.org/mozilla-central/file/883e17fc475f/js/src/jit-test/lib/array-compare.js#l16 ::: js/src/jit-test/tests/debug/Frame-onStep-12.js @@ +1,4 @@ > +// The bug we're testing for is that we could see a stop on an > +// instruction that assumed the line number of whatever was emitted > +// previously, which to the user could appear to be nonsensical. This > +// bit of code arranges for that line number to be something I had a hard time understanding this comment. I think future SpiderMonkey hackers who regress this test will be pretty lost. If the below is correct, could we use it instead? Because our script source notes record only those bytecode offsets at which source positions change, the default behavior in the absence of a source note is to attribute a bytecode instruction to the same source location as the preceding instruction. When control flows from the preceding bytecode to the one we're emitting, that's usually plausible. But successors in the bytecode stream are not necessarily successors in the control flow graph. If the preceding bytecode was a back edge of a loop, or the jump at the end of a 'then' clause, its source position can be completely unrelated to that of its successor. We try to avoid showing such nonsense offsets to the user by requiring breakpoints and single-stepping to stop only at a line's entry points, as reported by Debugger.Script.prototype.getLineOffsets; and then ensuring that those entry points are all offsets mentioned explicitly in the source notes, and hence deliberately attributed to the given bytecode. This bit of JavaScript compiles to bytecode ending in a branch instruction whose source position is the body of an unreachable loop. The first instruction of the bytecode we emit following it will inherit this nonsense position, if we have not explicitly emitted a source note for said instruction. This test steps across such code and verifies that control never appears to enter the unreachable loop. @@ +5,5 @@ > +// nonsensical -- the middle of a loop which cannot be entered. We > +// then use this to ensure that our plan of only letting stepping stop > +// on entry points does not stop on such an instruction. > + > +var bitOfCode = (" debugger;\n" + // +0 Use template strings here, and for the code blocks elsewhere in this test. @@ +25,5 @@ > + frame.onStep = function() { > + let foundLine = this.script.getOffsetLine(this.offset); > + if (this.script.getLineOffsets(foundLine).indexOf(this.offset) >= 0) { > + print("foundLine=" + foundLine); > + assertEq(foundLine <= debugLine + 1 || foundLine >= debugLine + 6, true); Paranoia, but, this test will also pass if no debugger trap ever runs. Use the same 'assertEq(log, ...)' idiom the other Debugger tests use, to ensure we're hitting the traps we expect. We can clear and check the log in testOne.
This looks like a great bug for the devedition-40 list.
Whiteboard: [devedition-40]
Priority: -- → P1
(In reply to Jim Blandy :jimb from comment #58) > Comment on attachment 8588039 [details] [diff] [review] > make entry points correspond to entries in the line table > > Review of attachment 8588039 [details] [diff] [review]: > ----------------------------------------------------------------- > > Still reviewing; some comments as I go: > > ::: js/src/jit-test/tests/debug/Frame-onStep-11.js > @@ +2,5 @@ > > +// step backward to some earlier statement. > > + > > +var g = newGlobal(); > > +g.eval("function f() {\n" + > > + " var x = 0;\n" + > > Use template strings here; they're much more legible. > > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/ > template_strings > > @@ +31,5 @@ > > + } > > + }; > > +}; > > + > > +g.eval("f();\n"); > > You can just say: > > g.f(); > > @@ +33,5 @@ > > +}; > > + > > +g.eval("f();\n"); > > + > > +assertEq(foundLine > debugLine, true); > > We should be able to tighten this test up a great deal. > > First, we can use g.evaluate instead of g.eval and pass the 'lineNumber' > option, to ensure that our code starts at a fixed line number. Then, we can > put the 'debugger' at the top of the function and step through the entire > rest of the function's execution, recording each new line number we > encounter. Then, we can check that we got the series of line numbers that we > expected. If any of those change, we want to know. > > There's an arraysEqual utility function defined here: > > https://hg.mozilla.org/mozilla-central/file/883e17fc475f/js/src/jit-test/lib/ > array-compare.js#l16 > > ::: js/src/jit-test/tests/debug/Frame-onStep-12.js > @@ +1,4 @@ > > +// The bug we're testing for is that we could see a stop on an > > +// instruction that assumed the line number of whatever was emitted > > +// previously, which to the user could appear to be nonsensical. This > > +// bit of code arranges for that line number to be something > > I had a hard time understanding this comment. I think future SpiderMonkey > hackers who regress this test will be pretty lost. If the below is correct, > could we use it instead? > > Because our script source notes record only those bytecode offsets at which > source positions change, the default behavior in the absence of a source > note is > to attribute a bytecode instruction to the same source location as the > preceding > instruction. When control flows from the preceding bytecode to the one we're > emitting, that's usually plausible. But successors in the bytecode stream are > not necessarily successors in the control flow graph. If the preceding > bytecode > was a back edge of a loop, or the jump at the end of a 'then' clause, its > source > position can be completely unrelated to that of its successor. > > We try to avoid showing such nonsense offsets to the user by requiring > breakpoints and single-stepping to stop only at a line's entry points, as > reported by Debugger.Script.prototype.getLineOffsets; and then ensuring that > those entry points are all offsets mentioned explicitly in the source notes, > and > hence deliberately attributed to the given bytecode. > > This bit of JavaScript compiles to bytecode ending in a branch instruction > whose > source position is the body of an unreachable loop. The first instruction of > the > bytecode we emit following it will inherit this nonsense position, if we have > not explicitly emitted a source note for said instruction. > > This test steps across such code and verifies that control never appears to > enter the unreachable loop. > > @@ +5,5 @@ > > +// nonsensical -- the middle of a loop which cannot be entered. We > > +// then use this to ensure that our plan of only letting stepping stop > > +// on entry points does not stop on such an instruction. > > + > > +var bitOfCode = (" debugger;\n" + // +0 > > Use template strings here, and for the code blocks elsewhere in this test. > > @@ +25,5 @@ > > + frame.onStep = function() { > > + let foundLine = this.script.getOffsetLine(this.offset); > > + if (this.script.getLineOffsets(foundLine).indexOf(this.offset) >= 0) { > > + print("foundLine=" + foundLine); > > + assertEq(foundLine <= debugLine + 1 || foundLine >= debugLine + 6, true); > > Paranoia, but, this test will also pass if no debugger trap ever runs. Use > the same 'assertEq(log, ...)' idiom the other Debugger tests use, to ensure > we're hitting the traps we expect. We can clear and check the log in testOne. Jim, this patch has been sitting in your review queue for 2 months. I know how busy you are, of course, but this is a high priority bug for the debugger. Do you think you could get to it some time soon? Thanks for looking into it!
Flags: needinfo?(jimb)
Whiteboard: [devedition-40] → [polish-backlog]
Rebased and updated for review (I think - I haven't looked at it closely recently).
Attachment #8588039 - Attachment is obsolete: true
Attachment #8588039 - Flags: review?(jimb)
I think I didn't finish updating the test case here.
Comment on attachment 8668025 [details] [diff] [review] make entry points correspond to entries in the line table I made some of the requested changes but I think didn't update one of the test cases yet.
Attachment #8668025 - Flags: review?(jimb)
Comment on attachment 8668025 [details] [diff] [review] make entry points correspond to entries in the line table Review of attachment 8668025 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/debug/Frame-onStep-12.js @@ +36,5 @@ > +var dbg = Debugger(g); > + > +g.eval("function nothing() { }\n"); > + > +var badStep = false; This variable doesn't seem to be used, just set twice. @@ +44,5 @@ > + frame.onStep = function() { > + let foundLine = this.script.getOffsetLine(this.offset); > + if (this.script.getLineOffsets(foundLine).indexOf(this.offset) >= 0) { > + print("foundLine=" + foundLine); > + assertEq(foundLine <= debugLine + 1 || foundLine >= debugLine + 6, true); As you noted, this test still needs revision. ::: js/src/vm/Debugger.cpp @@ +5118,5 @@ > if (!result) > return false; > for (BytecodeRangeWithPosition r(cx, script); !r.empty(); r.popFront()) { > + if (!r.frontIsEntryPoint()) > + continue; If I'm reading right, what this patch does is narrow the results to the *intersection* of those sites that both the FlowGraphSummary and the source notes consider interesting. But do we still want to consider the flow graph data at all? What would go wrong if we just disposed of the FlowGraphSummary altogether, and return for each line the array of offsets at which that line becomes current? @@ +5189,5 @@ > if (!result) > return false; > for (BytecodeRangeWithPosition r(cx, script); !r.empty(); r.popFront()) { > + if (!r.frontIsEntryPoint()) > + continue; Similarly.
Attachment #8668025 - Flags: review?(jimb) → review+
(In reply to Jim Blandy :jimb from comment #64) > If I'm reading right, what this patch does is narrow the results to the > *intersection* of those sites that both the FlowGraphSummary and the source > notes consider interesting. But do we still want to consider the flow graph > data at all? What would go wrong if we just disposed of the FlowGraphSummary > altogether, and return for each line the array of offsets at which that line > becomes current? See comment #49. But I am not sure that the conclusion I reached there is correct. I will need to research it.
(In reply to Tom Tromey :tromey from comment #65) > (In reply to Jim Blandy :jimb from comment #64) > > > If I'm reading right, what this patch does is narrow the results to the > > *intersection* of those sites that both the FlowGraphSummary and the source > > notes consider interesting. But do we still want to consider the flow graph > > data at all? What would go wrong if we just disposed of the FlowGraphSummary > > altogether, and return for each line the array of offsets at which that line > > becomes current? > > See comment #49. > But I am not sure that the conclusion I reached there is correct. > I will need to research it. I hacked this up quickly and ran the test suite to see what would happen. I think the analysis in comment #49 is incorrect, but the quick hack yields different results from the current code, so further front end changes are needed.
Whiteboard: [polish-backlog] → [polish-backlog][difficulty=medium]
Ok, I think I understand this a bit better now. The reason flow graph summary is still needed is that we rely on it to deal with column numbers sensibly. My hack was to add a flag to BytecodeRangeWithPosition to make it possible to filter on just line number notes -- that is, exclude column number notes. Then, I removed the use of FlowGraphSummary from DebuggerScript_getAllOffsets and DebuggerScript_getLineOffsets. The rationale for the first change was that we don't want to get offsets at any spot that there is a column-incrementing note. That would yield way too many offsets. However, it turns out we sometimes need these, and this is what the flow graph summary provides. Consider this test in debug/Script-getLineOffsets-05.js: test("if (i === 2)\n" + " log += 'X'; else log += '!';\n"); Here, the test relies on there being two entry points to the line that updates "log". In particular there is one after the "else" -- but this was rejected by the BytecodeRangeWithPosition change. Now, suppose we leave BytecodeRangeWithPosition unchanged. Then a line with multiple statements like "f(); g();" will report two stopping offsets. However, that is not what we want from this API. (Whether or not this is the behavior we want from the debugger, I don't know; see bug 1145747 and bug 1145752). So, on this basis I'm going to go ahead with the patch as is.
Update the test case per review.
Attachment #8668025 - Attachment is obsolete: true
Attachment #8669079 - Flags: review+
Add r= to commit message.
Attachment #8669079 - Attachment is obsolete: true
Attachment #8669083 - Flags: review+
Rebased. Also, slightly modified the new tests to pass -- the changes made here are undone in a later patch in the series.
Attachment #8669083 - Attachment is obsolete: true
Attachment #8669718 - Flags: review+
Flags: needinfo?(jimb)
Updated to fix devtools/server unit tests.
Attachment #8669718 - Attachment is obsolete: true
Update comment
Attachment #8669970 - Attachment is obsolete: true
Comment on attachment 8669972 [details] [diff] [review] make entry points correspond to entries in the line table This adds a "same frame" check to script.js. See the comment for an explanation. Without this check some otherwise reasonable unit tests in devtools/server fail because stepping winds up at unexpected spots. This is also related to bug 1172572.
Attachment #8669972 - Flags: review?(nfitzgerald)
Comment on attachment 8669972 [details] [diff] [review] make entry points correspond to entries in the line table Review of attachment 8669972 [details] [diff] [review]: ----------------------------------------------------------------- Frame-onStep-12 is very thorough! Great stuff, thank you! Just a few nits below. ::: devtools/server/actors/script.js @@ +820,5 @@ > + // and into subsequent calls in the outer frame. E.g., if there > + // is a call "a(b())" and the user steps into b, then this > + // condition makes it possible to step out of b and into a. > + if (this === startFrame && > + this.script.getLineOffsets(this.script.getOffsetLine(this.offset)).indexOf(this.offset) < 0) { This line is a bit of a mouthful and is kind of difficult for me to reason about because the same few words are repeated so much. Can we split this off into a simple helper function with a name that makes it more clear what it does semantically? `offsetIsLineEntryPoint(script, offset)` and then `... && !offsetIsLineEntryPoint(...)` in this conditional? ::: js/src/jit-test/tests/debug/Frame-onStep-11.js @@ +13,5 @@ > + } finally { // +7 > + x = 3; // +8 > + } // +9 > + x = 4; // +10 > + }`); // +11 FYI, you can use `g.evaluate("...", { lineNumber: 0 })` to specify a start line, so that you don't have to do the line delta arithmetic. You've already written the tests this way, so I leave it up to you to decide if it is worth it to rewrite. https://dxr.mozilla.org/mozilla-central/source/js/src/shell/js.cpp#4528 @@ +17,5 @@ > + }`); // +11 > + > +var dbg = Debugger(g); > + > +let foundLines = ''; Also, I know the debugger's jit-tests really love concatenating esoteric strings, but this could just be an array of line numbers. Again, I leave it up to you, since this doesn't really matter much. ::: js/src/jit-test/tests/debug/Frame-onStep-12.js @@ +79,5 @@ > +// Test the instructions at the end of a "catch". > +testOne("testCatchFinally", > + `try { > + throw new TypeError(); > + } catch (e if e instanceof TypeError) { Conditional catches are a spidermonkey-ism, not part of any standard, and therefore are deprecated and slated for removal sooner or later. Can this test have only one catch which runs the bitOfCode? Is there a reason we need the conditional catch and the empty catchall? ::: js/src/vm/Debugger.cpp @@ +4898,2 @@ > private: > + bool updatePosition() { Add a note about what true/false return means. Easy to make the mistake of assuming that it means success/failure if you don't read too closely. Maybe this should just mutate isEntryPoint directly, rather than return the value so that popFront can update isEntryPoint? That would make sense to me.
Attachment #8669972 - Flags: review?(nfitzgerald) → review+
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #76) > > + this.script.getLineOffsets(this.script.getOffsetLine(this.offset)).indexOf(this.offset) < 0) { > > This line is a bit of a mouthful and is kind of difficult for me to reason > about because the same few words are repeated so much. Can we split this off > into a simple helper function with a name that makes it more clear what it > does semantically? `offsetIsLineEntryPoint(script, offset)` and then `... && > !offsetIsLineEntryPoint(...)` in this conditional? Yeah, I'll do that. On irc Jim suggested a follow-up bug to collapse this idiom into a single method on Debugger.Script; I'll file one. > Also, I know the debugger's jit-tests really love concatenating esoteric > strings, but this could just be an array of line numbers. Again, I leave it > up to you, since this doesn't really matter much. This was explicitly requested in an earlier review, see comment #58. > Conditional catches are a spidermonkey-ism, not part of any standard, and > therefore are deprecated and slated for removal sooner or later. > > Can this test have only one catch which runs the bitOfCode? Is there a > reason we need the conditional catch and the empty catchall? This was just to be thorough when testing the emitter changes. I think modifying it is fine though.
Updated per review. I left one instance of the SpiderMonkey catch extension, as without it the particular test doesn't make sense; when the extension is deleted, the test can be as well. I added a comment to explain this.
Attachment #8669972 - Attachment is obsolete: true
Attachment #8670301 - Flags: review+
This doesn't work correctly with source-mapped sources; which manifests in one of the debugger pretty-print test cases. The issue is that the new test in script.js uses getLineOffsets, but in the source-mapped case we need to look at column offsets. I'm still investigating a fix. Maybe bug 863089 would help, but I am not sure yet.
I think bug 863089 is the way to go. It will let me replace the script.js change with a simple call to getOffsetLocation -- I plan to add an "is an entry point" property to the returned object. This may also suffice to close bug 1211906 as well.
Depends on: 863089
Add isEntryPoint and use that instead in script.js. Now we also don't need FlowGraphSummary in getAllColumnOffsets, either.
Attachment #8670301 - Attachment is obsolete: true
Attachment #8675843 - Flags: review?(nfitzgerald)
Comment on attachment 8675843 [details] [diff] [review] make entry points correspond to entries in the line table Review of attachment 8675843 [details] [diff] [review]: ----------------------------------------------------------------- \o/ ::: devtools/server/actors/script.js @@ +821,5 @@ > + // is a call "a(b())" and the user steps into b, then this > + // condition makes it possible to step out of b and into a. > + if (this === startFrame && > + !this.script.getOffsetLocation(this.offset).isEntryPoint) { > + return undefined; What is an entry point in "normal" (aka hand written) JS is not necessarily an entry point in minified-but-now-pretty-printed JS or more generally any source mapped JS. I think it makes sense to move this check after the source mapping related things below, or check this._options.useSourceMaps here.
Attachment #8675843 - Flags: review?(nfitzgerald) → review+
(In reply to Tom Tromey :tromey from comment #82) > Now we also don't need FlowGraphSummary in getAllColumnOffsets, either. This turned out to be mistaken. One case I found where this is still needed is when there is a "return" instruction and then a following "retrval". In this case, the retrval is dead -- and the flow graph is what excludes it from consideration as an entry point. There may be other cases, I am not sure. This particular one could be fixed by changing the bytecode emitter not to emit a dead retrval.
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #83) > Comment on attachment 8675843 [details] [diff] [review] > make entry points correspond to entries in the line table > > Review of attachment 8675843 [details] [diff] [review]: > ----------------------------------------------------------------- > > \o/ > > ::: devtools/server/actors/script.js > @@ +821,5 @@ > > + // is a call "a(b())" and the user steps into b, then this > > + // condition makes it possible to step out of b and into a. > > + if (this === startFrame && > > + !this.script.getOffsetLocation(this.offset).isEntryPoint) { > > + return undefined; > > What is an entry point in "normal" (aka hand written) JS is not necessarily > an entry point in minified-but-now-pretty-printed JS or more generally any > source mapped JS. > > I think it makes sense to move this check after the source mapping related > things below, or check this._options.useSourceMaps here. I think source mapping can't have an impact here. That is, there is no way for source mapping to introduce a new potential stopping point where isEntryPoint is false. The way I think about it is that the only candidate offsets for a stopping point are those PCs that have an explicit entry in the line table. All other PCs just inherit their location from the most recently seen line note. Since source mapping works solely with source locations and not the PCs, I think it is safe to always do this check early. I'm not sure if this explanation is very clear. I will add a bigger comment here explaining the situation.
Comment on attachment 8677548 [details] [diff] [review] make entry points correspond to entries in the line table A few updates here. Testing a later patch in the series revealed that (ugh!) I got the condition for isEntryPoint completely backwards. I fixed this and updated the test case included in this patch to be sure to catch the problem. I was also mistaken when I thought we could remove some uses of FlowGraphSummary. See comment #84. This could be the subject of a future optimization. I left the new condition in script.js where it is, see comment #85.
Attachment #8677548 - Flags: review?(nfitzgerald)
(In reply to Tom Tromey :tromey from comment #85) > I think source mapping can't have an impact here. That is, there is no way for > source mapping to introduce a new potential stopping point where isEntryPoint > is false. Maybe I am misunderstanding, in which case please help me understand :) Here is my current understanding: we receive minified code like this: <stmt1>; <stmt2>; <stmt3>; and we prettify and source-map it into this: <stmt1>; <stmt2>; <stmt3>; Now, (assuming none of these statements are loops or function decls) the only bytecodes that would be considered an entry point to the non source mapped line are associated with stmt1. When we step in the minified/non-source-mapped code, we will just continue to the entry point of the next line and continue past stmt2 and stmt3. However, when stepping through the source-mapped statements we do wish to stop on each statement and line. Those *are* "new potential stopping points"! Just realized something: I believe we do the "continue-till-on-a-new-line" in the actor, not the Debugger API level. Perhaps you were only talking about the Debugger API level? In which case, I think the scenario I just explained is nullified? > The way I think about it is that the only candidate offsets for a stopping > point are those PCs that have an explicit entry in the line table. All other > PCs just inherit their location from the most recently seen line note. > > Since source mapping works solely with source locations and not the PCs, I > think it is safe to always do this check early. I think part of the problem is that the PCs that get an explicit entry in the source notes are affected by whitespace :(
Comment on attachment 8677548 [details] [diff] [review] make entry points correspond to entries in the line table Review of attachment 8677548 [details] [diff] [review]: ----------------------------------------------------------------- Regardless of the presence of further complications, I think we should still land this patch now.
Attachment #8677548 - Flags: review?(nfitzgerald) → review+
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #88) > Maybe I am misunderstanding, in which case please help me understand :) > > Here is my current understanding: we receive minified code like this: > > <stmt1>; <stmt2>; <stmt3>; > > and we prettify and source-map it into this: > > <stmt1>; > <stmt2>; > <stmt3>; > > Now, (assuming none of these statements are loops or function decls) the > only bytecodes that would be considered an entry point to the non source > mapped line are associated with stmt1. I think this should still be ok, because the script.js change is considering "column" entry points (just like D.S.getAllColumnOffsets). So, I think the first instruction of each <stmt*> will be considered an entry point in this sense, and so the early return will not trigger. For more reassurance, see comment #80. The earlier line-based approach was regressing devtools/client/debugger/test/mochitest/browser_dbg_pretty-print-08.js I think this is basically the case described above. It's true that some logical mappings can't be expressed. However, this is a more fundamental problem. For example suppose you had a bit of lisp like: (setf value (+ x y)) Here the translation might be: value = x + y; Now, in Lisp it's perfectly sensible to step into the (+) form. However, this translation doesn't admit the possibility, because the "+" isn't considered an entry point -- all the instructions making up this expression share one entry in the line table. That is, the debugger could single-step through each instruction, but since they all share a source location, they would necessarily all map to the same thing via source maps; and so no extra stop could occur. A translator can give guidance by emitting separate statements: let tmp99237 = x + y; value = tmp99237; This is unwarranted chumminess with the bytecode emitter; but something that could maybe be addressed by the source map spec. Another approach would be to have much fuller debug information, like emitting a location for nearly every operation; then the source map could reflect whatever translations are desired. Future work here should probably also take bug 1145747 and bug 1145752 into consideration. For those it would be good to have a way to find the end of the current "statement" so that we could highlight it in the debugger when "column stepping".
(In reply to Tom Tromey :tromey from comment #90) > (In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #88) > > > Maybe I am misunderstanding, in which case please help me understand :) > > > > Here is my current understanding: we receive minified code like this: > > > > <stmt1>; <stmt2>; <stmt3>; > > > > and we prettify and source-map it into this: > > > > <stmt1>; > > <stmt2>; > > <stmt3>; > > > > Now, (assuming none of these statements are loops or function decls) the > > only bytecodes that would be considered an entry point to the non source > > mapped line are associated with stmt1. > > I think this should still be ok, because the script.js change is considering > "column" entry points (just like D.S.getAllColumnOffsets). So, I think the > first instruction of each <stmt*> will be considered an entry point in this > sense, > and so the early return will not trigger. > > For more reassurance, see comment #80. The earlier line-based approach was > regressing > devtools/client/debugger/test/mochitest/browser_dbg_pretty-print-08.js > I think this is basically the case described above. Ok, that clears everything up for me, thank you. > Another approach would be to have much fuller debug information, like > emitting a location for nearly every operation; then the source map could > reflect whatever translations are desired. IIRC, the reason we don't keep full location info is a memory optimization. Shu has talked about a mode to retain it all for better locations when profiling, we may be able to piggy back on that if it pans out (or we could decide to make it pan out ourselves, of course).
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/093802a6d8ae
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
https://hg.mozilla.org/integration/mozilla-inbound/rev/c11218a05463e65878b122132344d04be680cfef Fix a broken JS test. It landed in rev 093802a6d8ae (bug 1003554) and was apparently fine until it was merged to m-c/m-i, where it probably collided with rev bug 1217099 or bug 1217001. no_r=bustage to a CLOSED TREE.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: