debug: fallback to mark breakpoints verified on stopped/breakpoint events #1989
Conversation
amanthatdoescares
requested review from
0xTim,
adam-fowler and
award999
as code owners
award999
left a comment
award999
left a comment
There was a problem hiding this comment.
@amanthatdoescares thanks so much for the PR 🙏 I have some initial comments here to get you started. @matthewbastien is more familiar with the debugging codebase so let's have him take a look to.
Also the open todo for updating changelog in the PR template, you just need to do something like 85d5132, but this being a bug fix, you'd put it under the ### Fixed section for the latest release.
Co-authored-by: award999 <[email protected]>
matthewbastien
left a comment
matthewbastien
left a comment
There was a problem hiding this comment.
Thank you for fixing this! I just have some minor comments to make the code easier to read.
src/debugger/logTracker.ts
Outdated
| const normalizePath = (p: string): string => { | ||
| try { | ||
| // If adapter sends a URI-like path, parse it | ||
| if (p.startsWith("file://") || p.includes("://")) { |
There was a problem hiding this comment.
suggestion: You can use vscode.URI.parse() with strict mode enabled here. That way we don't have to do any manual parsing of the string. Using parse() with strict mode set to true will throw an error when the string is not a valid URI. We can fall back on vscode.Uri.file() like you have below if an error is thrown.
|
|
||
| const bpLine0 = line - 1; // VS Code uses 0-based lines | ||
|
|
||
| const breakpoints = vscode.debug.breakpoints.filter( |
There was a problem hiding this comment.
question: Is there anything stopping us from using .filter(b => b instanceof vscode.SourceBreakpoint) here instead of type casting? Would make this code a lot easier to read.
There was a problem hiding this comment.
We avoid instanceof vscode.SourceBreakpoint here because it’s unreliable in VS Code tests. Breakpoints can be created in a different JS context, causing instanceof checks to fail even though the object behaves like a SourceBreakpoint. Checking for the presence of location avoids that cross-context issue and is more robust
src/debugger/logTracker.ts
Outdated
|
|
||
| for (const bp of breakpoints) { | ||
| const loc = bp.location; | ||
| if (!loc) { |
There was a problem hiding this comment.
suggestion: This check is unnecessary as it should be handled by the filter above.
There was a problem hiding this comment.
Good point, that check is redundant given the filter above. I’ve removed it.
| } | ||
|
|
||
| // Force a UI refresh so the breakpoint shows as installed. | ||
| vscode.debug.removeBreakpoints([bp]); |
There was a problem hiding this comment.
question: Will this also trigger when the breakpoint is already resolved? It's probably not a huge issue, but I think we should be only doing this when the breakpoint that's being hit isn't marked as resolved.
|
|
||
| ### Fixed | ||
|
|
||
| - Fix breakpoint filtering fallback in debugger logging ([#1989](https://github.com/swiftlang/vscode-swift/pull/1989)) |
There was a problem hiding this comment.
chore: You'll have to rebase this since the CHANGELOG was recently updated for version 2.14.2. This may require moving this bullet to a new ### Fixed section.
There was a problem hiding this comment.
Will do — I’ll rebase and move the entry to the current unreleased ### Fixed section.
Co-authored-by: Matthew Bastien <[email protected]>
Removed version 2.14.2 entry and its fixed items from the changelog.
Added a note about using the JSON schema for editing config.json and fixed breakpoint filtering in debugger logging.
3 participants
Description
LLDB-DAP sometimes fails to mark breakpoints as verified in the
setBreakpointsresponse, even though the breakpoint is hit during debugging. This leads to the VS Code UI showing the breakpoint as “unverified” while execution actually stops there.This PR adds a safe fallback that marks breakpoints as verified when we detect them being hit via:
stoppedevent withreason: "breakpoint", orbreakpointevent containing a valid source and line.This improves consistency between actual debugger behavior and the UI, especially for Swift executables and
swift test.Issue: #1871
Tasks
[ ] Documentation has been updated