{{ message }}
Repository navigation
"go to cursor location" does not work with ssh connection #332
Description
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.
Activity
apply sourceFileMap for "go to cursor", fixes #332
brownts commentedon May 3, 2022
@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 commentedon May 3, 2022
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:
brownts commentedon May 3, 2022
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
jumpcommand, 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
jumpcommand (likely paired with setting a temporary breakpoint, as described here). And looking at the implementation ofMI2::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 commentedon May 3, 2022
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 ofrr) 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 commentedon May 3, 2022
... 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-continueshould 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 commentedon May 4, 2022
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 commentedon May 4, 2022
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 commentedon May 5, 2022
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 commentedon May 5, 2022
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!
apply sourceFileMap for gotoTargetsRequest