https://bugs.winehq.org/show_bug.cgi?id=56260
Bug ID: 56260 Summary: 16-bit Myst deadlocks when entering Book Product: Wine Version: 9.1 Hardware: x86-64 OS: Linux Status: NEW Severity: normal Priority: P2 Component: -unknown Assignee: wine-bugs@winehq.org Reporter: dark.shadow4@web.de Distribution: ---
First, work around bug 56225.
- Run winecfg and mount the Myst/cd folder as D:\ - Run `sudo sysctl vm.mmap_min_addr=0` - In one terminal, run `Xephyr :2 -ac -screen 800x600x8` - In another terminal, run `DISPLAY=:2 openbox` - In a third terminal, cd to "Myst/win3.1/MYST/" and run `DISPLAY=:2 wine 'MYST.EXE'`
Click through the intro and into the image in the book, this should get you into the first world. On wine it just plays a sound, and then locks up.
https://bugs.winehq.org/show_bug.cgi?id=56260
--- Comment #1 from Fabian Maurer dark.shadow4@web.de --- Created attachment 75971 --> https://bugs.winehq.org/attachment.cgi?id=75971 Win16 code that it is stuck in
When the deadlock occurs, winevdm uses one thread fully. Attaching the disassembly that is continuously run. As I see it, it just waits for a variable to be written. This variable should (AFAIK) be written when the waveOutOpen callback gets called with WOM_DONE.
Not exactly sure how that is supposed to work with cooperative multitasking, but I suspect that is an interrupt that allows that to work.
https://bugs.winehq.org/show_bug.cgi?id=56260
--- Comment #2 from Fabian Maurer dark.shadow4@web.de --- Created attachment 75972 --> https://bugs.winehq.org/attachment.cgi?id=75972 Test program to reproduce the issue
Attaching a test program that does what I think Myst is doing. Compile using OpenWatcom (command included in file).
For some reason, I get "invalid parameter" on my XP VM, Win3.1 Dosbox and Win 2000 VM... But another user in IRC tested on a real Win98 Machine and it works.
It hangs in Wine though, so I do think this is the issue. On Wine the WOM_DONE message is never sent because the thread tries to get the Win16 lock mutex, but since one thread is currently running that fails and waits forever.
https://bugs.winehq.org/show_bug.cgi?id=56260
--- Comment #3 from Fabian Maurer dark.shadow4@web.de --- Created attachment 75973 --> https://bugs.winehq.org/attachment.cgi?id=75973 Patch to avoid deadlock
Attaching a patch to avoid the deadlock. Not 100% sure this is the right thing to do, but my example serves as evidence that the callback should be possible even if the program is spinning endlessly.
https://bugs.winehq.org/show_bug.cgi?id=56260
Alex Henrie alexhenrie24@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |alexhenrie24@gmail.com
--- Comment #4 from Alex Henrie alexhenrie24@gmail.com --- You did an amazing job debugging this, thank you Fabian!
Instead of using a mutex to ensure that only one 16-bit thread is running at a time, I wonder if we could make the callback pause/suspend the original thread to simulate what would happen on Windows 3.1 during an interrupt.
https://bugs.winehq.org/show_bug.cgi?id=56260
--- Comment #5 from Fabian Maurer dark.shadow4@web.de --- Created attachment 75991 --> https://bugs.winehq.org/attachment.cgi?id=75991 Second Testprogram to reproduce the issue
I found a way to work around the "Invalid parameter" issue, attaching a new testprogram that hacks around the issue (Win16 progamming is a massive PITA, did I ever say that?) Anyways, this should work at least on XP with original ntvdm.
Instead of using a mutex to ensure that only one 16-bit thread is running at a time, I wonder if we could make the callback pause/suspend the original thread to simulate what would happen on Windows 3.1 during an interrupt.
Sounds at least sensible to me, after all (if I understand things right) 16bit programs allow only one running thread and having it run code outside of where it's expected could lead to issues.
Btw, not sure if that is obvious, but I couldn't use ReleaseThunkLock/RestoreThunkLock because that thread doesn't own the Win16 Mutex so it can't release it.
Could also be interesting to see how otvdm does it, from what I saw they also employ a mutex, but that sample (and MYST) work there. Maybe there's something to be learned here?
https://bugs.winehq.org/show_bug.cgi?id=56260
--- Comment #6 from Alex Henrie alexhenrie24@gmail.com --- Created attachment 76136 --> https://bugs.winehq.org/attachment.cgi?id=76136 [PATCH] krnl386: Pause other tasks while executing a callback.
Hi Fabian, I put together a patch to emulate 16-bit hardware interrupts in Wine by suspending other task threads while executing the callback. Could you take a look before I send it upstream?
https://bugs.winehq.org/show_bug.cgi?id=56260
--- Comment #7 from Fabian Maurer dark.shadow4@web.de --- Comment on attachment 75991 --> https://bugs.winehq.org/attachment.cgi?id=75991 Second Testprogram to reproduce the issue
/* WATCOM=/opt/watcom PATH=$WATCOM/binl:$PATH INCLUDE=$WATCOM/h:$WATCOM/h/win LIB=$WATCOM/lib286:$WATCOM/lib286/dos wcl -bcl=windows main.c -q mmsystem.lib -zu */
#include <windows.h> #include <stddef.h> #include <stdlib.h> #include <stdio.h> #include <mmsystem.h> #include <i86.h>
/* Error handling */
void error(const char* str) { MessageBox((HWND)0, str, "", 0); exit(1); }
void errori(const char* str, DWORD i) { static char t[20]; sprintf(t, "%s %lx", str, i); error(t); }
void mesi(const char* str, DWORD i) { static char t[20]; sprintf(t, "%s %lx", str, i); MessageBox((HWND)0, t, "", 0); }
void mes(const char* str) { MessageBox((HWND)0, str, "", 0); }
/* HOOK Stuff */
typedef struct { BYTE code_original[5]; BYTE code_hooked[5]; BOOL hooked; LPBYTE address; UINT selector; FARPROC callback; HGLOBAL hhook; } _hook_info; typedef _hook_info FAR* hook_info;
typedef UINT (WINAPI* tAllocCsToDsAlias)(UINT);
hook_info hook_init(HINSTANCE instance, const char* name_lib, const char* name_func, FARPROC hook_func) { HMODULE mod_kernel, mod_lib; FARPROC func; tAllocCsToDsAlias pAllocCsToDsAlias; int i; HGLOBAL hhook; hook_info hook;
hhook = GlobalAlloc(GMEM_MOVEABLE, sizeof(_hook_info)); hook = (hook_info)GlobalLock(hhook); hook->hhook = hhook; mod_kernel = GetModuleHandle("KERNEL"); mod_lib = GetModuleHandle(name_lib);
pAllocCsToDsAlias = (tAllocCsToDsAlias)GetProcAddress(mod_kernel,"AllocCsToDsAlias"); if (!pAllocCsToDsAlias) { error("AllocCsToDsAlias missing"); }
func = GetProcAddress(mod_lib, name_func); if (!func) { error("Can't get original func"); }
hook->callback = MakeProcInstance((FARPROC)hook_func, instance); if (hook->callback == NULL || hook->callback == hook_func) { error("MakeProcInstance failed"); }
hook->hooked = FALSE; hook->selector = pAllocCsToDsAlias(FP_SEG(func)); hook->address = (LPBYTE)MK_FP(hook->selector, FP_OFF(func)); if (hook->address[0] == 0xEA) { error("Already hooked, something went terribly wrong!"); } hook->code_hooked[0] = 0xEA; *(LPDWORD)(&hook->code_hooked[1]) = (DWORD)hook->callback;
for (i = 0; i< 5; i++) hook->code_original[i] = hook->address[i];
return hook; }
void hook_enable(hook_info hook) { int i; if (hook->hooked) return;
for (i = 0; i < 5; i++) hook->address[i] = hook->code_hooked[i]; hook->hooked = TRUE; }
void hook_disable(hook_info hook) { int i; if (!hook->hooked) return;
for (i = 0; i < 5; i++) hook->address[i] = hook->code_original[i]; hook->hooked = FALSE; }
void hook_free(hook_info hook) { HGLOBAL hhook; hook_disable(hook); FreeSelector(hook->selector); FreeProcInstance(hook->callback); hhook = hook->hhook; GlobalUnlock(hhook); GlobalFree(hhook); }
/* Main code */
static hook_info hook;
static int done = 0;
void CALLBACK _export waveOutProc(HWAVEOUT hwo, UINT uMsg, DWORD dwInstance, DWORD dwParam1, DWORD dwParam2) { if (uMsg == WOM_DONE) { done = 1; } }
int WINAPI _export HookGetCodeInfoFunc(FARPROC proc, SEGINFO FAR* seginfo) { hook_disable(hook); GetCodeInfo(proc, seginfo); hook_enable(hook);
seginfo->flags &= ~(0x10); return 1; }
BOOL IsWine(void) { HMODULE kernel = GetModuleHandle("KERNEL"); FARPROC proc = GetProcAddress(kernel, "__wine_call_int_handler"); return proc != NULL; }
int PASCAL WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine, int nCmdShow) { HWAVEOUT hout; PCMWAVEFORMAT format; WAVEHDR NEAR* block; int result; FARPROC callback; char NEAR* buffer; OFSTRUCT ofstruct; HFILE file;
if (!IsWine()) { hook = hook_init(hInstance, "KERNEL", "GetCodeInfo", (FARPROC)HookGetCodeInfoFunc); hook_enable(hook); }
if ((file = OpenFile("test.wav", &ofstruct, OF_READ)) == HFILE_ERROR) { error("CreateFile failed"); }
block = (WAVEHDR NEAR*)LocalAlloc(0, 8000 + sizeof(WAVEHDR));
buffer = (char NEAR*)block + sizeof(WAVEHDR); block->dwBufferLength = 8000; block->lpData = buffer;
if (_lread(file, buffer, 8000) == HFILE_ERROR) { error("ReadFile failed"); }
format.wf.wFormatTag = WAVE_FORMAT_PCM; format.wf.nChannels = 1; format.wf.nSamplesPerSec = 11025; format.wf.nBlockAlign = 1; format.wf.nAvgBytesPerSec = 11025; format.wBitsPerSample = 8;
callback = MakeProcInstance((FARPROC)waveOutProc, hInstance); if (callback == NULL) { error("MakeProcInstance failed"); }
if ((result = waveOutOpen(&hout, WAVE_MAPPER, (LPWAVEFORMAT)&format, (DWORD)callback, 0, CALLBACK_FUNCTION)) != MMSYSERR_NOERROR) { errori("waveOutOpen failed", result); } if (!IsWine()) { hook_free(hook); } waveOutPrepareHeader(hout, block, sizeof(WAVEHDR)); waveOutWrite(hout, block, sizeof(WAVEHDR)); while(!done) { } error("success!"); return 0; }
https://bugs.winehq.org/show_bug.cgi?id=56260
Fabian Maurer dark.shadow4@web.de changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #75972|0 |1 is obsolete| | Attachment #75991|0 |1 is obsolete| |
--- Comment #8 from Fabian Maurer dark.shadow4@web.de --- Created attachment 76137 --> https://bugs.winehq.org/attachment.cgi?id=76137 Second Testprogram to reproduce the issue v2
Wanted to edit the attachment, guess that just created a comment-spam. sorry about that. Here the fixed testprogram.
https://bugs.winehq.org/show_bug.cgi?id=56260
--- Comment #9 from Fabian Maurer dark.shadow4@web.de ---
Hi Fabian, I put together a patch to emulate 16-bit hardware interrupts in Wine by suspending other task threads while executing the callback. Could you take a look before I send it upstream?
Sure, thanks!
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.
Questions: - If we don't have an interrupt, in WOWTHUNK_CallWithRegs, why don't we use the lock instead? - I don't really understand the WCB16_REGS flag yet, what's the difference? - 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.
Apart from not understanding the interactions with the rest of the Win16 system, this looks good to me.
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!
https://bugs.winehq.org/show_bug.cgi?id=56260
--- Comment #11 from Fabian Maurer dark.shadow4@web.de ---
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?
Not really, but I'm missing the bigger picture. Win16 lock has the weird behavior that can be gotten multiple times recursively, but I don't know how important the actual count is. Probably not too important.
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.
Ah, I see.
-
WineHQ Bugzilla