search

eclipse-cdt-cloud / cdt-gdb-vscode

CDT GDB Visual Studio Code Extension
Eclipse Public License 2.0
21 stars 24 forks source link

Support disable/enable hardware breakpoint #89

Closed trongle0504 closed 1 year ago

trongle0504 commented 1 year ago

Currently, enabling hardwareBreakpoint in launch.json argument allows the debugger to set hardware break-point or software break-point only ("hardwareBreakpoint": true). We need to support enabling/disabling setting SW and HW break-point simultaneously while debugging.

eclipse-cdt-bot commented 1 year ago

Can one of the admins verify this patch?

trongle0504 commented 1 year ago

Hi @jonahgraham -san,

Could you please help me review the code? Please see the document attached to ticket IDE-63527 for details.

I also commit code in repository cdt-gdb-adapter. https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/pull/264

Thank you!

trongle0504 commented 1 year ago

As mentioned in eclipse-cdt-cloud/cdt-gdb-adapter#264 using a toggle for the new command is problematic as the initial state of hw breakpoint as set in launch.json is not taken into account here and leaves the UI commands in an inconsistent state.

I have serious concerns about the overall usability of this design and I hope that once you share the information in the below mentioned document I can understand the workflows.

For example, are you supposed to be able to change hw -> sw (or back) in the middle of a debug session?

Please see the document attached to ticket IDE-63527 for details.

Please provide required information in the open - this is an open source project so should not require access to private information to understand this.

Hi @jonahgraham ,

I have some changes in the purpose of this PR compared to the original one.

Below is the description of this PR:

The purpose of this PR is to support toggle breakpoint being able to change the Hardware breakpoint -> Software breakpoint (or back) in the middle of a debug session.

Case 1: If hardwareBreakpoint is not configured in launch.json, the default setting will be SW breakpoint.

  1. After launching the debugger, the user right-clicks on the lineNumber and selects Toggle Breakpoint. Screenshot_24

  2. The Debug Console will display a message indicating that HW breakpoint is now in use. Screenshot_25

  3. The user can continue to use Toggle Breakpoint to switch between HW and SW breakpoints.

Case 2: If "hardwareBreakpoint": true is configured in launch.json, the default setting will be HW breakpoint.

  1. After launching the debugger, the user right-clicks on the lineNumber and selects Toggle Breakpoint. Screenshot_24

  2. The Debug Console will display a message indicating that the SW breakpoint is now in use. Screenshot_27

  3. The user can continue to use Toggle Breakpoint to switch between HW and SW breakpoints.

Could you please help me review it again?

Thank you!

trongle0504 commented 1 year ago

Messages in the Debug Console are somewhat useful, but they can also be lost among lots of other output from the backend and it isn't visible if the Debug Console is not open.

VS Code has standard ways of notifying users. Please look at showInformationMessage and review UX guidelines on notifications.

I replaced the message at the Debug console to the one using VSCode's Notifications. Screenshot_37 Screenshot_39

trongle0504 commented 1 year ago

However in this case there is another problem. The current implementation provides no way to show user the current setting, leading to the user having to toggle the setting twice and looking at Debug Console to know the current setting.

Perhaps using the status bar to show current state is useful? Or a checkmark next to a menu item?

I used the status bar to show the current state of the HW breakpoint. Currently, the status is only visible when the user starts debugging, and when debugging is finished it will automatically hide.

The state also updates properly when the user configures "hardwareBreakpoint": true, "hardwareBreakpoint": false or even unconfigured "hardwareBreakpoint" at the launch.json file.

trongle0504 commented 1 year ago

As you have implemented it now the toggle will apply to future breakpoints that are inserted by GDB - that intention is not obvious to the user. Was there an intention to change existing breakpoints from hw -> sw (or vice versa)?

With current update user can change from HW breakpoint -> SW breakpoint and switch from SW breakpoint -> HW breakpoint

trongle0504 commented 1 year ago

Hi @jonahgraham , I've updated some of the changes you suggested including:

Could you please help me review it again?

Thank you!

trongle0504 commented 1 year ago

The final problem I have with this solution is that there is no way to show users for already inserted breakpoints whether it is a hardware or software breakpoint - other than user typing >info break or similar in the CLI.

Regarding this issue, I have investigated but I have no solution for this. Any ideas for this issue? or we will improve it in the future.

trongle0504 commented 1 year ago

I'm sorry, I would like to close this request because there are some complex cases to cover, and it may cause inconvenience for users. Thank you for taking the time to review the code.