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

read_addrs() can return fewer bytes than requested #115

Merged
merged 1 commit into from Dec 7, 2022
Merged

read_addrs() can return fewer bytes than requested #115

merged 1 commit into from Dec 7, 2022

Conversation

geigerzaehler
Copy link
Contributor

@geigerzaehler geigerzaehler commented Oct 27, 2022 •
edited

Description

The read_addrs() method for targets now returns a number of bytes that were read. This allows a target to gracefully signal the client that memory at an address range is not accessible.

I’ve implemented this functionality for the armv4t example to be able to test it.

Closes #111.

API Stability

This PR requires a breaking API change.

Checklist

  • Implementation
    • cargo build compiles without errors or warnings
    • cargo clippy runs without errors or warnings
    • cargo fmt was run
    • All tests pass
  • Documentation
    • rustdoc + approprate inline code comments
    • Updated CHANGELOG.md
    • (if appropriate) Added feature to "Debugging Features" in README.md
  • If implementing a new protocol extension IDET
  • If upstreaming an Arch implementation

I’m not sure how to update the changelog.

Validation

Read before accessible area

(gdb) x 0x00
0x0:    Cannot access memory at address 0x0

TRACE gdbstub::protocol::recv_packet     > <-- $m0,4#fd
TRACE gdbstub::protocol::response_writer > --> $#00

Read partially accessible memory

(gdb) x/4xb 0x6554fffe
0x6554fffe:     0x00    0x00    Cannot access memory at address 0x65550000

TRACE gdbstub::protocol::recv_packet     > <-- $m6554fffe,1#35
TRACE gdbstub::protocol::response_writer > --> $00#60
TRACE gdbstub::protocol::recv_packet     > <-- $m6554ffff,1#36
TRACE gdbstub::protocol::response_writer > --> $00#60
TRACE gdbstub::protocol::recv_packet     > <-- $m65550000,1#5f
TRACE gdbstub::protocol::response_writer > --> $#00

Read after accessible area

(gdb) x 0x6555000e
0x6555000e:     Cannot access memory at address 0x6555000e

TRACE gdbstub::protocol::recv_packet     > <-- $m6555000e,4#97
TRACE gdbstub::protocol::response_writer > --> $#00

@daniel5151 daniel5151 self-requested a review October 28, 2022 03:27
daniel5151
daniel5151 requested changes Oct 28, 2022
View reviewed changes
Copy link
Owner

@daniel5151 daniel5151 left a comment

Choose a reason for hiding this comment

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

all up this looks good!

oh, and I suppose I don't actually mention it anywhere (I really should): I use nightly rustfmt in gdbstub to automatically wrap doc comments, so you'll have to fix that.

examples/armv4t/gdb/mod.rs Outdated Show resolved Hide resolved
src/stub/core_impl/base.rs Outdated Show resolved Hide resolved
@geigerzaehler
Copy link
Contributor Author

I’ve addressed all your comments. Sorry I took a while.

@daniel5151
Copy link
Owner

Sorry I took a while.

all good :)

I'll take a look in the next day or two

daniel5151
daniel5151 requested changes Nov 12, 2022
View reviewed changes
Copy link
Owner

@daniel5151 daniel5151 left a comment

Choose a reason for hiding this comment

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

one minor doc issue + a clippy lint that needs to get resolved, but aside from that, this LGTM!

src/target/ext/base/singlethread.rs Outdated Show resolved Hide resolved
@geigerzaehler
Copy link
Contributor Author

I‘ve fixed the lint and the docs. Should be good to go know 🤞

daniel5151
daniel5151 requested changes Dec 6, 2022
View reviewed changes
Copy link
Owner

@daniel5151 daniel5151 left a comment

Choose a reason for hiding this comment

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

...sorry to keep nitpicking, but in my earlier comment, I also mentioned that you needed to fix both places that had the doc issue.

src/target/ext/base/multithread.rs Outdated
/// to indicate that memory starting at `start_addr + n` cannot be
/// accessed.
///
/// Implemenations may also return an appropriate non-fatal if the requested
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
/// Implemenations may also return an appropriate non-fatal if the requested
/// Implemenations may also return an appropriate non-fatal error if the requested

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad. I fixed the second instance too

@geigerzaehler
read_addrs() can return fewer bytes than requested
d024c17 
The `read_addrs()` method for targets now returns a number of bytes that
were read. This allows a target to gracefully signal the client that
memory at an address range is not accessible.

Signed-off-by: Thomas Scholtes <geigerzaehler@axiom.fm>
daniel5151
daniel5151 approved these changes Dec 7, 2022
View reviewed changes
Copy link
Owner

@daniel5151 daniel5151 left a comment

Choose a reason for hiding this comment

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

Thanks for getting this over the finish line, I appreciate it :)

@daniel5151 daniel5151 merged commit 0b0c456 into daniel5151:dev/0.7 Dec 7, 2022
@geigerzaehler geigerzaehler deleted the read-addrs-result-size branch December 7, 2022 17:53
daniel5151 pushed a commit that referenced this pull request Apr 26, 2023
@geigerzaehler @daniel5151
read_addrs() can return fewer bytes than requested (#115)
dfdb1d3 
The `read_addrs()` method for targets now returns a number of bytes that
were read. This allows a target to gracefully signal the client that
memory at an address range is not accessible.

Signed-off-by: Thomas Scholtes <geigerzaehler@axiom.fm>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daniel5151 daniel5151 daniel5151 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@geigerzaehler @daniel5151