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

[debug] Jump to Cursor feedback #75578

Closed
vadimcn opened this issue Jun 15, 2019 · 8 comments
Closed

[debug] Jump to Cursor feedback #75578

vadimcn opened this issue Jun 15, 2019 · 8 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug debug Debug viewlet, configurations, breakpoints, adapter issues verified Verification succeeded
Milestone

Comments

@vadimcn
Copy link
Contributor

vadimcn commented Jun 15, 2019

After having tried to implement the new "Jump to Cursor" functionality, I'd like to share some feedback:

  • VSCode does not surface errors returned from GotoRequest. Because GotoTargetsRequest does not specify which thread will be jumped, it is not possible to validate locations at that stage. (For example, language runtime may not allow moving execution point outside of the current function).
  • The lifetime of GotoTarget not not clearly spelled out in the DAP spec. Does adapter need to remember ids of all GotoTargets it had ever returned?
  • It would be helpful if GotoTarget contained adapter-defined data, that were round-tripped via subsequent GotoRequest. This way adapter wouldn't need to remember GotoTarget<->id association at all.
@weinand weinand added the debug Debug viewlet, configurations, breakpoints, adapter issues label Jun 15, 2019
@weinand weinand added this to the June 2019 milestone Jun 15, 2019
@gregg-miskelly
Copy link
Member

FYI the first issue is also a problem for the C# DA.

Also, I think the answer for the second question is that target ids should be valid until the next continue. Though specifying this in the spec sounds reasonable to me.

@isidorn
Copy link
Contributor

isidorn commented Jun 18, 2019

@vadimcn @gregg-miskelly thanks for feedback.
I have pushed a commit which tackles the first issue. Please try it out and let me know how it behaves for you.

@weinand leaving up the other two issues for you.

@isidorn isidorn removed their assignment Jun 18, 2019
@weinand
Copy link
Contributor

weinand commented Jun 24, 2019

The general rule for ID lifetime in DAP is:

  • IDs of "ephemeral" objects are valid while in the stopped state (or as Gregg said: "until the next 'continue'"). Examples are variable references, goto targets, step-in target, frames.
  • IDs of long living objects are valid at least for the lifetime of the debug session (e.g. processes, threads), or even longer (breakpoints, data breakpoints).

I have created an issue in the DAP-repo for the clarification request: microsoft/debug-adapter-protocol#60.

Round-tripping adapter-defined data is not an option any more because the DAP already uses IDs to allow for communicating arbitrary data between "GotoTarget" and "Goto" requests. In addition it is cheaper to round-trip only an ID instead of the whole data.

@weinand
Copy link
Contributor

weinand commented Jun 24, 2019

Assigning to @isidor because he provided the only real code fix for the three issues.

@weinand weinand closed this as completed Jun 24, 2019
@weinand weinand assigned isidorn and unassigned weinand Jun 24, 2019
@weinand weinand added the bug Issue identified by VS Code Team member as probable bug label Jun 24, 2019
@isidorn
Copy link
Contributor

isidorn commented Jun 24, 2019

@vadimcn @gregg-miskelly can you please verify the 1) is fixed for you so I add a verified label. Thanks

@gregg-miskelly
Copy link
Member

LGTM

@vadimcn
Copy link
Contributor Author

vadimcn commented Jun 24, 2019

Yeah, #1 looks fixed.

@isidorn isidorn added the verified Verification succeeded label Jun 25, 2019
@isidorn
Copy link
Contributor

isidorn commented Jun 25, 2019

Thanks. Adding verfieid label.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug debug Debug viewlet, configurations, breakpoints, adapter issues verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

4 participants