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

Pass kind argument to add_hw_watchpoint #60

Closed
bet4it opened this issue Jun 11, 2021 · 2 comments
Closed

Pass kind argument to add_hw_watchpoint #60

bet4it opened this issue Jun 11, 2021 · 2 comments
Labels
API-breaking Breaking API change
Milestone

Comments

@bet4it
Copy link
Contributor

bet4it commented Jun 11, 2021

Currently, when we add watchpoint, kind is ignored:

let addr =
<T::Arch as Arch>::Usize::from_be_bytes(cmd.addr).ok_or(Error::TargetMismatch)?;
let kind =
<T::Arch as Arch>::BreakpointKind::from_usize(cmd.kind).ok_or(Error::TargetMismatch)?;
let handler_status = match cmd_kind {
CmdKind::Add => {
use crate::target::ext::breakpoints::WatchKind::*;
let supported = match cmd.type_ {
0 => (ops.sw_breakpoint()).map(|op| op.add_sw_breakpoint(addr, kind)),
1 => (ops.hw_breakpoint()).map(|op| op.add_hw_breakpoint(addr, kind)),
2 => (ops.hw_watchpoint()).map(|op| op.add_hw_watchpoint(addr, Write)),
3 => (ops.hw_watchpoint()).map(|op| op.add_hw_watchpoint(addr, Read)),
4 => (ops.hw_watchpoint()).map(|op| op.add_hw_watchpoint(addr, ReadWrite)),

But according to gdb rsp doc, kind when set watchpoint means number of bytes to watch, which is not meaningless:

‘Z2,addr,kind’
    Insert (‘Z2’) or remove (‘z2’) a write watchpoint at addr. The number of bytes to watch is specified by kind. 

We need also pass kind as an argument to add_hw_watchpoint.

@daniel5151
Copy link
Owner

Well, this is embarrassing 😳
I'm surprised that I didn't catch this when I plumbed-through BreakpointKind support to software/hardware watchpoints... Oops.

In any case, thanks for reporting this discrepancy!
I'll land a fix in the dev/0.6 branch that updates the add_hw_watchpoint function signature appropriately (hopefully sometime this weekend).

Since this will require an API breaking change, it will also require a breaking gdbstub release. That's not too bad, as there've been enough breaking changes trickling into the dev/0.6 branch that I'll probably end up publishing 0.6 sometime relatively soon-ish. I don't have an exact timeline yet, but I'd wager 0.6 will be up on crates.io sometime in the next few weeks.

@daniel5151 daniel5151 self-assigned this Jun 11, 2021
@daniel5151 daniel5151 added API-breaking Breaking API change bug Something isn't working labels Jun 11, 2021
@daniel5151 daniel5151 added this to the 0.6 milestone Jun 11, 2021
daniel5151 added a commit that referenced this issue Jun 14, 2021
closes #60

also took this chance to rewrite / optimize some of the code in the
breakpoint handler. notably, I added match arm guards to help with DCE.
@daniel5151
Copy link
Owner

daniel5151 commented Jun 14, 2021

I've pushed a fix to the dev/0.6 branch.

@daniel5151 daniel5151 removed the bug Something isn't working label Jun 14, 2021
@daniel5151 daniel5151 removed their assignment Jun 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API-breaking Breaking API change
Projects
None yet
Development

No branches or pull requests

2 participants