riscv-openocd icon indicating copy to clipboard operation
riscv-openocd copied to clipboard

There are cases when WPs disabled by `disable_triggers()` can not be re-enabled by `enable_triggers()`.

Open en-sc opened this issue 1 year ago • 2 comments

Currently, disable_triggers() just goes through all the WPs and disables them one-by-one (the case when r->manual_hwbp_set is false): https://github.com/riscv-collab/riscv-openocd/blob/5afed58fcdb67ff87842dad9c3ab7f73ca0c8cd8/src/target/riscv/riscv.c#L2487-L2499 enable_triggers() works accordingly: https://github.com/riscv-collab/riscv-openocd/blob/5afed58fcdb67ff87842dad9c3ab7f73ca0c8cd8/src/target/riscv/riscv.c#L2526-L2536

Now consider the following situation:

  • A target has 2 triggers. The 1-st trigger supports configurations required by WP 1, WP 2, WP 3. The 2-nd trigger only supports configuration required by WP-2.
  • The users sets WP 1. WP 1 occupies the 1-st trigger.
  • The user sets WP 2. WP 2 occupies the 2-nd trigger,
  • The user removes WP 1, the 1-st trigger is free.
  • The user sets WP 3. WP 3 occupies the 1-st trigger.
  • The situation is as follows:
    • taget->watchpoints points to WP 2.
    • taget->watchpoints->next points to WP 3.
  • disable_triggers() is called (e.g. during step).
  • enable_triggers() will try to re-enable the disabled WP, but will fail, since WP 2 will occupy 1-st trigger and 2-nd triggerdoes not support WP 3.

en-sc avatar Aug 05 '24 12:08 en-sc

Moreover, seems like the state array is redundant here -- all watchpoints are enabled/disabled at once.

en-sc avatar Aug 07 '24 17:08 en-sc

It sounds like a reasonable fix would be to:

  • Keep track of watchpoint --> trigger slot mapping in target->watchpoints or similar data structure.
    • This is already in trigger_unique_id in struct riscv_info.
  • Only (temporarily) disable the watchpoints and do not remove them from the target->watchpoints structure in the process.
  • When re-enabling watchpoints, ensure they are placed back into the exact "trigger slots" as originally.

JanMatCodasip avatar Aug 14 '24 13:08 JanMatCodasip