[PATCH v2 0/1] MR4720: Implement test_continue() for amd64
Currently there is `NtContinue()` test only for `aarch64` so implement it for amd64 aswell. This implementation is very similar to `aarch64` and it's very basic test. It doesn't change/test `ContextFlags` so it won't catch https://bugs.winehq.org/show_bug.cgi?id=56050 but that can be implemented later on top of this. -- v2: ntdll/tests: Implement test_continue() for amd64 https://gitlab.winehq.org/wine/wine/-/merge_requests/4720
From: Dāvis Mosāns <davispuh(a)gmail.com> --- dlls/ntdll/tests/exception.c | 155 +++++++++++++++++++++++++++++++++++ 1 file changed, 155 insertions(+) diff --git a/dlls/ntdll/tests/exception.c b/dlls/ntdll/tests/exception.c index bac571b2733..095b3a7ba45 100644 --- a/dlls/ntdll/tests/exception.c +++ b/dlls/ntdll/tests/exception.c @@ -4413,6 +4413,160 @@ static void test_thread_context(void) #undef COMPARE } +static void test_continue(void) +{ + struct context_pair { + CONTEXT before; + CONTEXT after; + } contexts; + NTSTATUS (*func_ptr)( struct context_pair *, void *arg2, void *continue_func, void *capture_func ) = code_mem; + + static const BYTE call_func[] = + { + /* need to preserve these */ + 0x53, /* push rbx */ + 0x55, /* push rbp */ + 0x56, /* push rsi */ + 0x57, /* push rdi */ + 0x41, 0x54, /* push r12 */ + 0x41, 0x55, /* push r13 */ + 0x41, 0x56, /* push r14 */ + 0x41, 0x57, /* push r15 */ + + 0x48, 0x83, 0xec, 0x28, /* sub rsp, 8*5; stack space */ + 0x48, 0x89, 0x24, 0x24, /* mov [rsp+8*0], rsp; for validation */ + + /* save args */ + 0x48, 0x89, 0x4c, 0x24, 0x08, /* mov [rsp+8*1], rcx */ + 0x48, 0x89, 0x54, 0x24, 0x10, /* mov [rsp+8*2], rdx */ + 0x4c, 0x89, 0x44, 0x24, 0x18, /* mov [rsp+8*3], r8 */ + 0x4c, 0x89, 0x4c, 0x24, 0x20, /* mov [rsp+8*4], r9 */ + + /* invoke capture context */ + 0x41, 0xff, 0xd1, /* call r9 */ + + /* overwrite general registers */ + 0x48, 0xb8, 0xef, 0xbe, 0xad, 0xde, 0xef, 0xbe, 0xad, 0xde, /* mov rax, 0xdeadbeefdeadbeef */ + 0x48, 0x89, 0xc1, /* mov rcx, rax */ + 0x48, 0x89, 0xc2, /* mov rdx, rax */ + 0x48, 0x89, 0xc3, /* mov rbx, rax */ + 0x48, 0x89, 0xc5, /* mov rbp, rax */ + 0x48, 0x89, 0xc6, /* mov rsi, rax */ + 0x48, 0x89, 0xc7, /* mov rdi, rax */ + 0x49, 0x89, 0xc0, /* mov r8, rax */ + 0x49, 0x89, 0xc1, /* mov r9, rax */ + 0x49, 0x89, 0xc2, /* mov r10, rax */ + 0x49, 0x89, 0xc3, /* mov r11, rax */ + 0x49, 0x89, 0xc4, /* mov r12, rax */ + 0x49, 0x89, 0xc5, /* mov r13, rax */ + 0x49, 0x89, 0xc6, /* mov r14, rax */ + 0x49, 0x89, 0xc7, /* mov r15, rax */ + + /* overwrite SSE registers */ + 0x66, 0x48, 0x0f, 0x6e, 0xc0, /* movq xmm0, rax */ + 0x66, 0x0f, 0x6c, 0xc0, /* punpcklqdq xmm0,xmm0; extend to high quadword */ + 0x0f, 0x28, 0xc8, /* movaps xmm1, xmm0 */ + 0x0f, 0x28, 0xd0, /* movaps xmm2, xmm0 */ + 0x0f, 0x28, 0xd8, /* movaps xmm3, xmm0 */ + 0x0f, 0x28, 0xe0, /* movaps xmm4, xmm0 */ + 0x0f, 0x28, 0xe8, /* movaps xmm5, xmm0 */ + 0x0f, 0x28, 0xf0, /* movaps xmm6, xmm0 */ + 0x0f, 0x28, 0xf8, /* movaps xmm7, xmm0 */ + 0x44, 0x0f, 0x28, 0xc0, /* movaps xmm8, xmm0 */ + 0x44, 0x0f, 0x28, 0xc8, /* movaps xmm9, xmm0 */ + 0x44, 0x0f, 0x28, 0xd0, /* movaps xmm10, xmm0 */ + 0x44, 0x0f, 0x28, 0xd8, /* movaps xmm11, xmm0 */ + 0x44, 0x0f, 0x28, 0xe0, /* movaps xmm12, xmm0 */ + 0x44, 0x0f, 0x28, 0xe8, /* movaps xmm13, xmm0 */ + 0x44, 0x0f, 0x28, 0xf0, /* movaps xmm14, xmm0 */ + 0x44, 0x0f, 0x28, 0xf8, /* movaps xmm15, xmm0 */ + + /* FIXME: overwrite debug, x87 FPU and AVX registers to test those */ + + /* load args */ + 0x48, 0x8b, 0x4c, 0x24, 0x08, /* mov rcx, [rsp+8*1]; context */ + 0x48, 0x8b, 0x54, 0x24, 0x10, /* mov rdx, [rsp+8*2]; arg2 */ + 0x48, 0x83, 0xec, 0x70, /* sub rsp, 0x70; change stack */ + + /* setup context to return to label 1 */ + 0x48, 0x8d, 0x05, 0x1b, 0x00, 0x00, 0x00, /* lea rax, [rel 1] (0x1b) */ + 0x48, 0x89, 0x81, 0xf8, 0x00, 0x00, 0x00, /* mov [rcx + 0xf8 (context.Rip)], rax */ + + /* trash EFLAGS */ + 0x9c, /* pushfq */ + 0x58, /* pop rax */ + 0x48, 0x0d, 0x00, 0x45, 0x00, 0x00, /* or rax, 0x0100 + 0x4400; set trap flag so it's cleared. + FIXME: also clearing Direction Flag and Nested Task as those break Wine */ + + 0x48, 0xf7, 0xd0, /* not rax */ + 0x50, /* push rax */ + 0x9d, /* popfq */ + + /* invoke NtContinue... */ + 0xff, 0x94, 0x24, 0x88, 0x00, 0x00, 0x00, /* call [rsp+8*3+0x70] */ + + /* validate stack pointer */ + 0x48, 0x8b, 0x0c, 0x24, /* 1: mov rcx, [rsp] */ + 0x48, 0x39, 0xe1, /* cmp rcx, rsp */ + 0x74, 0x02, /* je 2; jump over ud2 */ + 0x0f, 0x0b, /* ud2; stack pointer invalid, let's crash */ + + /* invoke capture context */ + 0x48, 0x8b, 0x4c, 0x24, 0x08, /* mov rcx, [rsp+8*1]; context */ + 0x48, 0x81, 0xc1, 0xd0, 0x04, 0x00, 0x00, /* add rcx, 0x4d0; +sizeof(CONTEXT) to get context->after */ + 0xff, 0x54, 0x24, 0x20, /* call [rsp+8*4] */ + + /* free stack */ + 0x48, 0x83, 0xc4, 0x28, /* 2: add rsp, 8*5 */ + + /* restore back */ + 0x41, 0x5f, /* pop r15 */ + 0x41, 0x5e, /* pop r14 */ + 0x41, 0x5d, /* pop r13 */ + 0x41, 0x5c, /* pop r12 */ + 0x5f, /* pop rdi */ + 0x5e, /* pop rsi */ + 0x5d, /* pop rbp */ + 0x5b, /* pop rbx */ + 0xc3 /* ret */ + }; + + if (!pRtlCaptureContext) + { + win_skip("RtlCaptureContext is not available.\n"); + return; + } + + memcpy( func_ptr, call_func, sizeof(call_func) ); + FlushInstructionCache( GetCurrentProcess(), func_ptr, sizeof(call_func) ); + + func_ptr( &contexts, FALSE, NtContinue, pRtlCaptureContext ); + +#define COMPARE(reg) \ + ok( contexts.before.reg == contexts.after.reg, "wrong " #reg " %p/%p\n", (void *)(ULONG64)contexts.before.reg, (void *)(ULONG64)contexts.after.reg ) + + COMPARE( Rax ); + COMPARE( Rdx ); + COMPARE( Rbx ); + COMPARE( Rbp ); + COMPARE( Rsi ); + COMPARE( Rdi ); + COMPARE( R8 ); + COMPARE( R9 ); + COMPARE( R10 ); + COMPARE( R11 ); + COMPARE( R12 ); + COMPARE( R13 ); + COMPARE( R14 ); + COMPARE( R15 ); + + ok(memcmp(&contexts.before.Xmm0, &contexts.after.Xmm0, &contexts.before.Xmm15 - &contexts.before.Xmm0) == 0, + "wrong some SSE register, Xmm0 %lld/%lld Xmm15 %lld/%lld", contexts.before.Xmm0.Low, contexts.after.Xmm0.Low, + contexts.before.Xmm15.High, contexts.after.Xmm15.High); + +#undef COMPARE +} + static void test_wow64_context(void) { const char appname[] = "C:\\windows\\syswow64\\cmd.exe"; @@ -12456,6 +12610,7 @@ START_TEST(exception) test_debug_registers_wow64(); test_debug_service(1); test_simd_exceptions(); + test_continue(); test_virtual_unwind(); test___C_specific_handler(); test_restore_context(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4720
On Wed Dec 20 17:00:09 2023 +0000, Jinoh Kang wrote:
Hello there! Thanks for your contribution. Apropos commit "ntdll/tests: Move test_continue() to be used for all platforms": I think there is a better way to test `NtContinue()` across platforms. You're approach of sharing the `test_continue()` function header/footer works, but ends up branching on the architecture *twice*. ```c static void test_continue(void) { ... #ifdef __x86_64__ static const BYTE call_func[] = { ... }; #elif defined(__aarch64__) static const DWORD call_func[] = { ... }; #else ... #endif ... memcpy( func_ptr, call_func, sizeof(call_func) ); FlushInstructionCache( GetCurrentProcess(), func_ptr, sizeof(call_func) ); func_ptr( &contexts, FALSE, NtContinue, pRtlCaptureContext ); #ifdef __x86_64__ ... #elif defined(__aarch64__) ... #endif ... } ``` Notice that there is little code shared between architectures, other than `NtContinue` and `RtlCaptureContext`. Instead, I suggest that you define the entire `test_continue()` per each architecture: ```c #ifdef __x86_64__ ... static void test_thread_context(void) { ... #undef COMPARE } static void test_continue(void) { ... static const BYTE call_func[] = { ... }; ... memcpy( func_ptr, call_func, sizeof(call_func) ); FlushInstructionCache( GetCurrentProcess(), func_ptr, sizeof(call_func) ); func_ptr( &contexts, FALSE, NtContinue, pRtlCaptureContext ); ... } static void test_wow64_context(void) { ... } ... #elif defined(__arch64__) ... static void test_continue(void) { ... } ``` Even if this leads to code duplication, this allows us flexibility to extend `test_continue` with arch-specific logic in the future, and avoids interleaving of different test code for each architecture. I don't think that's better because essentially you want to test same functionality across all architectures but having them completely independent will cause tests to diverge.
Anyway I updated MR. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4720#note_56307
I don't think that's better
Everyone can have different opinions. I merely stated what matches the current design of the tests and thus what I belive to be most likely to be accepted. But yes, code duplication has its downsides. I'd argue that the current exception tests are a mess of duplicated test functions spread throughout the code. However, I don't believe the solution to this problem is to introduce *more* conditional compilation. Conditional compilation tends to increase complexity[^gazillo][^why-avoid], and makes it harder to ensure that the code compilers across all configurations. For example, x86-64 compiler cannot catch errors in code enabled for arm64 architecture, or vice versa. We have severely abused #ifdefs with irregularly sized sections of code, and I don't think we need more of that. If you still want it deduplicated, consider introducing abstraction or factoring out architecture-agnostic logic into own function *without* #if(def)s. However, take this advice with a caution: it can make tests less straightforward. [^gazillo]: Gazzillo, Paul; Wei, Shiyi. ["Conditional Compilation is Dead, Long Live Conditional Compilation!"](https://www.paulgazzillo.com/papers/icse19nier.pdf) ICSE-NIER '19: Proceedings of the 41st International Conference on Software Engineering: New Ideas and Emerging Results. [^why-avoid]: ["conditional compilation - Why should #ifdef be avoided in .c files?" - Stack Overflow](https://stackoverflow.com/questions/1851181/why-should-ifdef-be-avoided-in-c...)
because essentially you want to test same functionality across all architectures
Consistency is nice. However, it's not a strict requirement to "test the same functionality across all architectures." For example: RtlVirtualUnwind has different parameters across architectures, and doesn't even exist on i386 (32bit intel). You can't test what doesn't exist! In NtContinue()'s case, there are two parameters that the function accepts: 1. `context`: This has plenty of room for variation between architectures as well as existence of an emulation layer (e.g. wow64). They even have different set of context flags. Little is shared between architectures, so I think you can get far with code deduplication. 2. `alertable`: This is common on all architectures. It would be nice to define `test_NtContinue_alertable_param()` to test just this parameter if it exhibits the same behavior on all architectures. This MR is not interested in it, though.
but having them completely independent will cause tests to diverge.
This problem might not be as severe as you think. Also, note that tests are meant to run regularly; even if they diverge, each of them still has a consistent target to test against: Windows. Besides, even if NtContinue() is consistent across architectures, programs across architectures might not use it consistently, and we want our test cases to match real apps per each architecture if possible :'). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4720#note_56359
[...] consider introducing abstraction or factoring out architecture-agnostic logic into own function *without* #if(def)s.
I think that's probably better and quite readable, something like: ```c static void invoke_continue_test_func(TEST_CASE test_case, context_pair *contexts) { NTSTATUS (*func_ptr)( TEST_CASE test_case, struct context_pair *, void *arg2, void *continue_func, void *capture_func ) = code_mem; BYTE call_func[] = continue_test_func; /* arch specific */ memcpy( func_ptr, call_func, sizeof(call_func) ); FlushInstructionCache( GetCurrentProcess(), func_ptr, sizeof(call_func) ); if (test_case == TEST_CASE_ALERT) { func_ptr( test_case, &contexts, TRUE, NtContinue, pRtlCaptureContext ); } else { func_ptr( test_case, &contexts, FALSE, NtContinue, pRtlCaptureContext ); } } static void test_continue(void) { struct context_pair { CONTEXT before; CONTEXT after; } contexts; if (!pRtlCaptureContext) { win_skip("RtlCaptureContext is not available.\n"); return; } invoke_continue_test_func(TEST_CASE_0, &contexts); verify_continue_result(TEST_CASE_0, &contexts); invoke_continue_test_func(TEST_CASE_ALERT, &contexts); verify_continue_result(TEST_CASE_ALERT, &contexts); /* Test some CONTEXT flag magic, on other architectures this could be no-op */ invoke_continue_test_func(TEST_CASE_2, &contexts); verify_continue_result(TEST_CASE_2, &contexts); } ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4720#note_56384
On Fri Dec 22 18:49:57 2023 +0000, Dāvis Mosāns (davispuh) wrote:
[...] consider introducing abstraction or factoring out architecture-agnostic logic into own function *without* #if(def)s. I think that's probably better and quite readable, something like:
static void invoke_continue_test_func(TEST_CASE test_case, context_pair *contexts) { NTSTATUS (*func_ptr)( TEST_CASE test_case, struct context_pair *, void *arg2, void *continue_func, void *capture_func ) = code_mem; BYTE call_func[] = continue_test_func; /* arch specific */ memcpy( func_ptr, call_func, sizeof(call_func) ); FlushInstructionCache( GetCurrentProcess(), func_ptr, sizeof(call_func) ); if (test_case == TEST_CASE_ALERT) { func_ptr( test_case, &contexts, TRUE, NtContinue, pRtlCaptureContext ); } else { func_ptr( test_case, &contexts, FALSE, NtContinue, pRtlCaptureContext ); } } static void test_continue(void) { struct context_pair { CONTEXT before; CONTEXT after; } contexts; if (!pRtlCaptureContext) { win_skip("RtlCaptureContext is not available.\n"); return; } invoke_continue_test_func(TEST_CASE_0, &contexts); verify_continue_result(TEST_CASE_0, &contexts); invoke_continue_test_func(TEST_CASE_ALERT, &contexts); verify_continue_result(TEST_CASE_ALERT, &contexts); /* Test some CONTEXT flag magic, on other architectures this could be no-op */ invoke_continue_test_func(TEST_CASE_2, &contexts); verify_continue_result(TEST_CASE_2, &contexts); }In practice trying to make that sort of code generic adds more complexity than it's worth. Especially as soon as there's some platform-specific quirk that you want to test.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4720#note_56385
On Fri Dec 22 19:55:29 2023 +0000, Alexandre Julliard wrote:
In practice trying to make that sort of code generic adds more complexity than it's worth. Especially as soon as there's some platform-specific quirk that you want to test. What Julliard said. Your specific example already have *two* limitations:
1. It cannot handle variably-sized contexts (`CONTEXT_EX`). 2. It cannot handle 32-bit ARM: Windows uses Thumb mode, which requires you to sst the LSB of the function pointer to 1. (See `test_thread_context()` for ARM example). Once you generalize to cover these, you can see the code get quickly complicated. :/
If you still want it deduplicated, consider introducing abstraction or factoring out architecture-agnostic logic into own function *without* #if(def)s. **However, take this advice with a caution: it can make tests less straightforward.**
Notice the disclaimer; in general, it's a good idea to avoid introducing abstraction until you have explored the full problem space :') -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4720#note_56397
Jinoh Kang (@iamahuman) commented about dlls/ntdll/tests/exception.c:
#undef COMPARE }
+static void test_continue(void) +{ + struct context_pair { + CONTEXT before; + CONTEXT after; + } contexts; + NTSTATUS (*func_ptr)( struct context_pair *, void *arg2, void *continue_func, void *capture_func ) = code_mem; + + static const BYTE call_func[] = + { + /* need to preserve these */ + 0x53, /* push rbx */ Please use AT&T style (emitted by objdump and others) for consistency with existing code. For example, the operand order is reversed and registers are prefixed with `%`.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4720#note_56398
Jinoh Kang (@iamahuman) commented about dlls/ntdll/tests/exception.c:
+ 0x48, 0x8b, 0x4c, 0x24, 0x08, /* mov rcx, [rsp+8*1]; context */ + 0x48, 0x8b, 0x54, 0x24, 0x10, /* mov rdx, [rsp+8*2]; arg2 */ + 0x48, 0x83, 0xec, 0x70, /* sub rsp, 0x70; change stack */ + + /* setup context to return to label 1 */ + 0x48, 0x8d, 0x05, 0x1b, 0x00, 0x00, 0x00, /* lea rax, [rel 1] (0x1b) */ + 0x48, 0x89, 0x81, 0xf8, 0x00, 0x00, 0x00, /* mov [rcx + 0xf8 (context.Rip)], rax */ + + /* trash EFLAGS */ + 0x9c, /* pushfq */ + 0x58, /* pop rax */ + 0x48, 0x0d, 0x00, 0x45, 0x00, 0x00, /* or rax, 0x0100 + 0x4400; set trap flag so it's cleared. + FIXME: also clearing Direction Flag and Nested Task as those break Wine */ + + 0x48, 0xf7, 0xd0, /* not rax */ + 0x50, /* push rax */ Avoid changing unrelated flags. Reserved flag bits may obtain a meaning in the future and break tests.
```suggestion:-3+0 0x48, 0x81, 0x34, 0x24, 0xd5, 0x08, /* xorq $0x8d5,(%rsp) */ 0x00, 0x00, ``` Use xorq (QWORD PTR) instead of xorw (WORD PTR) to keep it straightforward and avoid potential store-to-load forwarding issues. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4720#note_56399
Jinoh Kang (@iamahuman) commented about dlls/ntdll/tests/exception.c:
+ 0x55, /* push rbp */ + 0x56, /* push rsi */ + 0x57, /* push rdi */ + 0x41, 0x54, /* push r12 */ + 0x41, 0x55, /* push r13 */ + 0x41, 0x56, /* push r14 */ + 0x41, 0x57, /* push r15 */ + + 0x48, 0x83, 0xec, 0x28, /* sub rsp, 8*5; stack space */ + 0x48, 0x89, 0x24, 0x24, /* mov [rsp+8*0], rsp; for validation */ + + /* save args */ + 0x48, 0x89, 0x4c, 0x24, 0x08, /* mov [rsp+8*1], rcx */ + 0x48, 0x89, 0x54, 0x24, 0x10, /* mov [rsp+8*2], rdx */ + 0x4c, 0x89, 0x44, 0x24, 0x18, /* mov [rsp+8*3], r8 */ + 0x4c, 0x89, 0x4c, 0x24, 0x20, /* mov [rsp+8*4], r9 */ Save arguments in the parameter home area wherever possible, instead of allocating its space yourself.
See https://learn.microsoft.com/en-us/cpp/build/stack-usage?view=msvc-170#stack-... for how x86-64 MSVC ABI defines the stack layout. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4720#note_56400
Jinoh Kang (@iamahuman) commented about dlls/ntdll/tests/exception.c:
+ 0x48, 0x89, 0xc3, /* mov rbx, rax */ + 0x48, 0x89, 0xc5, /* mov rbp, rax */ + 0x48, 0x89, 0xc6, /* mov rsi, rax */ + 0x48, 0x89, 0xc7, /* mov rdi, rax */ + 0x49, 0x89, 0xc0, /* mov r8, rax */ + 0x49, 0x89, 0xc1, /* mov r9, rax */ + 0x49, 0x89, 0xc2, /* mov r10, rax */ + 0x49, 0x89, 0xc3, /* mov r11, rax */ + 0x49, 0x89, 0xc4, /* mov r12, rax */ + 0x49, 0x89, 0xc5, /* mov r13, rax */ + 0x49, 0x89, 0xc6, /* mov r14, rax */ + 0x49, 0x89, 0xc7, /* mov r15, rax */ + + /* overwrite SSE registers */ + 0x66, 0x48, 0x0f, 0x6e, 0xc0, /* movq xmm0, rax */ + 0x66, 0x0f, 0x6c, 0xc0, /* punpcklqdq xmm0,xmm0; extend to high quadword */
0x0f, 0x57, 0xc0, /* xorps %xmm0,%xmm0 */
Same for all movaps below. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4720#note_56401
participants (3)
-
Alexandre Julliard (＠julliard) -
Dāvis Mosāns
-
Jinoh Kang (＠iamahuman)