On 16.02.2016 10:43, Henri Verbeet wrote:
On 12 February 2016 at 22:40, Sebastian Lackner sebastian@fds-team.de wrote:
Thanks for working on this. I assume you already saw the new test failures, those should be fixed of course. Besides that I have some more remarks on the patch:
Thank you for trying to review patches; doing it well takes real effort. As you noted yourself in https://www.winehq.org/pipermail/wine-devel/2016-February/111682.html, the icebp and and fpu exception failures are existing failures.
Sorry, I didn't recognize them as the usual failures because they were renamed by the patch. That doesn't invalidate the other remarks of my review though.
Int 0x2d is not just a special way to raise a breakpoint, its actually a kernel interface to send commands to the debugger.
Yes, but that statement is somewhat misleading. Most of the special behaviour of int $0x2d only happens when Windows is in kernel debug mode. Wine makes no attempt to implement kernel debug mode.
Actually my tests confirms that it works in user mode. The only blocker is wow64, which is not really surprising. Kernel mode seems to assume that valid values are stored in 64-bit registers, which is not the case, so a breakpoint exception is raised. Under normal circumstances calling a valid debug service (for example EAX=1 with valid arguments in other registers) raises no exception at all.
EAX contains the command value, other registers contain the arguments. An proper implementation is not necessary I guess, but at least for small values of EAX which are assigned to commands we would need better test coverage. Most anti- debugger tricks set EAX = 0 which is not tested at all so far.
win_context->Eip += 3;rec->ExceptionCode = EXCEPTION_BREAKPOINT;rec->ExceptionAddress = (char *)win_context->Eip;rec->NumberParameters = is_wow64 ? 1 : 3;rec->ExceptionInformation[0] = EAX_sig(context);rec->ExceptionInformation[1] = ECX_sig(context);rec->ExceptionInformation[2] = EDX_sig(context);break;} rec->ExceptionCode = EXCEPTION_ACCESS_VIOLATION; rec->NumberParameters = 2; rec->ExceptionInformation[0] = 0;diff --git a/dlls/ntdll/tests/exception.c b/dlls/ntdll/tests/exception.c index d6a610e..57b201a 100644 --- a/dlls/ntdll/tests/exception.c +++ b/dlls/ntdll/tests/exception.c @@ -219,6 +219,11 @@ static const struct exception
{ { 0xf1, 0x90, 0xc3 }, /* icebp; nop; ret */ 1, 1, FALSE, STATUS_SINGLE_STEP, 0 },
- { { 0xb8, 0xb8, 0xb8, 0xb8, 0xb8, /* mov $0xb8b8b8b8, %eax */
0xb9, 0xb9, 0xb9, 0xb9, 0xb9, /* mov $0xb9b9b9b9, %ecx */0xba, 0xba, 0xba, 0xba, 0xba, /* mov $0xbabababa, %edx */0xcd, 0x2d, 0xc3 }, /* int $0x2d; ret */17, 0, FALSE, STATUS_BREAKPOINT, 3, { 0xb8b8b8b8, 0xb9b9b9b9, 0xbabababa } },};
Could you maybe also provide a testcase when a debugger is attached? I would expect that it behaves different then. Actually it should not even produce an exception then because the exception is transparently translated into a debug event, like wine already does for debug strings for example.
I did of course test the behaviour with a debugger attached, although mostly to validate the %eip and exception address adjustments. The behaviour is consistent with what's in this patch. Different values of %eax (e.g. 0-5) don't seem to have any obvious effect outside of kernel debug mode.
On some systems it makes a difference, and causes a different ExceptionAddress value when a debugger is attached. However, the behaviour is not really consistent.
- ok( rec->ExceptionAddress == (char*)context->Eip ||
(rec->ExceptionCode == STATUS_BREAKPOINT && rec->ExceptionAddress == (char*)context->Eip + 1),I would prefer an exact test for ExceptionAddress here, off-by-one errors in exception handling would be kinda odd.
Well, yes, but as you can see from the test results, the behaviour is not consistent between different Windows versions.