December 2004 - Wine-Devel - list.winehq.org

Re: ptrace single-stepping change breaks Wine

by Jesse Allen

On Fri, 31 Dec 2004 09:30:04 -0800 (PST), Linus Torvalds <torvalds(a)osdl.org> wrote: > > > On Fri, 31 Dec 2004, Davide Libenzi wrote: > > > > I don't think that the Wine problem resolution is due to the POPF > > instruction handling. Basically Linus patch does a nice cleanup plus POPF > > handling, so maybe the patch can be split. > > The popf part is very nice in that it allows you to single-step and debug > this thing. > > The fact is, I can't debug Wine. The code-base is just too alien for me, > so to debug this thing I needed to come up with a silly example of TF > usage, and try to debug _that_ instead as if it were something unknown (ie > debugging by knowing what the program does is a no-no, since that would > have defeated the whole exercise). > > And handlign "popf" correctly really was the only sane way to debug it, > anything else would never have worked in a real-life debugging situation. > It's easy enough to say "well, just do it by hand", but that's not > practical when you debug with "si 1000" to try to pinpoint the behaviour a > bit. > > And clearly my debuggability exercise seem to have worked, since the end > result apparently ended up doing the right thing for Wine. I honestly think there may have been no other way. There was a reason why I was very vague at first. I could not pin-point an exact location of failure. Just grepping a good log shows over 500,000 calls to RaiseException. trap_handler ran over 300,000 times and as many TF clears. My brother told me to set watches or break points to see if I could find something wrong, but I simply told him there is too much going on. The only thing I could think that can be done is to mentally find debugging scenarios and hope to find where it could be breaking. But the only people that could have thought of a scenario are people that know linux well enough what the kernel is doing in the first place. > > This is why debuggability is important. I realize that people may think > I'm inconsistent (since I abhor kernel debuggers), but while _I_ abhor > debuggers, I also think that the primary objective of an operating system > is to make easy things easy, and hard things possible, so while I don't > much like debuggers, I consider it a fundamental failure if the kernel > doesn't have proper support for them. I think that copy protection routines are particually nasty. They purposely make debugging hard (again see above, no sane program would be like that). And the program's reason for failing is not the reason at all -- "please insert disc" -- the disc is already in there! So they don't say the real reason why it failed, leaving the user totally hopelessly lost on what they should do. It's really hard to file a bug report on that alone! Had I not placed my guess on ptrace early on, this problem may have gone undiscovered for a long time. I have checked transgaming and they seem to be not aware about this, but it would have been a rude awakening for them when they find that when most distros had updated to 2.6.9, that all the SecuRom protected games silently broke, and they would have had a heck of a time debugging them to find the reason, I'm sure, even with the specs on it! Though who knows if cedega is affected because their code-base is diverging, and I'm sure their copy protection support is very different. > > So I think it's worth splitting out the "popf" part of the patch, but even > if that one doesn't actually matter for Warcraft, I'd put it in just so > that we have a state where we're _supposed_ to be able to debug things > with TF in them. Just having the mental expectation that things like that > should work is important - otherwise we'll eventually end up having some > other subtle problem with TF handling. > > Linus > > Sure. I've tried the game and it doesn't require is_at_popf(). I thought it did because I thought it was required for handling TF correctly, but maybe this is another scenario. I think it's a good idea to keep it in too. I actually don't care about debugging the copy protection, I just want correct behavoir. If doing all this has pointed out these other issues too, then it's all well worth it, because next time it will be /easier/ and we won't have subtle problems. Jesse

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Linus Torvalds

On Fri, 31 Dec 2004, Davide Libenzi wrote: > > I don't think that the Wine problem resolution is due to the POPF > instruction handling. Basically Linus patch does a nice cleanup plus POPF > handling, so maybe the patch can be split. The popf part is very nice in that it allows you to single-step and debug this thing. The fact is, I can't debug Wine. The code-base is just too alien for me, so to debug this thing I needed to come up with a silly example of TF usage, and try to debug _that_ instead as if it were something unknown (ie debugging by knowing what the program does is a no-no, since that would have defeated the whole exercise). And handlign "popf" correctly really was the only sane way to debug it, anything else would never have worked in a real-life debugging situation. It's easy enough to say "well, just do it by hand", but that's not practical when you debug with "si 1000" to try to pinpoint the behaviour a bit. And clearly my debuggability exercise seem to have worked, since the end result apparently ended up doing the right thing for Wine. This is why debuggability is important. I realize that people may think I'm inconsistent (since I abhor kernel debuggers), but while _I_ abhor debuggers, I also think that the primary objective of an operating system is to make easy things easy, and hard things possible, so while I don't much like debuggers, I consider it a fundamental failure if the kernel doesn't have proper support for them. So I think it's worth splitting out the "popf" part of the patch, but even if that one doesn't actually matter for Warcraft, I'd put it in just so that we have a state where we're _supposed_ to be able to debug things with TF in them. Just having the mental expectation that things like that should work is important - otherwise we'll eventually end up having some other subtle problem with TF handling. Linus

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Linus Torvalds

On Fri, 31 Dec 2004, Daniel Jacobowitz wrote: > > Lots, I like it. The syscall trap will always be delivered before the > single-step trap, right, because signal delivery won't run until we > return to userspace? Yes. Although I've not actually tested it. Before, it used to show up as one event, and basically the "0x80" marker got lost, so effectively the "system call exit" part would have got lost. Now, it _may_ DTRT, with the caveat that the system call ptrace_notify() thing still has the same problem with restarting-with-a-signal. That's basically a "don't do that then", and is the status quo, of course, so this is at least not a regression. It's still pretty ugly, but apparently nobody really cares ;) Linus

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Linus Torvalds

On Fri, 31 Dec 2004, Andi Kleen wrote: > > > - you couldn't even debug signal handlers, because they were _really_ > > hard to get into unless you knew where they were and put a breakpoint > > on them. > > Ok I see this as being a problem. But I bet it could be fixed > much simpler without doing all this complicated and likely-to-be-buggy > popf parsing you added. And that is _exactly_ what we did. Guess what broke Wine? Uhhuh. Linus

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Thomas Sailer

On Freitag 31 Dezember 2004 14.31, Mike Hearn wrote: > What about this patch? This works now. Happy new year... Tom

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Davide Libenzi

On Fri, 31 Dec 2004, Andi Kleen wrote: > > - you couldn't even debug signal handlers, because they were _really_ > > hard to get into unless you knew where they were and put a breakpoint > > on them. > > Ok I see this as being a problem. But I bet it could be fixed > much simpler without doing all this complicated and likely-to-be-buggy > popf parsing you added. I don't think that the Wine problem resolution is due to the POPF instruction handling. Basically Linus patch does a nice cleanup plus POPF handling, so maybe the patch can be split. > > - you couldn't see the instruction after a system call. > > Are you sure? Yes, this was true with 2.4. Than it has been fixed some time ago. But handling that revealed a fragile handling of ptrace event delivery we had in do_syscall_trace(). Part of the Linus patch tries to solve the fact that on one side strace wants things to happen in a certain way, way that seem to break Wine. I think Linus cleanup of the ptrace event delivery can get strace, Wine and single-step-after-syscall right, w/out POPF handling. Then you guys can flame each other over the POPF handling ;) - Davide

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Daniel Jacobowitz

On Thu, Dec 30, 2004 at 09:47:42PM -0800, Linus Torvalds wrote: > So I looked at just sharing the code with the debug trap handler, and the > result is appended. strace works, as does all the TF tests I've thrown at > it, and the code actually looks better anyway (the old do_debug code looks > like it got the EIP wrong in VM86 mode, for example, this just cleans > that up too). Just use a common "send_sigtrap()" routine. > > Does this look saner? Lots, I like it. The syscall trap will always be delivered before the single-step trap, right, because signal delivery won't run until we return to userspace? -- Daniel Jacobowitz

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Mike Hearn

On Fri, 2004-12-31 at 14:13 +0100, Thomas Sailer wrote: > No this doesn't work. The decision which address space layout to use is done > in arch/i386/mm/mmap.c:arch_pick_mmap_layout, and this function is called by > the elf loader in fs/binfmt_elf.c:load_elf_binary, i.e. the decision which > address space layout to use for the current wine process is already done long > time before your personality syscall takes effect. > > I hoped there was some ELF section magic to turn this off (like execshield), > but there doesn't seem to be. So it has to be done before an exec then? I had assumed changing the personality would affect all mmaps from that point onwards, too bad. We do some execs as part of the Wine startup code so it shouldn't be too much of an issue. Anyway, all the setarch program does is do this syscall then exec the program so it shouldn't be too hard to do the equivalent in Wine. What about this patch? --- libs/wine/config.c (revision 14) +++ libs/wine/config.c (local) @@ -35,6 +35,10 @@ #endif #include "wine/library.h" +#ifdef HAVE_SYSCALL_H +#include <syscall.h> +#endif + static const char server_config_dir[] = "/.wine"; /* config dir relative to $HOME */ static const char server_root_prefix[] = "/tmp/.wine-"; /* prefix for server root dir */ static const char server_dir_prefix[] = "/server-"; /* prefix for server dir */ @@ -289,8 +293,13 @@ static void preloader_exec( char **argv, new_argv = xmalloc( (last_arg - argv + 2) * sizeof(*argv) ); memcpy( new_argv + 1, argv, (last_arg - argv + 1) * sizeof(*argv) ); new_argv[0] = full_name; + + /* set the abi personality */ + syscall(136, 0x8 /* PER_LINUX32 */ | 0x0200000 /* ADDR_COMPAT_LAYOUT */); + if (envp) execve( full_name, new_argv, envp ); else execv( full_name, new_argv ); + free( new_argv ); free( full_name ); return;

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Thomas Sailer

On Donnerstag 30 Dezember 2004 14.06, you wrote: > Tom, does this patch against Wine help? It should do the same thing as > the setarch program, so if that fixes it then this should also (if I've > understood how this mechanism works of course). No this doesn't work. The decision which address space layout to use is done in arch/i386/mm/mmap.c:arch_pick_mmap_layout, and this function is called by the elf loader in fs/binfmt_elf.c:load_elf_binary, i.e. the decision which address space layout to use for the current wine process is already done long time before your personality syscall takes effect. I hoped there was some ELF section magic to turn this off (like execshield), but there doesn't seem to be. Tom

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Andi Kleen

On Thu, Dec 30, 2004 at 04:38:21PM -0800, Linus Torvalds wrote: > > Can someone repeat again what was wrong with the old ptrace > > semantics before the initial change that caused all these complex > > changes? It seemed to work well for years. How about we just > > go back to the old state, revert all the recent ptrace changes > > and skip all that? > > Let me count the ways that were wrong before the changes: > - you couldn't debug any code that set TF. Really. ptrace would totally > destroy the TF state in the controlled process, so it would do > something totally different when debugged. Well, tough. I assume people who play with TF themselves know how to handle it without debuggers. Really adding instruction parsing for such a corner case seems like extreme overkill to me. I agree it is not nice, but is it really worth all that complexity to hide it? > - you couldn't even debug signal handlers, because they were _really_ > hard to get into unless you knew where they were and put a breakpoint > on them. Ok I see this as being a problem. But I bet it could be fixed much simpler without doing all this complicated and likely-to-be-buggy popf parsing you added. > - you couldn't see the instruction after a system call. Are you sure? > - ptrace returned bogus TF state after a single-step I am sure all debuggers in existence deal with that (and they will need to continue doing so because there are so many old kernels around) > descriptors, and we actually do that (badly) in another place: the AMD > "prefetch" check does exactly the same thing except it seems to get a few > details wrong (looks like it cannot handle 16-bit code), and only works > for the current process. Yes, it was intentional to simplify it. 16bit code is not supposed to use prefetches (and even if they do the probability of faulting is pretty small) Also we needed several iterations to get all the subtle bugs out (and I bet there are some issues left) -Andi

Dec. 31, 2004