https://bugs.winehq.org/show_bug.cgi?id=56260
--- Comment #10 from Alex Henrie alexhenrie24@gmail.com --- (In reply to Fabian Maurer from comment #9)
I find the wowthunk diff hard to follow (that's why I tend to add extra commits just to fix indentation/names), but as I understand it you replace the lock with a suspension of threads instead.
I find it's easier to commit the patch and then look at it with `git show -w`. On the surface the patch seems a lot more invasive than it is.
- If we don't have an interrupt, in WOWTHUNK_CallWithRegs, why don't we use
the lock instead?
The only function that calls WOWTHUNK_CallWithRegs with interrupt=FALSE is NE_StartTask, and the only function that calls NE_StartTask is task_start, which already locks Win16Mutex for the duration of the call. It didn't seem necessary to lock the mutex again, which would simply increment the lock count. Do you see anything that might depend on that side effect?
- I don't really understand the WCB16_REGS flag yet, what's the difference?
I couldn't find any documentation about it, but it evidently restores a whole set of saved registers instead of doing an ordinary function call. For example, WCB16_REGS would let you jump to the middle of a function instead of the beginning.
- in task_start can't we move the assignment to teb before the TASK_LinkTask
and then let TASK_LinkTask/TASK_UnLinkTask handle the critical section themselves? If I understand correctly, you only have that outside to guard against therace condition where task is linked but teb not assigned.
I thought about that, but SetPriority16 needs to remove a running task from the list and then add it back to the end of the list, and Task_SuspendAll needs all potentially running tasks to be in the list at all times. So, SetPriority16 can't unlock task_list_cs in between TASK_UnlinkTask and TASK_LinkTask. Similarly, TASK_CreateMainTask needs to hold the lock in between when it starts the main task and when it adds it to the list. In task_start we could set pTask->teb before locking task_list_cs, but there doesn't seem to be much to gain one way or the other.
Apart from not understanding the interactions with the rest of the Win16 system, this looks good to me.
Thanks for looking it over!