WIP: Breakpoint UI

[pfitzseb]

While this implementation basically works, there's still a lot to figure out:

• julia-client shouldn't need to maintain a stack of set breakpoints; it'd probably be best to only have that in julia so there's less potential for both sets of breakpoints to diverge
• Breakpoints should probably be unset when their marker changes (i.e. line inserted above the breakpoint or soemthing like that). Maybe re-set them at the new location afterwards?
• Only saved files should have their breakpoints actually applied, because that is the state Gallium sees. Maybe we should save files automatically when setting a breakpoint or something like that (which would require separate stacks, now that I think about it...)?
• Conditional breakpoints.
• Breakpoint pane.
• Other stuff I didn't yet think about, but this will be kinda tricky to get right I think.

Just for reference (need to get those if you want to try this branch):

• https://github.com/JunoLab/Atom.jl/tree/sp/breakpoints
• https://github.com/JunoLab/atom-ink/tree/sp/breakpoints

[ufechner7]

Any idea, why the Travis CI check fails?

[pfitzseb]

Because it isn't configured correctly -- see #236 for progress on that.

[MikeInnes]

I think you make some good points, and at the same time this will take some thought and iteration to work out in detail, so just getting something that does the job can be the priority.

julia-client shouldn't need to maintain a stack of set breakpoints; it'd probably be best to only have that in julia so there's less potential for both sets of breakpoints to diverge

I don't necessarily agree with this. For example, you can imagine a use case where we run a one-shot script with a new julia instance each time – I think setting the breakpoints on boot makes sense there. Though in future we'll probably have the debugger itself in its own process which will achieve the same effect.

[pfitzseb]

Yeah, this is definitely not easy to get right. Also, I haven't really used debuggers yet, so I'd appreciate any input regarding expected behaviour and such.

julia-client shouldn't need to maintain a stack of set breakpoints; it'd probably be best to only have that in julia so there's less potential for both sets of breakpoints to diverge

I don't necessarily agree with this. For example, you can imagine a use case where we run a one-shot script with a new julia instance each time – I think setting the breakpoints on boot makes sense there. Though in future we'll probably have the debugger itself in its own process which will achieve the same effect.

Yup, I don't agree with that either now that I've been thinking about it for a bit :D

[pfitzseb]

Active breakpoint:

Inactive breakpoint:

[pfitzseb]

It'd be great if someone could play around with this for a bit -- I think this should work well as is, but am not totally sure. Also, I'd rather implement the remaining two items (debugger pane item and conditional breakpoints) in a separate PR, mostly because I'm not sure yet how to best implement that.

[MikeInnes]

The code here is looking good, with the minor downside that I can't get it to work :)

I have a separate set of branches (omm/breakpoints) where I'm going to strip this down a bit to get the minimal thing working and fairly robust. There are some good ideas in the synchronisation code and stuff which I definitely don't want to throw out, but also some hard questions to answer. It's ok if in our first pass we simply ask users to be careful, and apply the extras later on.

To outline the issue, the sync problem is particularly tricky because (a) the code in the buffer, (b) the code on disk and (c) the loaded code in Julia can all get out of sync. The current approaches address a!=b well but not so much a!=c. I think we need to see more of how Gallium behaves with no sync handling in order to figure out the tradeoffs involved here.

[pfitzseb]

Are you on gallium master? If not, it's not possible to remove source breakpoints, which kinda hampers the toggleability a bit :D

Apart from that: a!=b isn't actually a problem, only a!=c -- and I apparently thought b==c was true, so I wrote code that handled that case. If were talking about source breakpoints only, then Gallium's behaviour for unsynched breakpoints is straightforward -- it halts execution at the wrong line. Imho it's reasonable to say "if Gallium doesn't place you at your breakpoint, make sure to save the file and reload your code", but maybe we could find some way to check for a==c (maybe something like setting breakpoints on the method definition nearest the breakpoint and then stepping down ourselves?). Will have to think about that a bit...

All that said: For the little bit I used this implementation, it works pretty well for Base code and mostly well for actively developed code, but YMMV.

[MikeInnes]

Ah, I better grab Gallium master then – that's at least part of the issue. I think that was exposing a bug in breakpoint removal as well.

I mention a because I know that ASTInterpreter loads code from disk at times; but I'm not sure if that's only for display in the repl, in which case it doesn't affect us. I agree we can probably do something smarter about this (perhaps keeping track of where BPs originated?), but we'll have to be really careful not to play whack-a-mole with confusing cases – being consistently wrong might end up being the lesser evil. (But I also agree we can tell people to be wary and figure that out later – as this is working well we should use it ASAP.)

I also want to fix the loading indicator; if it's always on while in debug mode, it's not particularly useful.

[MikeInnes]

The UI parts of this PR are first class – I really like the way the breakpoint registry is set up, among other things. I've done a bunch of shuffling things around and ironing out but it's basically as-is.

For now, I've simplified the backend parts quite a lot, in particular the approach to syncing breakpoints. I've no doubt that we'll want to do smarter things eventually, but with the debugger itself in a very beta stage I think it's important to keep things simple. For now, setting a breakpoint is exactly like calling Gallium.breakpoint(file, line), which will make it easier to track down the first wave of issues.

[pfitzseb]

Awesome, thanks for fixing this up -- I'll have a look at the new logic as soon as I can.