Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

service/dap: support setting breakpoints while running #2472

Merged
merged 7 commits into from
May 17, 2021

Conversation

polinasok
Copy link
Collaborator

@polinasok polinasok commented May 10, 2021

Unlike vscode-go, this implementation does not resume execution after halting and setting breakpoints. Resuming automatically turns out to be much trickier than just skipping a stopped event and continuing. It is possible that the stop occurred due to reasons other than our manual stop (breakpoint, call return), so we would need to detect thtat and report any stops that the user might be interested in. In addition, next would be cancelled by the halt command, so to resume it, we would need to implement an alternative version of halt that doesn't cancel next. So for now, we will just be reporting the pause to the user and let them decide when and how to resume execution manually.

Updates #1515
Updates golang/vscode-go#1475

@polinasok
Copy link
Collaborator Author

@hyangah @suzmue

service/dap/server.go Outdated Show resolved Hide resolved
service/dap/server_test.go Outdated Show resolved Hide resolved
service/dap/server_test.go Outdated Show resolved Hide resolved
service/dap/server_test.go Outdated Show resolved Hide resolved
service/dap/server_test.go Outdated Show resolved Hide resolved
service/dap/server_test.go Outdated Show resolved Hide resolved
service/dap/server.go Outdated Show resolved Hide resolved
service/dap/server.go Show resolved Hide resolved
service/dap/server.go Outdated Show resolved Hide resolved
@polinasok
Copy link
Collaborator Author

polinasok commented May 12, 2021

Failing test is TestAttachStopOnEntry, which I am hopefully fixing in the other PR #2476

if inProgress == api.Continue {
s.overrideStopReason <- skipStop
} else {
// This would be one of the step commands, which get cancelled by debugger.
Copy link
Member

Choose a reason for hiding this comment

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

There is no necessity for this logic. All step commands can be resumed, like continue and rewind by issuing a api.DirectionCongruentContinue command.

Copy link
Contributor

Choose a reason for hiding this comment

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

From my reading of the code in proc, when a step /next request is manually halted ClearInternalBreakpoints is always called.

It would be great if there is a way to be able to continue next requests, this is also a feature I am interested in for implementing logpoints.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We had this conversation a while back when trying to fix setting breakpoints in the old adapter:
golang/vscode-go#787 (comment)
I too saw ClearInternalBreakpoints and understood it as halt cancelling step.

Copy link
Member

Choose a reason for hiding this comment

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

What's a logpoint? Anyway it shouldn't be to hard to make a variant of halt that sets a flag so that ClearInternalBreakpoints isn't called in proc.(*Target).Continue.

Copy link
Contributor

Choose a reason for hiding this comment

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

A log point is like a trace point but with user supplied message

From DAP spec for setBreakpoints arguments:

 /**
   * If this attribute exists and is non-empty, the backend must not 'break'
   * (stop)
   * but log the message instead. Expressions within {} are interpolated.
   * The attribute is only honored by a debug adapter if the capability
   * 'supportsLogPoints' is true.
   */
  logMessage?: string;

service/dap/server.go Outdated Show resolved Hide resolved
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

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.

5 participants