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

Basic Data breakpoints #765

Merged
merged 14 commits into from
Dec 12, 2024
Merged

Basic Data breakpoints #765

merged 14 commits into from
Dec 12, 2024

Conversation

puremourning
Copy link
Owner

@puremourning puremourning commented Apr 30, 2023

  • ergonomics aren't great. Right now you hit F9 in the watch window to set a data breakpoint. This makes a kind of game-logic sense, but isn't intuitive.
  • Breakpoints window doesn't work at all (maybe promote behaviours to actual BP object finally?)
  • signs in the watch window when added that way? (can we track this in practice.. maybe put it on the expandable)
  • No persistence.
  • :VimspectorAddWatchpoint <....>
  • docs
  • tests

Works ok with lldb-dap and codelldb with this patch: vadimcn/codelldb#1161

basic usage: https://asciinema.org/a/f47vEkVI9FKisLPiAyfcC2LGW

not perfect.

Closes #524

@puremourning
Copy link
Owner Author

This change is Reviewable

@puremourning puremourning force-pushed the data-breakpoints branch 3 times, most recently from 48a5679 to 51d0bad Compare May 2, 2023 12:32
Copy link
Owner Author

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

Need also to

  • update the docs
  • write some tests
  • Check the breakpoints window, edit etc.

continue
if bp[ 'conn' ] != connection.GetSessionId():
continue
if not bp[ 'info' ].get( 'dataId' ):
Copy link
Owner Author

Choose a reason for hiding this comment

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

could 0 be a valid value?

@@ -91,6 +91,7 @@
'delete': [ '<Del>' ],
'set_value': [ '<C-CR>', '<leader><CR>' ],
'read_memory': [ '<leader>m' ],
'add_data_breakpoint': [ '<F9>' ],
Copy link
Owner Author

Choose a reason for hiding this comment

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

this should probably be based on the mappings mode, but I'm not sure anyone uses anything other than HUMAN mode...

variable: Expandable
view: View

variable, view = self._GetVariable( buf, line_num )
Copy link
Owner Author

Choose a reason for hiding this comment

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

I guess this is where we might decide that it's actually an expression data BP e.g. we add a watch like foo and we want a data bp on the foo variable, but we currently can only do it on the result of that expression.

We should also allow it from the command line like :VimspectorAddDataBreakpoint <expression>

I will probably need to implement that for CodeLLDB before it's worth adding to vimspector as I still haven't found an adapter that supports it.

Copy link
Owner Author

Choose a reason for hiding this comment

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

done for codelldb; turns out that lldb-dap (formerly lldb-vscode) supports partially (though it's buggy and doesn't implement the address/bytes extension)

Works with CodeLLDB mostly for named things. Expressions don't seem to
be supported by anyone.

Fixed some state issues with the connections being retained across
restarts in watches. ugh. so messy.

Added access type question.

Java debug adapter returns broken breakpoints info response
puremourning and others added 8 commits October 21, 2024 19:21
Include the message provided by the server. This is useful in the case
that watchpoint can't be set. Also track server status (VERIFIED, etc.)
and print that.
Store the connection ID and data ID in the "qf" hack, then handle these
commands trivially.

Fix a bug where we never cleared out data breakpoints if there were
none.
@puremourning puremourning changed the title WIP: Data breakpoints Basic Data breakpoints Dec 12, 2024
Copy link
Owner Author

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 7 of 14 files at r1, 4 of 8 files at r2, 8 of 8 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions

Also fix yet-another bug with empty data breakpoints lists not clearing
previously set data breakpoints.
Copy link
Owner Author

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 8 of 8 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions

@mergify mergify bot merged commit 1c24db4 into master Dec 12, 2024
9 checks passed
@mergify mergify bot deleted the data-breakpoints branch December 12, 2024 15:37
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.

[Feature Request]: Data breakpoints
1 participant