Skip to content

Conversation

@ConradIrwin
Copy link
Contributor

While working on debugger support for Zed, I noticed that
delve returns internal breakpoints in the "hit breakpoints" array when handling
a panic.

Although there's nothing really wrong with doing this, it caused an edge-case
in our parsing code (as in practice everyone uses unsigned integers for
breakpoint ids), and there's no particular value in doing so (because the
client has no record of the internal breakpoints).

ConradIrwin added a commit to zed-industries/dap-types that referenced this pull request Jun 11, 2025
These are returned by delve's internal breakpoints (for panic handling)

c.f. go-delve/delve#4027
@aarzilli
Copy link
Member

aarzilli commented Jun 11, 2025

Those aren't internal breakpoints, they just exist by default. You could delete them if you wanted (which is something people want to do #3653).

Copy link
Member

@aarzilli aarzilli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, although maybe one day we should emit breakpoint events to make clients aware of these instead.

@ConradIrwin
Copy link
Contributor Author

@aarzilli makes sense. Is there a way as a client to know that these exist, or as a delve consumer should I just expect them to be there and expose UI to control them?

I didn't see an immediately obvious "list all breakpoints" command in the DAP, so sorry if this is just a lack of my understanding.

@aarzilli
Copy link
Member

We'd have to emit breakpoint events ourselves and clients would have to handle them correctly (if we start doing this it would have to check that at least vscode supports it correctly).

There is no "list all breakpoints" API, you are not missing anything (or we are both missing it).

Copy link
Member

@derekparker derekparker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@derekparker derekparker merged commit 2ac3573 into go-delve:master Jun 12, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants