Markus Amsler wrote:
There's a fundamental bug in winedos/dosvm. c[1]. SIGUSR2 is sent to the current process, but it should be sent to the dosvm thread. This causes various ugly bugs. I found no way signaling a unix thread, without modifing the wineserver. I have implemented a thread_kill command in the wineserver, this solves my problem, but i'm quite sure it won't get applied.
I'm really not a wineserver/threading expert. Have I missed something?
Yep. If using Linux 2.6 kernel and recent pthread library, threads share the same pid and they must be signalled using tkill and not kill. The only place which knows how to correctly signal threads is send_thread_signal function in server/ptrace.c. So, the easy way to fix signalling is to use Windows thread identifier for identifying dosvm thread instead of pids (which don't work anyway) and add to wineserver a handler that just calls send_thread_signal, which I guess is what you have been doing, anyway.
Now, I have tried this approach and it seems to work. I have some prototype implementation lying around but I was thinking about doing something more complicated at the time, like using SIGUSR2 handler for doing Win32 remote operations discussed earlier on wine-devel, because signal handler is probably running in the correct context for doing this kind of things but I guess locking issues are going to be pretty complicated so it is probably better to just fix dosvm issues with a simple mechanism and not worry about anything else.
So, feel free to submit a patch, if it is simple enough it should get accepted or at least you should get some suggestions how to change the patch in order for it to get accepted.
Jukka Heinonen jhei@iki.fi writes:
Now, I have tried this approach and it seems to work. I have some prototype implementation lying around but I was thinking about doing something more complicated at the time, like using SIGUSR2 handler for doing Win32 remote operations discussed earlier on wine-devel, because signal handler is probably running in the correct context for doing this kind of things but I guess locking issues are going to be pretty complicated so it is probably better to just fix dosvm issues with a simple mechanism and not worry about anything else.
It would certainly be nice if we could convince dosvm to stop using SIGUSR2, there are lots of other things that could make better use of it. I don't really have a good solution to offer though (except maybe getting rid of dosvm altogether ;-)
Alexandre Julliard:
It would certainly be nice if we could convince dosvm to stop using SIGUSR2, there are lots of other things that could make better use of it. I don't really have a good solution to offer though (except maybe getting rid of dosvm altogether ;-)
Well, I'm not going to comment about dosvm usefulness, but I guess there are quite a few types of asynchronous requests Windows processes make to each other, so some kind of multiplexing system is going to be needed, anyway, and dosvm notification signal probably could use the same system without adding any extra complexity.
I was myself thinking about using a single signal whose handler just pulls asynchronous requests from wineserver as an implementation of the multiplexing system. Locking issues could be fixed using worker threads, signal safe critical section implementation or explicit signal blocking in protected code paths. But since there are quite a few ways for implementing multiplexing and fixing the resulting locking issues I probably just wait and see what the implementation turns out to be.
On Sun, 07 Nov 2004 12:10:15 +0200, Jukka Heinonen wrote:
I was myself thinking about using a single signal whose handler just pulls asynchronous requests from wineserver as an implementation of the multiplexing system. Locking issues could be fixed using worker threads, signal safe critical section implementation or explicit signal blocking in protected code paths. But since there are quite a few ways for implementing multiplexing and fixing the resulting locking issues I probably just wait and see what the implementation turns out to be.
I quite liked Michaels idea of the RT signals. Is there some reason we can't use them?
Mike Hearn mike@navi.cx writes:
I quite liked Michaels idea of the RT signals. Is there some reason we can't use them?
Portability, plus we don't really want queued behavior anyway.
Jukka Heinonen wrote:
Yep. If using Linux 2.6 kernel and recent pthread library, threads share the same pid and they must be signalled using tkill and not kill.
Yes, you're right. I already wondered why dosvm worked before, it was because I used a 2.4 kernel.
The only place which knows how to correctly signal threads is send_thread_signal function in server/ptrace.c. So, the easy way to fix signalling is to use Windows thread identifier for identifying dosvm thread instead of pids (which don't work anyway) and add to wineserver a handler that just calls send_thread_signal, which I guess is what you have been doing, anyway.
No, I did it much more uglier, that's why I asked the list. Thanks.
Alexandre Julliard wrote:
Jukka Heinonen jhei@iki.fi writes:
Now, I have tried this approach and it seems to work. I have some prototype implementation lying around but I was thinking about doing something more complicated at the time, like using SIGUSR2 handler for doing Win32 remote operations discussed earlier on wine-devel, because signal handler is probably running in the correct context for doing this kind of things but I guess locking issues are going to be pretty complicated so it is probably better to just fix dosvm issues with a simple mechanism and not worry about anything else.
It would certainly be nice if we could convince dosvm to stop using SIGUSR2, there are lots of other things that could make better use of it. I don't really have a good solution to offer though (except maybe getting rid of dosvm altogether ;-)
What about using a win32 compatible way to signal dosvm thread (I know in win32 there are no signals, but perhaps there's a undocu/hackish way). So one day winedos could compile/run on ReactOS/Windows.
I also attached the patch which solves my signalling issues with kernel 2.6.8 and libc 2.3.2. I'm pretty unsure about the wineserver stuff, so comments are welcome.
Markus
? thread_kill.diff Index: dlls/winedos/dosvm.c =================================================================== RCS file: /home/wine/wine/dlls/winedos/dosvm.c,v retrieving revision 1.59 diff -u -r1.59 dosvm.c --- dlls/winedos/dosvm.c 4 Aug 2004 19:08:19 -0000 1.59 +++ dlls/winedos/dosvm.c 31 Oct 2004 19:00:30 -0000 @@ -51,6 +51,7 @@ #include "thread.h" #include "dosexe.h" #include "dosvm.h" +#include "wine/server.h" #include "wine/debug.h" #include "excpt.h"
@@ -58,6 +59,8 @@ WINE_DECLARE_DEBUG_CHANNEL(module); WINE_DECLARE_DEBUG_CHANNEL(relay);
+extern HANDLE dosvm_thread, loop_thread; + WORD DOSVM_psp = 0; WORD DOSVM_retval = 0;
@@ -278,11 +281,22 @@ else pending_event = event;
if (!old_pending && DOSVM_HasPendingEvents()) { + BOOL success; TRACE("new event queued, signalling (time=%ld)\n", GetTickCount());
/* Alert VM86 thread about the new event. */ - kill(dosvm_pid,SIGUSR2); - + SERVER_START_REQ( send_thread_signal ) + { + req->handle = dosvm_thread; + req->signal = SIGUSR2; + wine_server_call( req ); + success = reply->success; + } + SERVER_END_REQ; + + if (!success) + ERR ("Couldn't alert vm86 thread!\n"); + /* Wake up DOSVM_Wait so that it can serve pending events. */ SetEvent(event_notifier); } else { Index: dlls/winedos/module.c =================================================================== RCS file: /home/wine/wine/dlls/winedos/module.c,v retrieving revision 1.45 diff -u -r1.45 module.c --- dlls/winedos/module.c 18 Oct 2004 21:19:57 -0000 1.45 +++ dlls/winedos/module.c 31 Oct 2004 19:00:31 -0000 @@ -103,9 +103,9 @@ /* global variables */
pid_t dosvm_pid; +HANDLE dosvm_thread, loop_thread;
static WORD init_cs,init_ip,init_ss,init_sp; -static HANDLE dosvm_thread, loop_thread; static DWORD dosvm_tid, loop_tid;
static void MZ_Launch( LPCSTR cmdtail, int length ); Index: include/wine/server_protocol.h =================================================================== RCS file: /home/wine/wine/include/wine/server_protocol.h,v retrieving revision 1.111 diff -u -r1.111 server_protocol.h --- include/wine/server_protocol.h 18 Aug 2004 00:04:58 -0000 1.111 +++ include/wine/server_protocol.h 31 Oct 2004 19:00:35 -0000 @@ -3095,6 +3095,20 @@ #define SET_GLOBAL_TASKMAN_WINDOW 0x04
+ +struct send_thread_signal_request +{ + struct request_header __header; + obj_handle_t handle; + int signal; +}; +struct send_thread_signal_reply +{ + struct reply_header __header; + int success; +}; + + enum request { REQ_new_process, @@ -3274,6 +3288,7 @@ REQ_set_clipboard_info, REQ_open_token, REQ_set_global_windows, + REQ_send_thread_signal, REQ_NB_REQUESTS };
@@ -3458,6 +3473,7 @@ struct set_clipboard_info_request set_clipboard_info_request; struct open_token_request open_token_request; struct set_global_windows_request set_global_windows_request; + struct send_thread_signal_request send_thread_signal_request; }; union generic_reply { @@ -3640,8 +3656,9 @@ struct set_clipboard_info_reply set_clipboard_info_reply; struct open_token_reply open_token_reply; struct set_global_windows_reply set_global_windows_reply; + struct send_thread_signal_reply send_thread_signal_reply; };
-#define SERVER_PROTOCOL_VERSION 149 +#define SERVER_PROTOCOL_VERSION 151
#endif /* __WINE_WINE_SERVER_PROTOCOL_H */ Index: server/protocol.def =================================================================== RCS file: /home/wine/wine/server/protocol.def,v retrieving revision 1.110 diff -u -r1.110 protocol.def --- server/protocol.def 18 Aug 2004 00:04:58 -0000 1.110 +++ server/protocol.def 31 Oct 2004 19:00:39 -0000 @@ -2166,3 +2166,12 @@ #define SET_GLOBAL_SHELL_WINDOWS 0x01 /* set both main shell and listview windows */ #define SET_GLOBAL_PROGMAN_WINDOW 0x02 #define SET_GLOBAL_TASKMAN_WINDOW 0x04 + + +/* send a Unix signal to a specific thread */ +@REQ(send_thread_signal) + obj_handle_t handle; /* thread handle */ + int signal; /* the unix signal */ +@REPLY + int success; /* wether signaling succeeded */ +@END Index: server/request.h =================================================================== RCS file: /home/wine/wine/server/request.h,v retrieving revision 1.102 diff -u -r1.102 request.h --- server/request.h 20 Jul 2004 22:17:39 -0000 1.102 +++ server/request.h 31 Oct 2004 19:00:40 -0000 @@ -280,6 +280,7 @@ DECL_HANDLER(set_clipboard_info); DECL_HANDLER(open_token); DECL_HANDLER(set_global_windows); +DECL_HANDLER(send_thread_signal);
#ifdef WANT_REQUEST_HANDLERS
@@ -463,6 +464,7 @@ (req_handler)req_set_clipboard_info, (req_handler)req_open_token, (req_handler)req_set_global_windows, + (req_handler)req_send_thread_signal, }; #endif /* WANT_REQUEST_HANDLERS */
Index: server/thread.c =================================================================== RCS file: /home/wine/wine/server/thread.c,v retrieving revision 1.103 diff -u -r1.103 thread.c --- server/thread.c 27 Oct 2003 22:10:22 -0000 1.103 +++ server/thread.c 31 Oct 2004 19:00:41 -0000 @@ -1022,3 +1022,15 @@ release_object( thread ); } } + +/* send a Unix signal to a specific thread */ +DECL_HANDLER(send_thread_signal) +{ + struct thread *thread; + + if ((thread = get_thread_from_handle( req->handle, THREAD_SUSPEND_RESUME ))) + { + reply->success = send_thread_signal( thread, req->signal ); + release_object( thread ); + } +} Index: server/trace.c =================================================================== RCS file: /home/wine/wine/server/trace.c,v retrieving revision 1.214 diff -u -r1.214 trace.c --- server/trace.c 22 Sep 2004 02:46:38 -0000 1.214 +++ server/trace.c 31 Oct 2004 19:00:45 -0000 @@ -2548,6 +2548,17 @@ fprintf( stderr, " old_taskman_window=%p", req->old_taskman_window ); }
+static void dump_send_thread_signal_request( const struct send_thread_signal_request *req ) +{ + fprintf( stderr, " handle=%p,", req->handle ); + fprintf( stderr, " signal=%d", req->signal ); +} + +static void dump_send_thread_signal_reply( const struct send_thread_signal_reply *req ) +{ + fprintf( stderr, " success=%d", req->success ); +} + static const dump_func req_dumpers[REQ_NB_REQUESTS] = { (dump_func)dump_new_process_request, (dump_func)dump_get_new_process_info_request, @@ -2726,6 +2737,7 @@ (dump_func)dump_set_clipboard_info_request, (dump_func)dump_open_token_request, (dump_func)dump_set_global_windows_request, + (dump_func)dump_send_thread_signal_request, };
static const dump_func reply_dumpers[REQ_NB_REQUESTS] = { @@ -2906,6 +2918,7 @@ (dump_func)dump_set_clipboard_info_reply, (dump_func)dump_open_token_reply, (dump_func)dump_set_global_windows_reply, + (dump_func)dump_send_thread_signal_reply, };
static const char * const req_names[REQ_NB_REQUESTS] = { @@ -3086,6 +3099,7 @@ "set_clipboard_info", "open_token", "set_global_windows", + "send_thread_signal", };
/* ### make_requests end ### */
-
Alexandre Julliard -
Jukka Heinonen -
Markus Amsler -
Mike Hearn