Breakpoints are randomly not set by the extension #307
Comments
|
After some digging and testing (with my very limited typescript knowledge), I do not understand the necessity of Lines 203 to 265 in 15f7bae Removing the variable and directly calling the cb() seems to fix it for me: protected setFunctionBreakPointsRequest(response: DebugProtocol.SetFunctionBreakpointsResponse, args: DebugProtocol.SetFunctionBreakpointsArguments): void {
const cb = (() => {
const all = [];
args.breakpoints.forEach(brk => {
all.push(this.miDebugger.addBreakPoint({ raw: brk.name, condition: brk.condition, countCondition: brk.hitCondition }));
});
Promise.all(all).then(brkpoints => {
const finalBrks = [];
brkpoints.forEach(brkp => {
if (brkp[0])
finalBrks.push({ line: brkp[1].line });
});
response.body = {
breakpoints: finalBrks
};
this.sendResponse(response);
}, msg => {
this.sendErrorResponse(response, 10, msg.toString());
});
}).bind(this);
cb();
}
protected setBreakPointsRequest(response: DebugProtocol.SetBreakpointsResponse, args: DebugProtocol.SetBreakpointsArguments): void {
const cb = (() => {
this.miDebugger.clearBreakPoints().then(() => {
let path = args.source.path;
if (this.isSSH) {
// trimCWD is the local path, switchCWD is the ssh path
path = systemPath.relative(this.trimCWD.replace(/\\/g, "/"), path.replace(/\\/g, "/"));
path = resolve(this.switchCWD.replace(/\\/g, "/"), path.replace(/\\/g, "/"));
}
const all = args.breakpoints.map(brk => {
return this.miDebugger.addBreakPoint({ file: path, line: brk.line, condition: brk.condition, countCondition: brk.hitCondition });
});
Promise.all(all).then(brkpoints => {
const finalBrks = [];
brkpoints.forEach(brkp => {
// TODO: Currently all breakpoints returned are marked as verified,
// which leads to verified breakpoints on a broken lldb.
if (brkp[0])
finalBrks.push(new DebugAdapter.Breakpoint(true, brkp[1].line));
});
response.body = {
breakpoints: finalBrks
};
this.sendResponse(response);
}, msg => {
this.sendErrorResponse(response, 9, msg.toString());
});
}, msg => {
this.sendErrorResponse(response, 9, msg.toString());
});
}).bind(this);
cb();
}Not sure whether this has some unwanted side effects or whether the |
|
the debugReady seems to be set once the Running immediately instead of waiting for the event could mean that some configuration is skipped and with that bugs with the break insert functions could arise for example when the path translations aren't sent yet to GDB. I think your solution here is working for you because you are giving the callback more time in the race to finish, but it's not a solid solution and it will break once the break command is dependent on some of the previous setup commands. (like with SSH, WSL or MSYS/MinGW setups) I would have thought this should be fixed on master with #304 being merged, after reading it might actually be that that is the cause for the race being lost by the break function now? (cc @brownts)
|
I thought so that I missed something, thanks for the explanation. It could be fixed on master I'm not a 100% sure. But this could be a separate issue. |
|
I took a look at where the "debug-ready" event is emitted and in this case it's the That's my suspicion about what is going on here. It seems to me that the setting of the A simple test to prove out this theory would be to add a handler for the "debug-ready" event that just sets this flag. Something like the following (untested of course 😉) added to the mibase.ts:initDebugger): this.miDebugger.on("debug-ready", (() => { this.debugReady = true; })); |
|
Sorry for the late reply. Adding a log message at emit debug-ready (mi2.ts) and setting breakpoint request (mibase.ts) seems to verify your assumption. Bad case: Good case: adding your proposal to initDebugger seems to fix this issue! protected initDebugger() {
[...]
this.miDebugger.on("thread-exited", this.threadExitedEvent.bind(this));
this.miDebugger.on("debug-ready", (() => { this.debugReady = true; }));
this.sendEvent(new InitializedEvent());
[...]
}Now both cases work for me! |
|
Hi @JanHildenbrandBosch, no problem on the response time, I'm just glad we understand the problem and were able to get you unstuck. @WebFreak001, I'm wondering why we implement the breakpoint setting in this manner (by potentially registering callbacks with the event system to handle them at a later time). It seems if we just delayed the sending of the "initialized" event back to the editor, we wouldn't have to add callbacks based on the According to the DAP Specification section on "Configuring breakpoint and exception behavior", it indicates:
Per the wording above, the protocol is supposed to be flexible enough to avoid having to "... implement a buffering strategy ...", which seems like what this is doing by adding the callbacks to the event. Currently, we send the "initialized" event during the early part of the "launch" or "attach", but I wonder if we should instead send it when the "debug-ready" event fires. This should guarantee that no breakpoint configuration is sent by the editor until after the debugger has been completely initialized. If we did that, I don't think we'd even need the Maybe sending this event early was necessary when the 50ms timer was in place, however with the current implementation, the debugger will not be commanded to start running until after the configuration has completed. |
|
Just a note: I think it is time for a new release once this issue is set. @brownts would you consider a PR? |
@GitMensch, can we resolve PR #259 before we create a PR for this issue, so we aren't having to resolve merge conflicts? There is some overlap between that PR and what would be changed here. It would be good to incorporate that PR for the next release as well. |
|
@brownts Sure, the related PR is merged now :-) |
Edit: Could be related to #276 (comment) and #268
Although a breakpoint is set in Visual Studio Code before the debugging task is executed, only sometimes the extension will instruct gdb to insert a breakpoint.
Running VSCode Version 1.55.2, gdb (Ubuntu 10.2-0ubuntu1~ 20.04 ~ 1) and extension version 0.25.1 from the VSCode extension marketplace.
gdb --version>= 7.7.1gdbcwdandtargetare properly setlldb --version>= 3.7.1lldb-micwdandtargetare properly setConfiguration:
and
"debug.allowBreakpointsEverywhere": true,Output:
Rerunning it without changing anything sometimes leads to a successful breakpoint insertion:
Adding
"showDevDebugOutput": true,increased the probability that the breakpoints were inserted -> Race Condition?!?Also tested shortly with the latest master (15f7bae), but ran sometimes into issues that the debugging session never started.
The text was updated successfully, but these errors were encountered: