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.
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.
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.
- 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.