[Dexter] Add DAP instruction and function breakpoint handling #152718
Conversation
|
✅ With the latest revision this PR passed the Python code formatter. |
73804d1 to
2bb714e
Compare
f537f45 to
7089cfd
Compare
| self.dap_id_to_dex_ids[dap_bp_id] = [dex_bp_id] | ||
| visited_dap_ids.add(dap_bp_id) | ||
| self.pending_breakpoints = False | ||
| # Is this right? Are we guarenteed the order of the outgoing/incoming lists? |
There was a problem hiding this comment.
Technically the specification only explicitly says "The breakpoints returned are in the same order as the elements of the breakpoints (or the deprecated lines) array in the arguments." for source breakpoints*. However, I'd be very surprised if any debugger chose to do it differently, given that the response is not guaranteed to contain sufficient information to otherwise map the requested breakpoints to the received breakpoints - it would effectively be a useless feature otherwise.
*Function and instruction breakpoints have the wording "The [response] array elements correspond to the elements of the breakpoints array.", which I take to mean that the ordering should be the same, but it's not as explicit as the source breakpoints case.
There was a problem hiding this comment.
Ah cool, thanks for the info. I've updated the comments to be more certain
|
This LGTM overall. I'll let @SLTozer do the final approval |
| @@ -606,7 +693,13 @@ def _get_launch_params(self, cmdline): | |||
| """ "Set the debugger-specific params used in a launch request.""" | |||
|
|
|||
| def launch(self, cmdline): | |||
| assert len(self.file_to_bp.keys()) > 0 | |||
| # FIXME: This should probably not a warning, not an assert. | |||
There was a problem hiding this comment.
At the moment, the reason it's an assert is that IIRC Dexter should always have either added a breakpoint or exited early at this point - so at the very least it would be a raise, rather than a warning.
There was a problem hiding this comment.
You may have instructed dexter to set a breakpoint, but the debugger fail to bind any. In that case, dexter asserting isn't very user friendly... in any case, I've reworded the fixme (I still believe it should be fixed at some point).
No description provided.