Tommy's APC version of his VirtualAllocEx / CreateRemoteThread patch seems to be safe (since APCs only run a points where threads in well-written programs are not holding locks), is complete enough to make many apps happy, and is probably the best that can be done without a service thread or a real kernel change. I've encouraged him to submit it for real. Let's get this puppy committed!
"Dan Kegel" dank@kegel.com writes:
Tommy's APC version of his VirtualAllocEx / CreateRemoteThread patch seems to be safe (since APCs only run a points where threads in well-written programs are not holding locks),
Unfortunately there's no such guarantee, kernel APCs will run on every wait. Unless you meant user APCs which only run on alertable waits, but this means they would essentially never run. Like so much of the Windows API, APCs are a feature that could have been useful if it had been properly thought out...
On 8/4/06, Alexandre Julliard julliard@winehq.org wrote:
Tommy's APC version of his VirtualAllocEx / CreateRemoteThread patch seems to be safe (since APCs only run a points where threads in well-written programs are not holding locks),
Unfortunately there's no such guarantee, kernel APCs will run on every wait. Unless you meant user APCs which only run on alertable waits,
Whoops. I meant user APCs, and I thought Tommy was using user APCs, but I see now he's using system APCs. Drats.
but this means they would essentially never run.
If it's true that no windows app enters an alertable wait condition normally, then I guess APCs (without a service thread) are out.
How about a custom windows message? ALL gui programs have a message pump, so that has a better chance of actually getting through. - Dan
On 8/4/06, Alexandre Julliard julliard@winehq.org wrote:
"Dan Kegel" dank@kegel.com writes:
Tommy's APC version of his VirtualAllocEx / CreateRemoteThread patch seems to be safe (since APCs only run a points where threads in well-written programs are not holding locks),
Unfortunately there's no such guarantee, kernel APCs will run on every wait.
I'm not too clear why this is a problem. I thought that kernel APCs run on just interruptible waits, which excludes the wait on suspend and perhaps others.
Also, I was under the impression that only locks taken in the code path of the VM and thread creation functions had to be respected. If necessary, non-interruptible waits could/should be done while holding these locks (although I looked through the code and it doesn't look like any applicable code does a wait while holding a lock).
As a side note, there was a bug in my APC implementation--the process APC queue was flushed on every thread cleanup.
Tommy
On 8/4/06, Thomas Kho tkho@ucla.edu wrote:
Tommy's APC version of his VirtualAllocEx / CreateRemoteThread patch seems to be safe (since APCs only run a points where threads in well-written programs are not holding locks),
Unfortunately there's no such guarantee, kernel APCs will run on every wait.
I'm not too clear why this is a problem.
The closest Alexandre has come to answering this is http://www.mail-archive.com/wine-devel@winehq.org/msg29265.html but with APCs we at least handle one of his two objections (an APC definitely doesn't interrupt any critical section initialization). leaving "One problem is that many locks have to be acquired in a specific order to avoid deadlocks, and since you don't know which locks the thread is already holding you can't guarantee the order"
So all you have to do is identify all the locks that your APCs might need to acquire, and verify that they are always acquired in the same order by all possible code paths. (Or did I miss something, Alexandre?)
A torture test program running on a multi-CPU box might give some warm fuzzies, too, especially if you artificially increase the duration that locks are held by inserting busywaits. (That will increase the chance of threads colliding.) - Dan
"Dan Kegel" dank@kegel.com writes:
So all you have to do is identify all the locks that your APCs might need to acquire, and verify that they are always acquired in the same order by all possible code paths. (Or did I miss something, Alexandre?)
Well, you are right that running APCs only on waits is a lot safer than interrupting the thread anywhere. Unfortunately, interrupting the thread is really what you want. In particular remote operations on suspended threads have to be supported, since manipulating a process from the debugger is probably the number one reason for this feature.
Still, doing that stuff in APCs is a step in the right direction, you just need to make sure you can safely run these APCs from the SIGUSR1 handler.
On 8/5/06, Alexandre Julliard julliard@winehq.org wrote:
Still, doing that stuff in APCs is a step in the right direction, you just need to make sure you can safely run these APCs from the SIGUSR1 handler.
Do we have to verify that now, or can that wait until we want to add support for debuggers? - Dan
Dan Kegel wrote:
On 8/5/06, Alexandre Julliard julliard@winehq.org wrote:
Still, doing that stuff in APCs is a step in the right direction, you just need to make sure you can safely run these APCs from the SIGUSR1 handler.
Do we have to verify that now, or can that wait until we want to add support for debuggers?
Most of the *serious* debuggers I know of make use of remote virtual functions. This includes WineDbg, WinDbg (the MS one)... - WinDbg (the MS debugger) - WineDbg - gdb (when compiled on Windows) So that can be rather easily tested.
A+
On 8/5/06, Eric Pouech eric.pouech@wanadoo.fr wrote:
Still, doing that stuff in APCs is a step in the right direction, you just need to make sure you can safely run these APCs from the SIGUSR1 handler.
Do we have to verify that now, or can that wait until we want to add support for debuggers?
Most of the *serious* debuggers I know of make use of remote virtual functions. This includes WineDbg, WinDbg (the MS one)...
- WinDbg (the MS debugger)
Installer doesn't work: http://bugs.winehq.org/show_bug.cgi?id=5866 (not a showstopper, but does make it harder to test with)
- WineDbg
Where? I just looked: dank@lappy:~/wine-git/programs/winedbg$ grep CreateRemote *.c dank@lappy:~/wine-git/programs/winedbg$ grep VirtualAlloc *.c dank@lappy:~/wine-git/programs/winedbg$
- gdb (when compiled on Windows)
I just peeked, and vanilla gdb-6.4 doesn't contain the string CreateRemoteThread. Some old version of 'gdb-next' did, though, but the only mention of it I can find is here: http://darwinsource.opendarwin.org/DevToolsDec2001/gdb-203/src/gdb-next/winp...
Got a place one can grab working sources + binary for a gdb that does use CreateRemoteThread? - Dan
On 8/5/06, Dan Kegel dank@kegel.com wrote:
On 8/5/06, Eric Pouech eric.pouech@wanadoo.fr wrote:
Still, doing that stuff in APCs is a step in the right direction, you just need to make sure you can safely run these APCs from the SIGUSR1 handler.
Do we have to verify that now, or can that wait until we want to add support for debuggers?
Most of the *serious* debuggers I know of make use of remote virtual functions. This includes WineDbg, WinDbg (the MS one)...
- WinDbg (the MS debugger)
Installer doesn't work: http://bugs.winehq.org/show_bug.cgi?id=5866 (not a showstopper, but does make it harder to test with)
This is fixed by "msi: Fix the compressed files logic"
http://winehq.org/pipermail/wine-patches/2006-August/029358.html
but it hasn't been committed yet.
Dan Kegel wrote:
On 8/5/06, Eric Pouech eric.pouech@wanadoo.fr wrote:
Still, doing that stuff in APCs is a step in the right direction, you just need to make sure you can safely run these APCs from the SIGUSR1 handler.
Do we have to verify that now, or can that wait until we want to add support for debuggers?
Most of the *serious* debuggers I know of make use of remote virtual functions. This includes WineDbg, WinDbg (the MS one)...
- WinDbg (the MS debugger)
Installer doesn't work: http://bugs.winehq.org/show_bug.cgi?id=5866 (not a showstopper, but does make it harder to test with)
- WineDbg
Where? I just looked: dank@lappy:~/wine-git/programs/winedbg$ grep CreateRemote *.c dank@lappy:~/wine-git/programs/winedbg$ grep VirtualAlloc *.c dank@lappy:~/wine-git/programs/winedbg$
- gdb (when compiled on Windows)
I just peeked, and vanilla gdb-6.4 doesn't contain the string CreateRemoteThread. Some old version of 'gdb-next' did, though, but the only mention of it I can find is here: http://darwinsource.opendarwin.org/DevToolsDec2001/gdb-203/src/gdb-next/winp...
Got a place one can grab working sources + binary for a gdb that does use CreateRemoteThread?
- Dan
I was talking (at least) about VirtualQueryEx, which should be also implemented. All the debuggers use it for memory inspection. Some of the debuggers use VirtualAllocEx for some nice features (like calling a function in the debuggee), and they use the created space to push some values to be used for those specific functions (and also inject code the same way).
A+
On 8/5/06, Eric Pouech eric.pouech@wanadoo.fr wrote:
I was talking (at least) about VirtualQueryEx, which should be also implemented. All the debuggers use it for memory inspection.
FWIW, an implementation for Linux was posted at http://www.winehq.org/pipermail/wine-devel/2002-July/007482.html Perhaps some of that code could be reused. - Dan
Dan Kegel <dank <at> kegel.com> writes:
On 8/5/06, Eric Pouech <eric.pouech <at> wanadoo.fr> wrote:
I was talking (at least) about VirtualQueryEx, which should be also implemented. All the debuggers use it for memory inspection.
FWIW, an implementation for Linux was posted at http://www.winehq.org/pipermail/wine-devel/2002-July/007482.html Perhaps some of that code could be reused.
- Dan
Eric's patch is rather old and even today works like a charme. Gets my beloved OllyDbg running ;)
Dan Kegel wrote:
On 8/5/06, Eric Pouech eric.pouech@wanadoo.fr wrote:
I was talking (at least) about VirtualQueryEx, which should be also implemented. All the debuggers use it for memory inspection.
FWIW, an implementation for Linux was posted at http://www.winehq.org/pipermail/wine-devel/2002-July/007482.html Perhaps some of that code could be reused.
I sure know of the patch, as I wrote it ;-)
the issues with it are: - the exact information we get via the remote interface is not the same as the program itself would get when using the local VirtualQuery function. This includes both the detailed flags from a memory block, but also the memory block mapped for a unix .so file (the remote ones gives the information, while the local doesn't) - portability (one works for Linux)
A+
On Sat, 05 Aug 2006 10:32:15 +0200, Alexandre Julliard wrote:
Still, doing that stuff in APCs is a step in the right direction, you just need to make sure you can safely run these APCs from the SIGUSR1 handler.
How is the thread to interrupt to be selected? I really am not seeing what's wrong with a service thread here, it seems the safest and simplest way forward. We can easily put it into stealth mode by disabling the attach/detach notifications.
thanks -mike
-
Alexandre Julliard -
Dan Kegel -
Eric Pouech -
James Hawkins -
Mike Hearn -
Stefan Siebert -
Thomas Kho