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

Breakpoint support on Linux #108

Merged
merged 51 commits into from Sep 30, 2020
Merged

Breakpoint support on Linux #108

merged 51 commits into from Sep 30, 2020

Conversation

Arthamys
Copy link
Contributor

@Arthamys Arthamys commented Sep 6, 2020

Here is a first version that supports setting and removing breakpoints, as well as a single_step function.
I wanted to get your feedback on the interface, as well as some inputs on the edge cases that should be handled.

Bjorn mentioned in #55 that code should handle "faulting instructions at the place of the breakpoint" but I don't know what should be the particular reaction we should have in this situation. Should we not allow settings the breakopint?

@bjorn3
Copy link
Contributor

bjorn3 commented Sep 6, 2020

Bjorn mentioned in #55 that code should handle "faulting instructions at the place of the breakpoint" but I don't know what should be the particular reaction we should have in this situation. Should we not allow settings the breakopint?

I was referring to the situation where you hit a breakpoint and then when continuing the first instruction causes a fault. You need to make sure that you don't immediately set the breakpoint again such that continuing would run the instruction again instead of hitting the breakpoint, but you must also ensure that when the instruction does succeed this time the breakpoint is set again.

src/target/linux.rs Show resolved Hide resolved
src/target/linux.rs Outdated Show resolved Hide resolved
src/target/linux.rs Outdated Show resolved Hide resolved
src/target/linux.rs Outdated Show resolved Hide resolved
tests/runtime_breakpoint.rs Outdated Show resolved Hide resolved
tests/runtime_breakpoint.rs Outdated Show resolved Hide resolved
tests/test_utils.rs Outdated Show resolved Hide resolved
@bjorn3 bjorn3 requested a review from nbaksalyar Sep 14, 2020
Copy link
Contributor

@blitzerr blitzerr left a comment

You tackled an involved issue (by my standards) as your first PR. kudos to you @Arthamys

examples/repl.rs Outdated Show resolved Hide resolved
examples/repl.rs Outdated Show resolved Hide resolved
src/target/linux.rs Outdated Show resolved Hide resolved
src/target/linux.rs Outdated Show resolved Hide resolved
tests/runtime_breakpoint.rs Outdated Show resolved Hide resolved
tests/runtime_breakpoint.rs Outdated Show resolved Hide resolved
src/target/linux/software_breakpoint.rs Outdated Show resolved Hide resolved
src/target/linux.rs Outdated Show resolved Hide resolved
@bjorn3
Copy link
Contributor

bjorn3 commented Sep 30, 2020

This hit the spurious #116. The good news is that all other tests pass!

examples/repl.rs Outdated Show resolved Hide resolved
examples/repl.rs Outdated Show resolved Hide resolved
tests/runtime_breakpoint.rs Show resolved Hide resolved
tests/testees/loop.rs Outdated Show resolved Hide resolved
tests/runtime_breakpoint.rs Outdated Show resolved Hide resolved
src/target/linux/software_breakpoint.rs Outdated Show resolved Hide resolved
Copy link
Member

@nbaksalyar nbaksalyar left a comment

Great work @Arthamys, thank you!

I've added a couple of general comments about future work, and there is a couple of minor typos.
Would you prefer to address them in this PR, or raise a separate one?

self.breakpoints.borrow_mut().insert(addr, bp.clone());
bp
}
// If there is already a breakpoint set, we give back the existing one
Copy link
Member

@nbaksalyar nbaksalyar Sep 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, we might need to support multiple breakpoints set in one place. For example, for breakpoints with different conditions (watching variables), and for the case of 'internal' breakpoints (i.e. the ones set by the debugger itself). I believe that's a separate issue, though, and should be tackled with a separate PR.

Copy link
Contributor

@bjorn3 bjorn3 Sep 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Watching variables could be handled by a wrapper around LinuxTarget. The code should be relatively platform independent. Setting multiple breakpoints could also be handled by such wrapper.

Copy link
Contributor Author

@Arthamys Arthamys Sep 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having more involved types of breakpoints is of course something that we should tackle, but as you suggested, it should probably be part of another PR once we have breakpoints support on another platform than Linux.

src/target/linux/software_breakpoint.rs Outdated Show resolved Hide resolved
src/target/linux/software_breakpoint.rs Outdated Show resolved Hide resolved
src/target/linux/software_breakpoint.rs Outdated Show resolved Hide resolved
bjorn3
bjorn3 approved these changes Sep 30, 2020
Copy link
Member

@nbaksalyar nbaksalyar left a comment

Thank you!

@bjorn3 bjorn3 merged commit e4eb3f3 into headcrab-rs:master Sep 30, 2020
@bjorn3
Copy link
Contributor

bjorn3 commented Sep 30, 2020

Thanks a lot! Do you want to make a follow up to remove the patch breakpoint function hack from the repl and tests?

@Arthamys
Copy link
Contributor Author

Arthamys commented Sep 30, 2020

Yes, I'll get on that tomorrow!

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

Successfully merging this pull request may close these issues.

None yet

4 participants