[PATCH v5 0/2] MR3566: server: Store both contexts in pending context object.
And discard irrelevant parts in select request. We currently store something in CTX_PENDING and ignore it later in select request if we're currently running in wow context. -- v5: ntdll/tests: Add tests for setting context on unsuspended thread. server: Store both contexts in pending context object. https://gitlab.winehq.org/wine/wine/-/merge_requests/3566
From: Jacek Caban <jacek(a)codeweavers.com> And discard irrelevant parts in select request. --- server/thread.c | 67 +++++++++++++++++++++++-------------------------- 1 file changed, 32 insertions(+), 35 deletions(-) diff --git a/server/thread.c b/server/thread.c index 3e35b27f694..83ae381d5c5 100644 --- a/server/thread.c +++ b/server/thread.c @@ -119,11 +119,10 @@ struct context { struct object obj; /* object header */ unsigned int status; /* status of the context */ - context_t regs[3]; /* context data */ + context_t regs[2]; /* context data */ }; #define CTX_NATIVE 0 /* context for native machine */ #define CTX_WOW 1 /* context if thread is inside WoW */ -#define CTX_PENDING 2 /* pending native context when we don't know whether thread is inside WoW */ /* flags for registers that always need to be set from the server side */ static const unsigned int system_flags = SERVER_CTX_DEBUG_REGISTERS; @@ -292,7 +291,6 @@ static struct context *create_thread_context( struct thread *thread ) context->status = STATUS_PENDING; memset( &context->regs, 0, sizeof(context->regs) ); context->regs[CTX_NATIVE].machine = native_machine; - context->regs[CTX_PENDING].machine = native_machine; return context; } @@ -1591,8 +1589,6 @@ DECL_HANDLER(select) const context_t *native_context = (const context_t *)((const char *)(result + 1) + req->size); const context_t *wow_context = (ctx_count > 1) ? native_context + 1 : NULL; - if (current->context && current->context->status != STATUS_PENDING) goto invalid_param; - if (native_context->machine == native_machine) { if (wow_context && wow_context->machine != current->process->machine) goto invalid_param; @@ -1605,7 +1601,14 @@ DECL_HANDLER(select) } else goto invalid_param; - if (!current->context && !(current->context = create_thread_context( current ))) return; + if ((ctx = current->context)) + { + if (ctx->status != STATUS_PENDING) goto invalid_param; + /* if context was modified in pending state, discard irrelevant changes */ + if (wow_context) ctx->regs[CTX_NATIVE].flags &= ~ctx->regs[CTX_WOW].flags; + else ctx->regs[CTX_WOW].flags = ctx->regs[CTX_WOW].machine = 0; + } + else if (!(current->context = create_thread_context( current ))) return; ctx = current->context; if (native_context) @@ -1618,13 +1621,6 @@ DECL_HANDLER(select) ctx->regs[CTX_WOW].machine = current->process->machine; copy_context( &ctx->regs[CTX_WOW], wow_context, wow_context->flags & ~ctx->regs[CTX_WOW].flags ); } - else if (ctx->regs[CTX_PENDING].flags) - { - unsigned int flags = ctx->regs[CTX_PENDING].flags & ~ctx->regs[CTX_NATIVE].flags; - copy_context( &ctx->regs[CTX_NATIVE], &ctx->regs[CTX_PENDING], flags ); - ctx->regs[CTX_NATIVE].flags |= flags; - } - ctx->regs[CTX_PENDING].flags = 0; ctx->status = STATUS_SUCCESS; current->suspend_cookie = req->cookie; wake_up( &ctx->obj, 0 ); @@ -1931,36 +1927,37 @@ DECL_HANDLER(set_thread_context) set_error( STATUS_INVALID_PARAMETER ); else if (thread->state != TERMINATED) { - unsigned int ctx = CTX_NATIVE; - const context_t *context = &contexts[CTX_NATIVE]; - unsigned int flags = system_flags & context->flags; - unsigned int native_flags = context->flags & req->native_flags; + unsigned int flags = system_flags & contexts[CTX_NATIVE].flags; if (thread != current) stop_thread( thread ); - else if (flags) set_thread_context( thread, context, flags ); + else if (flags) set_thread_context( thread, &contexts[CTX_NATIVE], flags ); + if (thread->context && !get_error()) { - if (ctx_count == 2) + /* If context is in a pending state, we don't know if we will use WoW or native + * context, so store both and discard irrevelant one in select request. */ + const int is_pending = thread->context->status == STATUS_PENDING; + unsigned int native_flags = contexts[CTX_NATIVE].flags; + + if (ctx_count == 2 && (is_pending || thread->context->regs[CTX_WOW].machine)) { - /* If the target thread doesn't have a WoW context, set native instead. - * If we don't know yet whether we have a WoW context, store native context - * in CTX_PENDING and update when the target thread sends its context(s). */ - if (thread->context->status != STATUS_PENDING) - { - ctx = thread->context->regs[CTX_WOW].machine ? CTX_WOW : CTX_NATIVE; - context = &contexts[ctx]; - } - else ctx = CTX_PENDING; + context_t *ctx = &thread->context->regs[CTX_WOW]; + + /* some regs are always set from the native context */ + flags = contexts[CTX_WOW].flags & ~req->native_flags; + if (is_pending) ctx->machine = contexts[CTX_WOW].machine; + else native_flags &= req->native_flags; + + copy_context( ctx, &contexts[CTX_WOW], flags ); + ctx->flags |= flags; } - flags = context->flags; - if (native_flags && ctx != CTX_NATIVE) /* some regs are always set from the native context */ + + if (native_flags) { - copy_context( &thread->context->regs[CTX_NATIVE], &contexts[CTX_NATIVE], native_flags ); - thread->context->regs[CTX_NATIVE].flags |= native_flags; - flags &= ~native_flags; + context_t *ctx = &thread->context->regs[CTX_NATIVE]; + copy_context( ctx, &contexts[CTX_NATIVE], native_flags ); + ctx->flags |= native_flags; } - copy_context( &thread->context->regs[ctx], context, flags ); - thread->context->regs[ctx].flags |= flags; } } else set_error( STATUS_UNSUCCESSFUL ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3566
From: Jacek Caban <jacek(a)codeweavers.com> Based on patch by Evan Tang. --- dlls/ntdll/tests/thread.c | 85 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/dlls/ntdll/tests/thread.c b/dlls/ntdll/tests/thread.c index 56fb078babc..1ec2410409a 100644 --- a/dlls/ntdll/tests/thread.c +++ b/dlls/ntdll/tests/thread.c @@ -197,6 +197,90 @@ static void test_errno(void) ok( val == 0xbeef, "wrong value %x\n", val ); } +#if defined(__i386__) +# define IP_REG(ctx) ctx.Eip +#elif defined(__x86_64__) +# define IP_REG(ctx) ctx.Rip +#endif + +#ifdef IP_REG +static volatile int exit_ip_test; +static WINAPI DWORD ip_test_thread_proc( void *param ) +{ + SetEvent( param ); + while (!exit_ip_test); + return ERROR_SUCCESS; +} +#endif + +static void test_set_thread_context_ip(void) +{ +#ifdef IP_REG + UINT_PTR old_ip, target; + HANDLE thread, event; + char *target_ptr; + CONTEXT ctx; + DWORD res; + int i; + + /* jmp to self at offset 0 and 4 */ + static const char target_code[] = {0xeb, 0xfe, 0x90, 0x90, 0xeb, 0xfe}; + + target_ptr = VirtualAlloc( NULL, 65536, MEM_RESERVE | MEM_COMMIT, PAGE_EXECUTE_READWRITE ); + memcpy( target_ptr, target_code, sizeof(target_code) ); + target = (UINT_PTR)target_ptr; + + event = CreateEventW( NULL, TRUE, FALSE, NULL ); + thread = CreateThread( NULL, 65536, ip_test_thread_proc, event, 0, NULL ); + ok( thread != NULL, "Failed to create thread: %lx\n", GetLastError() ); + res = WaitForSingleObject( event, 1000 ); + ok( !res, "wait returned: %ld\n", res ); + CloseHandle( event ); + + memset( &ctx, 0, sizeof(ctx) ); + ctx.ContextFlags = CONTEXT_ALL; + res = GetThreadContext( thread, &ctx ); + ok( res, "Failed to get thread context: %lx\n", GetLastError() ); + old_ip = IP_REG(ctx); + + IP_REG(ctx) = target; + res = SetThreadContext( thread, &ctx ); + ok( res, "Failed to set thread context: %lx\n", GetLastError() ); + + for (i = 0; i < 10; i++) + { + IP_REG(ctx) = target; + res = SetThreadContext( thread, &ctx ); + ok( res, "Failed to set thread context: %lx\n", GetLastError() ); + + ctx.ContextFlags = CONTEXT_ALL; + res = GetThreadContext( thread, &ctx ); + ok( res, "Failed to get thread context: %lx\n", GetLastError() ); + ok( IP_REG(ctx) == target, "IP = %p, expected %p\n", (void *)IP_REG(ctx), target_ptr ); + + IP_REG(ctx) = target + 4; + res = SetThreadContext( thread, &ctx ); + ok( res, "Failed to set thread context: %lx\n", GetLastError()) ; + + ctx.ContextFlags = CONTEXT_ALL; + res = GetThreadContext( thread, &ctx ); + ok( res, "Failed to get thread context: %lx\n", GetLastError() ); + ok( IP_REG(ctx) == target + 4, "IP = %p, expected %p\n", (void *)IP_REG(ctx), target_ptr + 4 ); + } + + exit_ip_test = 1; + ctx.ContextFlags = CONTEXT_ALL; + IP_REG(ctx) = old_ip; + res = SetThreadContext( thread, &ctx ); + ok( res, "Failed to restore thread context: %lx\n", GetLastError() ); + + res = WaitForSingleObject( thread, 1000 ); + ok( !res, "wait returned: %ld\n", res ); + + VirtualFree( target_ptr, 0, MEM_RELEASE ); +#endif +} + START_TEST(thread) { init_function_pointers(); @@ -204,4 +288,5 @@ START_TEST(thread) test_dbg_hidden_thread_creation(); test_unique_teb(); test_errno(); + test_set_thread_context_ip(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3566
On Mon Aug 28 22:58:19 2023 +0000, Jinoh Kang wrote:
Something like this should be more readable: ```c if (thread->context && !get_error()) { const unsigned int native_flags = contexts[CTX_NATIVE].flags & req->native_flags; const int is_pending = thread->context->status == STATUS_PENDING; const int selected_ctx = ctx_count == 2 && thread->context->regs[CTX_WOW].machine ? CTX_WOW : CTX_NATIVE; unsigned int i; for (i = 0; i < ctx_count; i++) { context_t *dest_context = &thread->context->regs[i]; unsigned int new_flags = 0; /* some regs are always set from the native context */ if (i == CTX_NATIVE) new_flags |= native_flags; /* If context is in a pending state, we don't know if we will use WoW or native * context, so store both and discard irrevelant one in select request. */ if (is_pending || i == selected_ctx) new_flags |= contexts[i].flags & ~native_flags; if (i == CTX_WOW && !dest_context->machine) { if (!is_pending) continue; /* If pending, lazily initialize machine */ dest_context->machine = thread->process->machine; } copy_context( dest_context, &contexts[i], new_flags ); dest_context->flags |= new_flags; } } ``` I pushed a version that reorders context copies, allowing tweaking `native_flags` in wow context branch. This should fix the check for native context copying and avoid extra `copy_context` calls.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3566#note_43627
Jinoh Kang (@iamahuman) commented about server/thread.c:
- if (thread->context->status != STATUS_PENDING) + /* If context is in a pending state, we don't know if we will use WoW or native + * context, so store both and discard irrevelant one in select request. */ + const int is_pending = thread->context->status == STATUS_PENDING; + unsigned int native_flags = contexts[CTX_NATIVE].flags; + + if (ctx_count == 2 && (is_pending || thread->context->regs[CTX_WOW].machine)) { - ctx = thread->context->regs[CTX_WOW].machine ? CTX_WOW : CTX_NATIVE; - context = &contexts[ctx]; - } - else ctx = CTX_PENDING; + context_t *ctx = &thread->context->regs[CTX_WOW]; + + /* some regs are always set from the native context */ + flags = contexts[CTX_WOW].flags & ~req->native_flags; From Julliard's commit fa47ea740020dd5bffa94b3cd835559393b2f752, we always mask `req->native_flags` with `contexts[CTX_NATIVE].flags` before using it. This patch removes that mask.
I'm not sure if that masking was intentional, since the client side already applies the mask before passing it to the server. I'll leave that decision up to @julliard. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3566#note_43712
On Tue Aug 29 15:20:43 2023 +0000, Jinoh Kang wrote:
This causes regression in 64-bit ntdll:exception on new-style wow64: ```diff --- OLD 2023-08-26 07:05:32.639000000 +0900 +++ NEW 2023-08-26 07:07:53.909000000 +0900 @@ -15,6 +15,16 @@ exception.c:3627: exception: c0000008 flags:0 addr:0000000000AB0010 context: Rip:0000000000AB0010 exception.c:3740: Test marked todo: Frame handler called exception.c:3742: Test marked todo: UnhandledExceptionFilter wasn't called +exception.c:8328: Test failed: expected 0x12340000, got 0 +exception.c:8329: Test failed: expected 0x12340001, got 0 +exception.c:8330: Test failed: expected 0x12340002, got 0 +exception.c:8331: Test failed: expected 0x12340003, got 0 +exception.c:8332: Test failed: expected 0x155, got 0 +exception.c:8341: Test failed: expected 0x12340000, got 0 +exception.c:8342: Test failed: expected 0x12340001, got 0 +exception.c:8343: Test failed: expected 0x12340002, got 0 +exception.c:8344: Test failed: expected 0x12340003, got 0 +exception.c:8345: Test failed: expected 0x155, got 0 exception.c:8396: vect. handler 80000003 addr:0000000000AB0030 exception.c:8396: vect. handler 80000003 addr:0000000000AB0030 exception.c:8396: vect. handler 80000003 addr:0000000000AB0030 @@ -221,9 +231,10 @@ exception.c:4467: Test failed: cs64: wrong fs 0063 / 0000 exception.c:4468: Test failed: cs64: wrong gs 002b / 0000 exception.c:4516: Test failed: cs64: ecx set to 00000000 +exception.c:4520: Test failed: cs64: rcx set to 00007FFFFE1FF820 exception.c:4388: in 32-bit mode 0023 -exception.c:4439: Test failed: cs64: ecx set to 00000003 -exception.c:4453: Test failed: cs64: rcx set to 0FEDCBA987654321 +exception.c:4439: Test failed: cs64: ecx set to 7bc6f610 +exception.c:4453: Test failed: cs64: rcx set to 000000007BC6F610 exception.c:4608: rec 000000000021F260, context 000000000021ED70. exception.c:4609: context->Rip 0xab0025, context->Rsp 0x21f488, ContextFlags 0x10005f. exception.c:4566: handler context->Rip 0xab0025, codemem 0000000000AB0000. @@ -362,4 +373,4 @@ exception.c:8746: vect. handler 80000003 addr:0000000000AB0001 018c:exception: 7 tests executed (0 marked as todo, 0 as flaky, 0 failures), 0 skipped. 0020:exception: Silenced 0 todos, 0 skips and 14 traces. -0020:exception: 4955 tests executed (129 marked as todo, 0 as flaky, 12 failures), 0 skipped. +0020:exception: 4931 tests executed (129 marked as todo, 0 as flaky, 23 failures), 0 skipped. ``` Seems to be resolved now.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3566#note_43714
On Tue Aug 29 15:22:08 2023 +0000, Jacek Caban wrote:
I pushed a version that reorders context copies, allowing tweaking `native_flags` in wow context branch. This should fix the check for native context copying and avoid extra `copy_context` calls. The new code is quite dense, but avoiding `continue` looks good.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3566#note_43716
I'm removing myself as a reviewer since I don't think I should be blocking this MR, but I'm not so sure about whether I can be considered authoritative on the design either. Sorry. Please feel free to re-request review if you have any changes (e.g., masking), though. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3566#note_44177
participants (3)
-
Jacek Caban -
Jacek Caban (＠jacek)
-
Jinoh Kang (＠iamahuman)