-
Notifications
You must be signed in to change notification settings - Fork 24
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
implement ExitThread #56
base: main
Are you sure you want to change the base?
Conversation
x86/src/x86.rs
Outdated
@@ -238,8 +238,8 @@ impl X86 { | |||
let mut soonest = None; | |||
for (i, cpu) in self.cpus.iter().enumerate() { | |||
match cpu.state { | |||
CPUState::Running => {} | |||
CPUState::DebugBreak | CPUState::Error(_) | CPUState::Exit(_) => { | |||
CPUState::Running | CPUState::Exit(_) => {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This area is a little weird.
Right now CPUState::Exit is used to mark "the process overall wants to terminate" (see the implementation of exit_process
), and so the various places that consider CPU state, like this one, treat any CPU going into the exit state as a "quit the process now" marker.
Meanwhile, new_cpu
(the function used to start a new thread) doesn't consider any "free" CPUs (as would be freed up by ExitThread) for reuse, hmm.
I think with this modification it's possible for this loop to not find a CPU that is ready to run (if all the CPUs got into the exit state), which would mean the soonest.unwrap
below would fail.
I think, having typed all that out, calling this state "Exit" is the problem because it muddles "exit process" with "exit thread". So I think what you should do instead is make a new CPUState, maybe ::Free
, that a terminated thread goes into when it's no longer in use, and then use that in ExitThread, retrowin32_thread_main, and new_cpu.
BTW, if you run exe/rust/build.sh it builds a "threads.exe" program that can exercise some of this. I just added some build setup docs on how to build this: Not sure it's worth it for you, just FYI in case it helps |
88cde64
to
949a3a1
Compare
I've added a new diff --git a/x86/src/x86.rs b/x86/src/x86.rs
index 222969de..cfd8f386 100644
--- a/x86/src/x86.rs
+++ b/x86/src/x86.rs
@@ -69,6 +69,14 @@ impl CPU {
self.state = CPUState::Error(msg);
}
+ pub fn reset(&mut self) {
+ self.regs = Registers::default();
+ self.flags = Flags::empty();
+ self.fpu = FPU::default();
+ self.state = Default::default();
+ self.futures.clear();
+ }
+
// /// Check whether reading a T from mem[addr] would cause OOB, and crash() if so.
// fn check_oob<T>(&mut self, addr: u32) -> bool {
// if addr < NULL_POINTER_REGION_SIZE {
@@ -210,6 +218,13 @@ impl X86 {
}
pub fn new_cpu(&mut self) -> &mut CPU {
+ for i in 0..self.cpus.len() {
+ if self.cpus[i].state == CPUState::Free {
+ self.cpus[i].reset();
+ return &mut *self.cpus[i];
+ }
+ }
+
self.cpus.push(Box::pin(CPU::new()));
self.cpus.last_mut().unwrap()
} I started doing something like this, but I realized that I'm not too well versed in what parts should be reset. In particular, with the code I have, it seems like it might be better to just |
Oh yeah, swap_remove makes sense to me! I wonder if there's some way you can swap_remove the current CPU such that we don't even need the ::Free state, but I think it's probably hard for a CPU to tear itself down when the code is deep within some call stack involving it... |
FYI I dropped CPUStatus::Exit here c1735a7 |
No description provided.