Repository navigation
Rework scheduler locking - #197
Conversation
daniel5151
left a comment
daniel5151
left a comment
There was a problem hiding this comment.
Excellent first PR! A few minor comments, but at first blush - I don't see anything wrong with this approach.
And yeah, no worries about providing the big mess of logs. There are a lot of scenarios, and aside from trusting that you're validating the changes as you work on them, I'm sure I'll also spend some time poking around with the examples as we progress further along in 0.8's development
|
Sounds good! Typically with PRs after feedback, I amend my commit and force push, though I'm not sure how well GitHub handles this (most of my experience is with GitLab, which handles this...decently enough). I know this is a controversial practice though; do you have any strong feelings about this? I'm comfortable enough just pushing new commits if that's what you prefer. |
|
The PR is gonna get squash-merge'd, so you can feel free to iterate on it however you like! |
9e4cf4f to
0c0e6ff
Compare
|
After incorporating the changes to |
ff70565 to
d78fe5c
Compare
| // "Specifying no actions is an error." | ||
| b"" => None, | ||
| b"?" => Some(vCont::Query), | ||
| _ => Some(vCont::Actions(Actions::new_from_buf(body))), |
There was a problem hiding this comment.
I believe the following formulation will let you avoid the unnecessary empty-body handling in the iterator:
| _ => Some(vCont::Actions(Actions::new_from_buf(body))), | |
| [b';', rest @ ..] => Some(vCont::Actions(Actions::new_from_buf(rest))), | |
| _ => None, |
There was a problem hiding this comment.
I thought about doing that at first too, but the protocol specifies that the leading semicolon is part of the action, so I figured keeping the semicolon in there is a more pure representation of the actions, and doesn't carry around the cognitive baggage of having to remember "this datatype has the full contents of every action *except the first one, which is missing the leading semicolon".
That being said, it's small enough and you could just as easily make the case that removing the leading semicolon cleans up the loop, and it's just shifting the cognitive baggage between the code versus datastructures, and it becomes a matter of preference.
Just wanted to offer an alternative viewpoint; I'll plan to implement the suggestion, unless you say otherwise!
There was a problem hiding this comment.
Yeah, I think it's totally fine treating individual packet parser modules (i.e: _vCont.rs, etc...) as single "cognitive domains". And even though the iterator data structure does end up being used outside the module, external consumers don't need to care about how the parser works (i.e: whether it was front-loaded vs. deferred, etc...).
| // ourselves with: 1 action, or 2 actions where the second is a | ||
| // continue action (which occurs when XXX?) that we ignore. |
There was a problem hiding this comment.
I believe the behavior I observed in the past was that GDB would send over a packet along the lines of vCont;s:foo,c, even in single-threaded scenarios.
| /// and end (exclusive) addresses, or another stop condition is met | ||
| /// (e.g: a breakpoint it hit). | ||
| /// | ||
| /// If no thread is specified, step all threads, if supported. |
There was a problem hiding this comment.
I know I just said that we should apply this transform to all functions... but now that I see this change in the context of range stepping, I wonder if this is actually the right call?
Sure, at a protocol level, this would be valid... but I wonder if it's actually possible (in practice) to configure GDB / LLDB to send over a range-step action as the fallback for all threads? Heck, same thought applies to single-step as a fallback action, and the reverse-step/continue actions as well.
Maybe for v0, we should leave all actions except for set_resume_action_continue as taking a specific tid, and adding some error-handling inside gdbstub to warn the user if the GDB client tries to use a non-continue fallback action (and, in turn - to open an issue on github reporting how they managed to get into that situation)?
At that point, assuming someone does stumble across a valid debugging scenario with a non-continue default resume actions... it shouldn't be too hard to add new (backwards-compatible) set_resume_action_{step,reange_step}_as_fallback IDETs to allow hooking into that functionality.
The benefit is that in the common case where a client + target do not support fallback actions aside from continue, the implementation of set_resume_action_* foo methods don't need to include error handling for None thread IDs.
There was a problem hiding this comment.
Heh, that's why I left it off of the step resume actions in the first iteration, but figured it probably wasn't too much to ask of targets to complain about not being able to handle it.
The error message for PacketUnexpected (the original error returned in this case) seems to accomplish this, saying it's something we don't expect to happen, and a request to file an issue, so I'm planning to just revert these changes, unless you'd prefer a more specific error variant
There was a problem hiding this comment.
PacketUnexpected is the "cop out" error message when past me was too lazy to add in a proper error message with more context, hah.
I'm fine using it here for expediency (and wouldn't block the PR on it), but if you want to add a proper error message with some more context, that might be better.
Error paths as a whole need more love, but that's a larger work stream in and of itself #112
d78fe5c to
0949523
Compare
|
In my last comment regarding the |
daniel5151
left a comment
daniel5151
left a comment
There was a problem hiding this comment.
2 small nits, but otherwise LGTM!
| if self.exec_mode.is_empty() { | ||
| // This should never happen (gdbstub will always ensure at least one | ||
| // `set_resume_action_XXX` method is called), but in case it does, we explicitly | ||
| // log it and return the closest event that represents this. | ||
| eprintln!("Running while all threads are stopped; this should never happen! Treating as 0 steps"); | ||
| return RunEvent::Event(Event::DoneStep, CpuId::Cpu); | ||
| } |
There was a problem hiding this comment.
again, I really think this entire block should just be omitted.
The docs and gdbstub implementation make it clear that the contract forbids calling this method without having called set_resume_action_*, so we shouldn't nudge end users to hedge against bugs in gdbstub.
If you want to leave this in purely in the example code, then swap it out with:
| if self.exec_mode.is_empty() { | |
| // This should never happen (gdbstub will always ensure at least one | |
| // `set_resume_action_XXX` method is called), but in case it does, we explicitly | |
| // log it and return the closest event that represents this. | |
| eprintln!("Running while all threads are stopped; this should never happen! Treating as 0 steps"); | |
| return RunEvent::Event(Event::DoneStep, CpuId::Cpu); | |
| } | |
| assert!(!self.exec_mode.is_empty(), "gdbstub violated resume() contract (called prior to calling `set_resume_action_XXX` at least once"); |
There was a problem hiding this comment.
Whoops! For some reason I thought you only meant the block in the resume() function, not this one. I just removed the block entirely.
| b"?" => Some(vCont::Query), | ||
| _ => Some(vCont::Actions(Actions::new_from_buf(body))), | ||
| [b';', rest @ ..] => Some(vCont::Actions(Actions::new_from_buf(rest))), | ||
| // Anything that doesn't start with a semicolon is malformed |
There was a problem hiding this comment.
nix the comment (the ? case is literally 2 lines up lol)
There was a problem hiding this comment.
Looking at it again, the empty case also doesn't need to exist, so I took that out too
This lays down some groundwork for v0.8, where the primary new feature will be multiprocess support, which will be similar to multithreaded support. As discussed on the multiprocess github support issue, there's a simpler way to support the scheduler locking behavior that we plan to use for multiprocess targets, and since we're breaking the API anyways with v0.8, it's a decent time to implement it for multithreaded targets as well.
0949523 to
2acb95f
Compare
2 participants
Description
This lays down some groundwork for v0.8, where the primary new feature will be multiprocess support, which will be similar to multithreaded support.
As discussed on the multiprocess github support issue, there's a simpler way to support the scheduler locking behavior that we plan to use for multiprocess targets, and since we're breaking the API anyways with v0.8, it's a decent time to implement it for multithreaded targets as well.
While I was in the area, I fixed some typos I noticed when I had copied most of these during local development of multiprocess-extensions.
This PR does NOT do anything about the proposed changes in this comment regarding the order in which we parse resume actions laid out here: #124 (comment).
API Stability
This is part of the initial transition to v0.8. Justification for breaking the API is provided here: #124 (comment)
Another quick API breakage that might make sense to throw in this PR too is changing the name of
is_thread_alivetocheck_thread_alive(available for bikeshedding), which removes the need for the#[allow(clippy::wrong_self_convention)].Checklist
rustdocformatting looks good (viacargo doc)examples/armv4twithRUST_LOG=trace+ any relevant GDB output under the "Validation" section below./example_no_std/check_size.shbefore/after changes under the "Validation" section belowexamples/armv4t./example_no_std/check_size.sh)ArchimplementationValidation
I tested a lot of different scenarios in armv4t for both
dev/0.8and this PR. I'm taking a "ask forgiveness, not permission" here approach for the logs; there's a lot of logs to generate (which also means a lot of logs for you to sift through), so for now I'll list all the scenarios I tested, and I can post logs on request (or test alternative scenarios).continuefrom thread 1continuefrom thread 2nexti 10from thread 1nexti 80from thread 2 (which led to the discovery of Remove / Rename syntheticDoneStepstop reason #196)continuefrom thread 1 (program hangs; I needed to ctrl+C, switch to thread 2,continue, ctrl+C, switch to thread 1,continue, which makes sense given the behavior ofscheduler-lockand the example binary)continuefrom thread 2 (program hangs; ctrl+C'ing and switching between threads doesn't actually seem to ever resolve. When I have more time I'd like dig into the example binary to figure out why that might be and if there's a deeper underlying bug, but this is the behavior exhibited bydev/0.8, so for the purposes of this PR I stopped investigating there)nexti 10from thread 1nexti 80from thread 2Let me know if you would like logs for any (...or all) of these, or if there are any situations I didn't consider.
Before/After `./example_no_std/check_size.sh` output
Before
After