[go: up one dir, main page]
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.

Sign up for GitHub

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

Jump to bottom

WIP - Rewrite of parts of the breakpoint and stepping implementation. #2288

Draft
wants to merge 42 commits into
base: master
Choose a base branch
from
Draft

WIP - Rewrite of parts of the breakpoint and stepping implementation. #2288

wants to merge 42 commits into from

Conversation

noppej
Copy link
Contributor
@noppej noppej commented Mar 12, 2024

This is a fairly large PR, because some 'missing' / 'unreliable' functionalities required new infrastructure before they could be reimplemented, and required some 'experimentation', so I was not able to break it into smaller PR's.

In summary, the limitations I focussed on were as follows:

  • The existing implementation for setting breakpoints at a specific address worked as expected.
  • The existing implementation for setting breakpoints at a specific file/line/column source location (mapping to a suitable instruction address) was limited to working for source code lines that were explicitly mentioned in the DWARF line program sequence rows, and would not work for cases such as described in Disabled breakpoints #2180. This had implications for stepping also.
  • The existing stepping code had huge 'blindspots', as a result of limited DWARF information in the line program sequence rows, and no call site information for non-inlined functions.
  • The stepping would often just give errors ("can't do that"), or sometimes, and arguably worse, would try to step to unreachable code.

The approach to addressing these, was as follows:

  • Centralize all debug halting code (breakpoints and stepping) to a new module called debug::halting.
  • Implement the concept of Block's, as an intermediate between Sequence's and Instruction's. These blocks are a heuristic based approximation of the basic block as described in the DWARF spec (Section 6.2.1). There are some limitations where the implementation errs on the side of caution (e.g. unless we disassemble per platform, we don't know which instructions are branching, so don't assume sequential instructions are in the same block).
  • Adapt the current breakpoint setting implementation to use the new Sequence->Block->Instruction structures.
  • Ditto for the current stepping implementation. This allowed the deletion of a lot of unreliable logic.

Status after the rewrite:

  • There are opportunities to improve further, but these require additional infrastructure, like the support of software breakpoints. I've marked some of these with TODO comments in the code.
  • All the current breakpoint tests pass without modification.
  • TODO: We need new tests for breakpoints at previously "can't do that" locations, (e.g. Disabled breakpoints #2180).
  • TODO: We need tests for the stepping code. Currently I have done manual testing only. Tests for stepping will have some limitations, because we can't 'single step' the target processor in a test, but we should be able to get good test coverage of the 'target location' calculations required during stepping.

List of tests required for stepping code:
... wip ...

noppej added 15 commits March 5, 2024 16:54
@noppej
Fixes to FunctionDie
e2d5b36
@noppej
Introduce source_instructions::Block as an intermediate container b…
5240b5f 
…etween `Sequence` and `Instruction`.
@noppej
wip
10b9c8e
@noppej
Refactor source_instructions into a multi file mod instructions
aa67b7a
@noppej
Working instruction blocks, before adding range boundaries
e5de482
@noppej
Merge branch 'fix_function_ranges' into breakpoint_and_stepping
72a0171
@noppej
wip
931c2c1
@noppej
wip2
ca3ca9d
@noppej
wip 3 - recurse to iterate
93a9fdd
@noppej
Merge branch 'master' into breakpoint_and_stepping
8e77480
@noppej
Separate sequence of program line instructions into Blocks
4022531
@noppej
Search for breakpoint addresses across block boundaries.
6260efe
@noppej
Improvements to breakpoint matching algos.
f22ffff
@noppej
Rename instructions module to halting and create `halting::steppi…
93b85e6 
…ng.rs`

Move canonical_unit_path_eq out of debug_info module.
@noppej
Adapt Stepping to use new breakpoint/sequence/block/instruction imp…
f9de24a 
…lementation.StepOver works, with outstanding TODO's
@noppej noppej added the skip-changelog This PR does not require a changelog entry on release label Mar 12, 2024
@noppej
Copy link
Contributor Author
noppej commented Mar 12, 2024

@SergioGasquez If you have the time/inclination to do so, I'd appreciate if you can do some testing with this branch. Hint: You can install it with cargo install probe-rs --features cli --git https://github.com/probe-rs/probe-rs --branch breakpoint_and_stepping --force

I am confident that it fixes your #2180, even though there are still a few stepping scenarios I know need work.
Feel free to document any issues you run into :)

@SergioGasquez
Copy link
Contributor

@SergioGasquez If you have the time/inclination to do so, I'd appreciate if you can do some testing with this branch. Hint: You can install it with cargo install probe-rs --features cli --git https://github.com/probe-rs/probe-rs --branch breakpoint_and_stepping --force

I am confident that it fixes your #2180, even though there are still a few stepping scenarios I know need work. Feel free to document any issues you run into :)

Just tried your branch, and I was able to step into the method that was previously ignored! Thanks for working on this fix!

@noppej
Narrow scope of sequence searches to improve step performance
46239ad
@noppej
Add support for DW_AT_specification to FunctionDie and its attributes.
29a672c
@noppej
Resolve inline functions where the abstract origin is coded as a `gim…
5cddfbf 
…li::DebugInfoRef`
@noppej
Merge branch 'master' into add_function_specification_die
0c69672
@noppej
Responses to PR review
1037ef2
@noppej
Replace manual arithmetic in DebugInfo::get_die_at_offset()
f20edde
@noppej
Merge branch 'master' into breakpoint_and_stepping
487667b
@noppej
Merge branch 'add_function_specification_die' into breakpoint_and_ste…
63018f8 
…pping
@noppej
Revert "Merge branch 'add_function_specification_die' into breakpoint…
78e5048 
…_and_stepping"

This reverts commit 63018f8, reversing
changes made to 487667b.
@noppej
Perf: Step Over uses next linked block if it exists.
e199ccc
@noppej
Merge branch 'master' into breakpoint_and_stepping
1befc37
@noppej noppej mentioned this pull request Mar 28, 2024
Merged
@bugadani
Copy link
Contributor

@noppej just FYI this PR has merge conflicts now that should be resolved. Other than that, are the TODOs in the PR description still unresolved?

@noppej
Copy link
Contributor Author
noppej commented Apr 16, 2024

@bugadani Thanks. I'm still actively working on #1935, which will contribute to being able to finalize this PR.

@noppej noppej mentioned this pull request Apr 24, 2024
Merged
12 tasks
@Erhannis
Copy link

Is this branch functional? I just installed the plugin, and I dunno if I'm just encountering a particular set of edge cases, but it's nearly unusable. Only like 1 in 10 lines accept a breakpoint, and stepping forward (or even stepping out) almost always lands me e.g. at some __release call 6 levels into system code. If this branch works well enough, I'm inclined to compile it and use it instead of the main branch.

@Erhannis
Copy link
Erhannis commented May 21, 2024 •
edited
Loading

Oh, phew. I installed this branch, and immediately things work properly. (So far as my 5 seconds of testing indicate.)
Edit: Or...mostly. I got a weird result after one of several steps, but MOSTLY things are working. If I discover anything that seems worthy of reporting I'll report.

@noppej
Copy link
Contributor Author
noppej commented May 21, 2024

@Erhannis This is a bit of an 'experimental' branch. Doing breakpoints and stepping is rather tricky with the available debuginfo, and I really appreciate your positive feedback. I'm in the middle of finalizing another PR, and hopefully in the next day or so I will bring this branch up to date with the latest improvements on 'master'. That should give you something worthwhile to work with.

Any specific use cases where things are still wonky will be much appreciated :)

@Erhannis
Copy link

@noppej Nothing too specific that I've noticed - some lines don't accept breakpoints where I might expect them to, and a time or two stepping has landed me in a weird place, but usually I can work around it without too much trouble. While I'm not sure the following is relevant to this branch, in the "variables" list I've been getting some "unsupported memory implementation" messages, as well as "Location Value not supported for referenced variables" which at least has pretty clear intent, and also, with a samples: &mut [u8], I see samples > data_ptr > *data_ptr > Error: This is a bug! Attempted to evaluate a Variable with no type or no memory location". Oh - looks like I also get that when expanding a fixed array, like for both values of a [u8; 2]`.

@noppej
Copy link
Contributor Author
noppej commented May 22, 2024 •
edited
Loading

in the "variables" list I've been getting some "unsupported memory

@Erhannis ... I can't reproduce this. I get
Screenshot 2024-05-22 at 4 42 02 PM

Are you able to log a reproduce-able test case as a new issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Vollbrecht Vollbrecht Vollbrecht left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
skip-changelog This PR does not require a changelog entry on release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@noppej @SergioGasquez @bugadani @Erhannis @Vollbrecht