"go to cursor location" does not work with ssh connection

[GitMensch]

The reason is that this call is handled from mi2.ts, while the source conversion is done in mibase.ts.

I have a working version that passes the MI2 instance to MI2DebugSession and then access it to let it do the conversion.
Another alternative would be the other way around - moving the path conversion ssh<->local to be stored and done from the MI2DebugSession.

@WebFreak001 @brownts Are both or one of those "dirty"?
I'm available for doing a PR (I actually have the code for the variants above ;-), just would like to know which way (if one) I shpould take before creating a PR.

[brownts]

@GitMensch, I tried to duplicate this issue after I had applied the changes in #352, but was not able to observe the issue you describe. I tried to duplicate it by right-clicking in the file and selecting "Run to Cursor" during an active debug session. This behavior seemed to work as expected for me, so I was not able to duplicate the problem. Do I need to do something different in order to reproduce this?

[GitMensch]

Nope, when I've tried the "go to" feature led to break-insert -t (MI2.goto); but the "run to cursor", which I think should use this, just creates a "normal" breakpoint now (see debug output, there's even no need to use the debugger to debug the extension as I did; which showed the call event to be a plain "breakpoint add").

The questions here:

• Why does we get an event of "breakpoint add" instead of goto (or is this just a local issue)?
• Is the change itself "correct" or should it be done different?
• Not sure: Why are some commands handled in MI2 while others are in MI2DebugSession?

[brownts]

I apologize for the previous misunderstanding. It looks like this functionality maps to the VSCode "Jump to Cursor" on the right mouse button context menu. To be honest, I've never used this functionality before, but looking in the GDB manual at the jump command, I think I can see where I might use it in the future.

OK, so now that I understand what the issue is, I believe it makes sense that "Run to Cursor" and "Jump to Cursor" are implemented differently. It makes sense that "Run to Cursor" is implemented using a simple "set breakpoint and continue" strategy, as we want to continue execution normally until we get to the cursor, not jump/goto directly to the cursor location. However, for the "Jump to Cursor", we're skipping execution of anything between the current location and where the cursor is. This needs to be handled in the debugger using whatever is appropriate, and likely why the DAP has a special request for this. For GDB, it means using the jump command (likely paired with setting a temporary breakpoint, as described here). And looking at the implementation of MI2::goto, that is exactly how it's implemented (i.e., sets a temporary breakpoint to prevent execution after the jump, and then jumps).

As per your question about the split between MI2 and MI2DebugSession, my observation is that the low-level debugger interaction is captured in MI2 (and MI2_LLDB for diverging functionality...and in fact there is a different LLDB implementation of this 'goto' command), and functionality at a higher level closer to the client (e.g., VSCode) is handled in MI2DebugSession, since MI2DebugSession is derived from the VSCode DebugSession/ProtocolSever/etc. functionality. I'm sure @WebFreak001 probably could shine more light on the design decisions here, but that's how I currently see the split between the two. From a top-down perspective, requests from the client originate in the MI2DebugSession and that ends up calling the MI2 functionality to delegate commands to the debugger. Whether the source file mapping should reside in MI2 or MI2DebugSession might be somewhat arbitrary, but since the awareness of whether the debugger is being run over an SSH connection is kept in the MI2DebugSession, it probably makes sense to keep the source file mapping at that level too.

I'll add my comments about the actual change in the pull request.

[GitMensch]

See, we were both confused by this function :-)
But yes, both" run until" and "go to (jump)" are useful; for the later always look out for possible stack trashes or resource leak - best to only jump within the same method and force a return where in an upper method.

The most common use case for jump is going backwards without total record full / record btrace (or commonly more better: use of rr) to inspect a previous state again - or to jump backwards, fix the "code" by assigning variables as they should be, then continue.

I totally agree that the design should be better stated, especially as I'm coming back to the code after weeks without ever "been in completely" I wonder about the same design issues (which partially come from the vscode<->DAP part, partially from the "multiple debuggers" part and partially because "that's the way @WebFreak001 wants it", and partially because there was no better design found yet).
Having more class and method comments and the general design "as good as known" in HACKING.md would help a lot. @brownts Would you mind adding your current view there so we can improve it step by step?

For the issue at hand: if the SSH awareness is in MI2DebugSession then the source file mapping should be done there completely (with the actual code for that in a separate file - as it is now).

[GitMensch]

... thinking about it more: the "run until" part is still created with a non-temporary breakpoint - that seems wrong. Either a temporary breakpoint and exec-continue should be used there or -exec-until (maybe let the user decide, some people are confused when they land somewhere else (because the scope was left), others are confused when the inferior runs and exits because the place is never reached.

[brownts]

... thinking about it more: the "run until" part is still created with a non-temporary breakpoint - that seems wrong. Either a temporary breakpoint and exec-continue should be used there or -exec-until (maybe let the user decide, some people are confused when they land somewhere else (because the scope was left), others are confused when the inferior runs and exits because the place is never reached.

I don't believe the DAP is rich enough to support that. There doesn't seem to be any concept of a temporary breakpoint in the DAP. The "Run to Cursor" is not a specific DAP request, but is instead constructed from the SetBreakpoints and Continue requests. Therefore, the editor is responsible for using the SetBreakpoints request to both add and remove the breakpoint.

[GitMensch]

OK, so we can't do anything about that for now, leaving the original issue that we need the sourceFileMap handling somewhere and possibly a move of some functions between MI2DebugSession <-> MI2.

[brownts]

I don't believe any functions need to be moved. I suggested a 1-2 line change in my PR comment which just adds the mapping in the MI2DebugSession class. That would be consistent with where the other breakpoint and stack frame mapping is performed.

[GitMensch]

Ah, now I see. In this case... retested again with master -> got "no source file x. found" (the request was -break-insert - t x:\\some\\file.c), then applied your suggested change - works like a charm and is much clearer.
Thank you!