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

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

Open
en-sc opened this issue Aug 5, 2024 · 2 comments

Comments

@en-sc
Copy link
Collaborator

en-sc commented Aug 5, 2024

Currently, disable_triggers() just goes through all the WPs and disables them one-by-one (the case when r->manual_hwbp_set is false):

/* Just go through the triggers we manage. */
struct watchpoint *watchpoint = target->watchpoints;
int i = 0;
while (watchpoint) {
LOG_TARGET_DEBUG(target, "Watchpoint %d: set=%d", i, watchpoint->is_set);
state[i] = watchpoint->is_set;
if (watchpoint->is_set) {
if (riscv_remove_watchpoint(target, watchpoint) != ERROR_OK)
return ERROR_FAIL;
}
watchpoint = watchpoint->next;
i++;
}

enable_triggers() works accordingly:
struct watchpoint *watchpoint = target->watchpoints;
int i = 0;
while (watchpoint) {
LOG_TARGET_DEBUG(target, "Watchpoint %d: cleared=%" PRId64, i, state[i]);
if (state[i]) {
if (riscv_add_watchpoint(target, watchpoint) != ERROR_OK)
return ERROR_FAIL;
}
watchpoint = watchpoint->next;
i++;
}

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
Copy link
Collaborator Author

en-sc commented Aug 7, 2024

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

@JanMatCodasip
Copy link
Collaborator

JanMatCodasip commented Aug 14, 2024

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.

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

No branches or pull requests

2 participants