Adam Gundy <arg(a)cyberscience.com> writes:
a new version of the valgrind patch has been uploaded to sourceforge - the only change is a small fix to the signal handling which should prevent "signal handler frame" uninitialized errors.
http://sourceforge.net/tracker/index.php?func=detail&aid=710006&group_id=462...
comments? bug reports? success stories?
I played a bit with it, it works pretty well, and I have already fixed a few bugs found with it. Great work! I have also applied those of your fixes that seemed correct, the rest will need a bit more work The signal stuff in particular is not going to work properly with NPTL, it would be really better if we could somehow make valgrind use real threads. I have no idea if this is possible though. -- Alexandre Julliard julliard(a)winehq.com
At 12:17 01/04/03 -0800, you wrote:
Adam Gundy <arg(a)cyberscience.com> writes:
a new version of the valgrind patch has been uploaded to sourceforge - the only change is a small fix to the signal handling which should prevent "signal handler frame" uninitialized errors.
http://sourceforge.net/tracker/index.php?func=detail&aid=710006&group_id=462...
comments? bug reports? success stories?
I played a bit with it, it works pretty well, and I have already fixed a few bugs found with it. Great work!
I have also applied those of your fixes that seemed correct, the rest will need a bit more work The signal stuff in particular is not going to work properly with NPTL, it would be really better if we could somehow make valgrind use real threads. I have no idea if this is possible though.
which part of the signal stuff won't work? all that has happened is that the "kill( <pid>, SIGUSR1 )" has moved from the wineserver to the client (for 'local' threads). I thought that the NPTL stuff still wanted a PID-per-thread model just so it can do this? OTOH if you can *really* switch to a pure pthreads model, that should be no problem since valgrind comes with a pthread library... as to making valgrind use real threads, I think that is a BIG job, and unlikely to happen, since the only thing that wants 'real' threads is WINE - everything else is happy with pthreads. Seeya, Adam -- Real Programmers don't comment their code. If it was hard to write, it should be hard to read, and even harder to modify. These are all my own opinions.
Adam Gundy <arg(a)cyberscience.com> writes:
which part of the signal stuff won't work? all that has happened is that the "kill( <pid>, SIGUSR1 )" has moved from the wineserver to the client (for 'local' threads). I thought that the NPTL stuff still wanted a PID-per-thread model just so it can do this?
No you can't use kill, you have to use pthread_kill, but then it will break the non NPTL case. And on Solaris you have to do some lwp stuff instead. Basically there is quite a bit of complexity that the server takes care of that you would have to replicate in the client.
OTOH if you can *really* switch to a pure pthreads model, that should be no problem since valgrind comes with a pthread library...
Pure pthreads is not possible, we need to support inter-process signals and things like that.
as to making valgrind use real threads, I think that is a BIG job, and unlikely to happen, since the only thing that wants 'real' threads is WINE - everything else is happy with pthreads.
Well, sending a signal to a thread from another process is standard Linux functionality, so I'd argue that valgrind should support it one way or another. -- Alexandre Julliard julliard(a)winehq.com
At 09:27 02/04/03 -0800, Alexandre Julliard wrote:
Adam Gundy <arg(a)cyberscience.com> writes:
which part of the signal stuff won't work? all that has happened is that the "kill( <pid>, SIGUSR1 )" has moved from the wineserver to the client (for 'local' threads). I thought that the NPTL stuff still wanted a PID-per-thread model just so it can do this?
No you can't use kill, you have to use pthread_kill, but then it will break the non NPTL case. And on Solaris you have to do some lwp stuff instead. Basically there is quite a bit of complexity that the server takes care of that you would have to replicate in the client.
Solaris isn't an issue here - valgrind only works on linux/x86. I haven't seen the WINE NPTL code, but... is there any particular reason that you have to use pthread_kill()? further down in your response you said:
Well, sending a signal to a thread from another process is standard Linux functionality, so I'd argue that valgrind should support it one way or another.
which surely implies that it is OK to use kill() instead of pthread_kill() as a mechanism for sending signals to a thread... I assume that under NPTL pthread_kill() is just a thin wrapper for kill() anyway?
as to making valgrind use real threads, I think that is a BIG job, and unlikely to happen, since the only thing that wants 'real' threads is WINE - everything else is happy with pthreads.
Well, sending a signal to a thread from another process is standard Linux functionality, so I'd argue that valgrind should support it one way or another.
true, if by 'thread' you mean 'cloned thread'. Changing valgrind to _really_ support the clone() system call is a huge job... if it can even be done. Seeya, Adam -- Real Programmers don't comment their code. If it was hard to write, it should be hard to read, and even harder to modify. These are all my own opinions.
Adam Gundy <arg(a)cyberscience.com> writes:
Solaris isn't an issue here - valgrind only works on linux/x86.
Of course Solaris is an issue, unless you somehow make your patch take effect only when running under valgrind.
Well, sending a signal to a thread from another process is standard Linux functionality, so I'd argue that valgrind should support it one way or another.
which surely implies that it is OK to use kill() instead of pthread_kill() as a mechanism for sending signals to a thread... I assume that under NPTL pthread_kill() is just a thin wrapper for kill() anyway?
No, it's a wrapper for tkill(), which is the system call to send a signal to a thread. kill() sends it to the whole process instead. -- Alexandre Julliard julliard(a)winehq.com
At 08:08 03/04/03 -0800, Alexandre Julliard wrote:
Adam Gundy <arg(a)cyberscience.com> writes:
Solaris isn't an issue here - valgrind only works on linux/x86.
Of course Solaris is an issue, unless you somehow make your patch take effect only when running under valgrind.
currently the patched wineserver returns a PID to signal when the suspended thread is in the 'local' client process (ie the calling thread is in the same process as the thread to be suspended). If the thread is 'remote' then it just returns 0 (or -1 I forget) and sends the signal itself. There is no reason that it can't always return -1 on Solaris...
Well, sending a signal to a thread from another process is standard Linux functionality, so I'd argue that valgrind should support it one way or another.
which surely implies that it is OK to use kill() instead of pthread_kill() as a mechanism for sending signals to a thread... I assume that under NPTL pthread_kill() is just a thin wrapper for kill() anyway?
No, it's a wrapper for tkill(), which is the system call to send a signal to a thread. kill() sends it to the whole process instead.
fair enough - we can still do that in the client if asked to by the wineserver, there is a (tiny) amount of code duplication here - but not enough to worry about. if we are building with NPTL, then presumably WINE will be linked -lpthread, and be calling pthread_create() etc. valgrind will replace the pthread library with its own version - so long as we call pthread_kill() in the client, everything will work. the whole problem is that the wineserver wants to have unique PIDs for each thread so that it can send signals to them, and attach to them for debugging. my patch for the wineserver means that the wineserver doesn't want to send signals for 'normal' operation within a process - when suspending a local thread we defer the thread suspend (currently sending SIGUSR1) back to the client. killing a local thread could work the same way - it doesn't at the moment, but if you are killing a thread your code is badly broken anyway. the only remaining reason for the wineserver to want unique PIDs is ptrace() - but we can't debug a process running under valgrind anyway! Seeya, Adam -- Real Programmers don't comment their code. If it was hard to write, it should be hard to read, and even harder to modify. These are all my own opinions.
Adam Gundy <arg(a)cyberscience.com> writes:
the whole problem is that the wineserver wants to have unique PIDs for each thread so that it can send signals to them, and attach to them for debugging. my patch for the wineserver means that the wineserver doesn't want to send signals for 'normal' operation within a process - when suspending a local thread we defer the thread suspend (currently sending SIGUSR1) back to the client.
Apart from the duplication of the signal mechanism, it also introduces races, since we depend on the client to do part of the suspend code. It means the server state will not necessarily match the actual state of the client thread, which could cause trouble. I'm still not convinced we want that in the standard tree.
killing a local thread could work the same way - it doesn't at the moment, but if you are killing a thread your code is badly broken anyway.
Many Windows apps do that. -- Alexandre Julliard julliard(a)winehq.com
killing a local thread could work the same way - it doesn't at the moment, but if you are killing a thread your code is badly broken anyway.
Many Windows apps do that. why would we want to be broken ??
===== Sylvain Petreolle (spetreolle at users dot sourceforge dot net) ICQ #170597259 No more War ! "What if tomorrow the War could be over ?" Morpheus, in "Reloaded". For the Law of Oil and Fire, Im an European that lives in France. For all my Brothers and friends, Im a human living on Earth. ___________________________________________________________ Do You Yahoo!? -- Une adresse @yahoo.fr gratuite et en français ! Yahoo! Mail : http://fr.mail.yahoo.com
Apart from the duplication of the signal mechanism, it also introduces races, since we depend on the client to do part of the suspend code. It means the server state will not necessarily match the actual state of the client thread, which could cause trouble. I'm still not convinced we want that in the standard tree. looking at VG MLs, it seems that there's already some ongoing work (http://www.goop.org/~jeremy/valgrind/ get to #77) which would provide a better signal handling, so this might be a way to get through that (I didn't test it though)
A+ -- Eric Pouech
At 21:44 03/04/03 +0200, Eric Pouech wrote:
Apart from the duplication of the signal mechanism, it also introduces races, since we depend on the client to do part of the suspend code. It means the server state will not necessarily match the actual state of the client thread, which could cause trouble. I'm still not convinced we want that in the standard tree. looking at VG MLs, it seems that there's already some ongoing work (http://www.goop.org/~jeremy/valgrind/ get to #77) which would provide a better signal handling, so this might be a way to get through that (I didn't test it though)
this patch is providing SIGINFO support and sigcontext availability in a signal handler. I have independantly implemented the sigcontext functionality needed by WINE; we have been discussing using this patch instead on the valgrind list... it doesn't help with this race condition (which I'm pretty convinced already exists) Seeya, Adam -- Real Programmers don't comment their code. If it was hard to write, it should be hard to read, and even harder to modify. These are all my own opinions.
participants (4)
-
Adam Gundy -
Alexandre Julliard -
Eric Pouech -
Sylvain Petreolle