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

Update single_step_gdb_behavior of all architectures with proper value #95

Merged
merged 2 commits into from Jan 30, 2022

Conversation

bet4it
Copy link
Contributor

@bet4it bet4it commented Jan 25, 2022

Description

I create a generic stub gdb_generic.rs which can be used to test the behavior of single step of all architectures. All you need is set architecture with the architecture you want to test before target remote, for example:

(gdb) set architecture powerpc:common
The target architecture is set to "powerpc:common".
(gdb) target remote :9001
Remote debugging using :9001
warning: No executable has been specified and target does not support
determining executable automatically.  Try using the "file" command.
0x00000000 in ?? ()
(gdb) stepi
0x00000000 in ?? ()

I don't consider the situation that single_step_gdb_behavior should be set to Optional because as I said in #92 (comment), it only possible happens on arm.

This generic stub uses the Dynamic Arch selection (#53) way.
And you may notice that resume and write_registers are totally useless for this real program, but I must implement them. We may make them optional?

API Stability

  • This PR does not require 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
    • Included a basic sample implementation in examples/armv4t
    • Included output of running examples/armv4t with RUST_LOG=trace + any relevant GDB output under the "Validation" section below
    • Confirmed that IDET can be optimized away (using ./scripts/test_dead_code_elim.sh and/or ./example_no_std/check_size.sh)
    • OR Implementation requires adding non-optional binary bloat (please elaborate under "Description")
  • If upstreaming an Arch implementation
    • I have tested this code in my project, and to the best of my knowledge, it is working as intended.

@daniel5151 daniel5151 self-requested a review January 26, 2022 03:58
@@ -263,7 +250,4 @@ pub enum SingleStepGdbBehavior {
///
/// e.g: MIPS
Ignored,
/// Unknown behavior - no one has tested this platform yet. If possible,
/// please conduct a test + upstream your findings to `gdbstub_arch`.
Unknown,
Copy link
Owner

Choose a reason for hiding this comment

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

Removing this variant would technically be a breaking API change, and given how "small" the change is, I'd rather not push out a whole new gdbstub 0.7 for that.

Instead, lets just mark this variant as #[doc(hidden)], and then I'll spin up a tracking task to remove the variant entirely whenever the next breaking release comes out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why you need to release gdbstub 0.7 only for this?
You can just leave this commit in the dev/0.7 branch and release it along with all the other changes when they are done in gdbstub 0.7.
And you can release gdbstub_arch 0.2.1 for this commit. Then people won't encounter breaking API change for this even gdbstub 0.7 is released in the future.

Copy link
Owner

Choose a reason for hiding this comment

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

yes, I know all this, but this PR is targeting master, not (the currently non-existent) dev/0.7 branch.
This is a good reminder that I should set up a dev/0.7 branch now as well, and once I do that, it'd be reasonable to open a second PR that removes this variant on that branch (though given how small the change is, that's something I'd just do myself...)

Please add the #[doc(hidden)] attribute as part of this PR, so that I can push out gdbstub 0.6.1 that soft-deprecates this variant, along with pushing out gdbstub_arch 0.2.1, which includes the semantic Arch changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I add SingleStepGdbBehavior::Unknown back, although I'm really reluctant to do this.
I think it's really not necessary to push out gdbstub 0.6.1 that the only difference with 0.6.0 is #[doc(hidden)], because even if Unknown exists, nobody will use it. People won't notice it if they use the defined architecture, and if they want to create a new architecture, they will clear it is not something they need. You only need to push out gdbstub_arch 0.2.1.

That said, given that these changes would all be non-breaking and land in gdbstub_arch, it's fine to have some Unknowns for now and update things as folks have time to look into each arch.

So the first commit after gdbstub 0.6.0 is a breaking API change🙃


Fine, it's your project and you are always right. You can just revert the later commit after you create the dev/0.7 branch🙂

Copy link
Owner

Choose a reason for hiding this comment

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

When I implemented the Unknown variant, I knew full well that it'd end up getting removed fairly quickly, and that I'd need to push out a new minor gdbstub_arch release, along with (possibly) a new minor gdbstub release that soft-deprecates the newly added variant.

While it may seem silly to add a feature to 0.6 that immediately gets soft deprecated, the reality is that - for my own sanity - I really just wanted to get 0.6 out the door ¯\_(ツ)_/¯

So the first commit after gdbstub 0.6.0 is a breaking API change🙃

Not super sure what you mean by that.

Of course, whatever first commit ends up in dev/0.7 will be a breaking change. That's the whole point of opening a new dev branch - to put in any new breaking changes.

It could have just as easily been the case that you disappeared for a few months, and in the meantime, someone else came along and opened a PR to add a new non-breaking protocol extension to the library, which wouldn't even require opening a new dev/0.7 branch

@daniel5151
Copy link
Owner

I don't consider the situation that single_step_gdb_behavior should be set to Optional because as I said in #92 (comment), it only possible happens on arm.

Ehh, I don't have the energy to argue here. Given that going from Required -> Optional is a "loosening" of behavior, I don't mind if it ends up getting changed as some point down the line.


And you may notice that resume and write_registers are totally useless for this real program, but I must implement them. We may make them optional?

wrt. resume: I totally forgot to rename resume to cont for the single thread case, that's my bad. That said, I do consider support for the c resume reason a requirement for any "resumable" target, hence why the method is non-optional. We've already had this discussion, and I think we came to the conclusion of "agree to disagree" here.

wrt. write_registers: once again, please refer to the following line in the GDB RSP spec:

At a minimum, a stub is required to support the ‘?’ command to tell GDB the reason for halting, ‘g’ and ‘G’ commands for register access, and the ‘m’ and ‘M’ commands for memory access.

@daniel5151 daniel5151 merged commit a6e9703 into daniel5151:master Jan 30, 2022
@daniel5151
Copy link
Owner

gdbstub_arch 0.2.1 has been published to crates.io that includes these changes :)

I've decided to hold off on releasing gdbstub 0.6.1 just yet, since I'm inclined to agree that a whole new minor commit just for a doc change seems a bit silly.

If I misunderstood your intent, and you'd like to see 0.6.1 published with this doc tweak, do let me know.

@bet4it bet4it deleted the step branch January 31, 2022 01:52
@bet4it
Copy link
Contributor Author

bet4it commented Jan 31, 2022

I've decided to hold off on releasing gdbstub 0.6.1 just yet, since I'm inclined to agree that a whole new minor commit just for a doc change seems a bit silly.

Yes, that's what I want!
But if we don't make a new release for this, why I need to add SingleStepGdbBehavior::Unknown back🙃 That's why I said I'm reluctant to do this.
If you still have some big non-breaking changes that want to be included in 0.6, then it's understandable.

@daniel5151
Copy link
Owner

I should probably clarify how I structure branches on this repo:

  • master only ever includes semver-compatible changes to latest release of gdbstub (which is currently 0.6)
  • dev/0.X branches are master + any breaking changes

Do you see why I asked you to re-add Unknown?

This PR is being merged into master, which means it must be semver compatible with 0.6, which means we cannot remove an enum variant as part of this PR. That would be a semver violation.

Instead, we've merged this PR into master in a semver-compatible way (i.e: marking Unknown as doc(hidden), and I've opened a tracking task to remove the variant entirely whenever I end up creating a new dev/0.7 branch for breaking changes.

@bet4it
Copy link
Contributor Author

bet4it commented Jan 31, 2022

IIRC, you created dev/0.6 branch as soon as you release 0.5, and didn't maintain a master branch that kept compatibility with 0.5 at that time, that's what I want this time: we can just do our work on dev/0.7 branch if you don't have further plan on 0.6.

@daniel5151
Copy link
Owner

Yeah, that was just a fluke :)
It just so happened that after releasing 0.5, I very quickly realized that there were some breaking changes I needed to make. I was hoping to release at least a couple minor releases, but that never really happened...

The 0.4 -> 0.5 transition is a better example - in that case, there were quite a few non-breaking features implemented on master before I ended up making dev/0.5 for breaking changes.


Just so you know, now that 0.6 is released, I'm going to be taking a step back from gdbstub for a bit. Of course, I'll continue reviewing PRs and addressing issues, but I won't be doing any active feature development for a while.

I'm hoping that in that time, folks will still send smaller, semver-compatible protocol extension IDET PRs (targeting the master branch), and that I could release a couple 0.6.x minor versions with those changes.

That said, it sounds like you have some breaking-change PRs you'd like to send my way. If that's the case, I've gone ahead and make a dev/0.7 branch, which you can target those PRs against :)

@bet4it
Copy link
Contributor Author

bet4it commented Feb 1, 2022

Oh, I don't have other breaking-change PRs to be sent, this is the only breaking-change PR😅

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

2 participants