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

implement ExitThread #56

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

implement ExitThread #56

wants to merge 1 commit into from

Conversation

LinusU
Copy link
Contributor

@LinusU LinusU commented Sep 15, 2024

No description provided.

@LinusU LinusU mentioned this pull request Sep 15, 2024
27 tasks
win32/src/winapi/stack_args.rs Outdated Show resolved Hide resolved
win32/src/winapi/kernel32/thread.rs Outdated Show resolved Hide resolved
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(_) => {}
Copy link
Owner

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.

@evmar
Copy link
Owner

evmar commented Sep 21, 2024

BTW, if you run exe/rust/build.sh it builds a "threads.exe" program that can exercise some of this.
https://github.com/evmar/retrowin32/blob/main/exe/rust/README.md

I just added some build setup docs on how to build this:
https://github.com/evmar/retrowin32/blob/main/doc/build_setup.md

Not sure it's worth it for you, just FYI in case it helps

@LinusU LinusU force-pushed the ExitThread branch 2 times, most recently from 88cde64 to 949a3a1 Compare September 22, 2024 12:59
@LinusU
Copy link
Contributor Author

LinusU commented Sep 22, 2024

I've added a new Free state, and was going to implement cpu reuse in new_cpu but not sure exactly how to proceed 🤔

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 swap_remove the cpu instead of keeping it around as free? 🤔

@evmar
Copy link
Owner

evmar commented Sep 23, 2024

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...

@evmar
Copy link
Owner

evmar commented Sep 25, 2024

FYI I dropped CPUStatus::Exit here c1735a7

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.

2 participants