AFAICS stepping, continuing and breaking with Ctrl-C now work. Thread selection, registers and backtrace should now be correct even with multiple threads.
Breakpoints are working when using explicit addresses but debug information is apparently still wrong sometimes, so breaking into source or function do not work reliably yet. Watchpoints are still not implemented.
Rémi Bernon (15): server: Wait before suspending threads in APC. ntdll: Support DBG_REPLY_LATER event continuation. winedbg: Support using lldb as frontend. winedbg: Add snscanf function to safely parse packets. winedbg: Cleanup extract_packets for faster acking. winedbg: Cleanup return for kill and status packets. winedbg: Support partial qXfer replies. winedbg: Support qXfer:features:read request. winedbg: Support qXfer:libraries:read request. winedbg: Support qXfer:threads:read request. winedbg: Support QStartNoAckMode to reduce verbosity. winedbg: Fix register read/write handlers. winedbg: Rewrite and simplify vCont request handler. winedbg: Improve continue/step packets implementation. winedbg: Use tid for other/exec thread operations.
dlls/ntdll/signal_arm.c | 8 +- dlls/ntdll/signal_arm64.c | 15 +- dlls/ntdll/signal_i386.c | 15 +- dlls/ntdll/signal_powerpc.c | 8 +- dlls/ntdll/signal_x86_64.c | 12 +- include/ntstatus.h | 1 + include/winnt.h | 1 + programs/winedbg/debugger.h | 2 + programs/winedbg/gdbproxy.c | 1017 ++++++++++++++++++++--------------- programs/winedbg/winedbg.c | 1 + server/thread.c | 5 + server/thread.h | 1 + 12 files changed, 625 insertions(+), 461 deletions(-)
The APC_BREAK_PROCESS call creates a thread that raises an exception, suspending the whole process. There's a race condition between this thread and the APC thread notifying the server of its completion.
This fixes "winedbg --gdb" sometimes not responding after Ctrl-C.
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- server/thread.c | 5 +++++ server/thread.h | 1 + 2 files changed, 6 insertions(+)
diff --git a/server/thread.c b/server/thread.c index 80db41b48d2..a2fd4908792 100644 --- a/server/thread.c +++ b/server/thread.c @@ -199,6 +199,7 @@ static inline void init_thread_structure( struct thread *thread ) thread->exit_code = 0; thread->priority = 0; thread->suspend = 0; + thread->in_apc = 0; thread->desktop_users = 0; thread->token = NULL; thread->desc = NULL; @@ -583,6 +584,7 @@ static void set_thread_info( struct thread *thread, /* stop a thread (at the Unix level) */ void stop_thread( struct thread *thread ) { + if (thread->in_apc) return; /* currently doing apc, will be suspended on return */ if (thread->context) return; /* already inside a debug event, no need for a signal */ /* can't stop a thread while initialisation is in progress */ if (is_process_init_done(thread->process)) send_thread_signal( thread, SIGUSR1 ); @@ -1575,6 +1577,8 @@ DECL_HANDLER(select) wake_up( &apc->obj, 0 ); close_handle( current->process, req->prev_apc ); release_object( apc ); + current->in_apc = 0; + stop_thread_if_suspended( current ); }
reply->timeout = select_on( &select_op, op_size, req->cookie, req->flags, req->timeout ); @@ -1591,6 +1595,7 @@ DECL_HANDLER(select) if (apc->call.type != APC_NONE && (reply->apc_handle = alloc_handle( current->process, apc, SYNCHRONIZE, 0 ))) { + current->in_apc = 1; reply->call = apc->call; release_object( apc ); break; diff --git a/server/thread.h b/server/thread.h index 66e35603d36..08700455972 100644 --- a/server/thread.h +++ b/server/thread.h @@ -82,6 +82,7 @@ struct thread affinity_t affinity; /* affinity mask */ int priority; /* priority level */ int suspend; /* suspend count */ + int in_apc; /* currently doing apc */ obj_handle_t desktop; /* desktop handle */ int desktop_users; /* number of objects using the thread desktop */ timeout_t creation_time; /* Thread creation time */
Hi Rémi,
On 27/01/2020 13:07, Rémi Bernon wrote:
The APC_BREAK_PROCESS call creates a thread that raises an exception, suspending the whole process. There's a race condition between this thread and the APC thread notifying the server of its completion.
What's wrong about that? Suspending process on thread creation shouldn't be a problem. The debugger should process this event, which will resume the new thread and let it continue to the break point. Before introducing APC_BREAK_PROCESS I had to fix it in another place in winedbg, see commit 1bb98982d682a85b82e, maybe there is similar problem in gdb proxy?
Thanks,
Jacek
On 1/27/20 10:58 PM, Jacek Caban wrote:
Hi Rémi,
On 27/01/2020 13:07, Rémi Bernon wrote:
The APC_BREAK_PROCESS call creates a thread that raises an exception, suspending the whole process. There's a race condition between this thread and the APC thread notifying the server of its completion.
What's wrong about that? Suspending process on thread creation shouldn't be a problem. The debugger should process this event, which will resume the new thread and let it continue to the break point. Before introducing APC_BREAK_PROCESS I had to fix it in another place in winedbg, see commit 1bb98982d682a85b82e, maybe there is similar problem in gdb proxy?
Thanks,
Jacek
In gdbproxy mode, winedbg calls DebugBreakProcess whenever Ctrl-C is pressed, then WaitForDebugEvent for the corresponding event.
However *sometimes* DebugBreakProcess never returns and it cannot wait for the event and process it unless it is done in another thread -which would require changes in gdbproxy I would rather avoid. That may be how winedbg works in frontend mode?
The reason why DebugBreakProcess sometimes returns but not always is that it requests the APC, which completion notification is sometimes received by the server before the exception is raised and all threads are suspended, but sometimes it isn't. I don't think this was intended, and it would probably be better if DebugBreakProcess returns -or not- in a consistent way.
On 27.01.2020 23:19, Rémi Bernon wrote:
On 1/27/20 10:58 PM, Jacek Caban wrote:
Hi Rémi,
On 27/01/2020 13:07, Rémi Bernon wrote:
The APC_BREAK_PROCESS call creates a thread that raises an exception, suspending the whole process. There's a race condition between this thread and the APC thread notifying the server of its completion.
What's wrong about that? Suspending process on thread creation shouldn't be a problem. The debugger should process this event, which will resume the new thread and let it continue to the break point. Before introducing APC_BREAK_PROCESS I had to fix it in another place in winedbg, see commit 1bb98982d682a85b82e, maybe there is similar problem in gdb proxy?
Thanks,
Jacek
In gdbproxy mode, winedbg calls DebugBreakProcess whenever Ctrl-C is pressed, then WaitForDebugEvent for the corresponding event.
However *sometimes* DebugBreakProcess never returns and it cannot wait for the event and process it unless it is done in another thread -which would require changes in gdbproxy I would rather avoid. That may be how winedbg works in frontend mode?
The reason why DebugBreakProcess sometimes returns but not always is that it requests the APC, which completion notification is sometimes received by the server before the exception is raised and all threads are suspended, but sometimes it isn't. I don't think this was intended, and it would probably be better if DebugBreakProcess returns -or not- in a consistent way.
OK, I see, there is a problem. But I think that with your patch the race is still there, it's just much less likely. Server may have already sent the signal before client sent select request, but it may be blocked until server call is finishes. Then, if select returns system APC, it will be immediately interrupted by the signal.
Jacek
On 1/28/20 2:09 PM, Jacek Caban wrote:
On 27.01.2020 23:19, Rémi Bernon wrote:
On 1/27/20 10:58 PM, Jacek Caban wrote:
Hi Rémi,
On 27/01/2020 13:07, Rémi Bernon wrote:
The APC_BREAK_PROCESS call creates a thread that raises an exception, suspending the whole process. There's a race condition between this thread and the APC thread notifying the server of its completion.
What's wrong about that? Suspending process on thread creation shouldn't be a problem. The debugger should process this event, which will resume the new thread and let it continue to the break point. Before introducing APC_BREAK_PROCESS I had to fix it in another place in winedbg, see commit 1bb98982d682a85b82e, maybe there is similar problem in gdb proxy?
Thanks,
Jacek
In gdbproxy mode, winedbg calls DebugBreakProcess whenever Ctrl-C is pressed, then WaitForDebugEvent for the corresponding event.
However *sometimes* DebugBreakProcess never returns and it cannot wait for the event and process it unless it is done in another thread -which would require changes in gdbproxy I would rather avoid. That may be how winedbg works in frontend mode?
The reason why DebugBreakProcess sometimes returns but not always is that it requests the APC, which completion notification is sometimes received by the server before the exception is raised and all threads are suspended, but sometimes it isn't. I don't think this was intended, and it would probably be better if DebugBreakProcess returns -or not- in a consistent way.
OK, I see, there is a problem. But I think that with your patch the race is still there, it's just much less likely. Server may have already sent the signal before client sent select request, but it may be blocked until server call is finishes. Then, if select returns system APC, it will be immediately interrupted by the signal.
Jacek
Let's name these locations:
[A]: @@ -1591,6 +1595,7 @@ DECL_HANDLER(select) if (apc->call.type != APC_NONE && (reply->apc_handle = alloc_handle( current->process, apc, SYNCHRONIZE, 0 ))) { + current->in_apc = 1; reply->call = apc->call; release_object( apc ); break;
[B]: @@ -1575,6 +1577,8 @@ DECL_HANDLER(select) wake_up( &apc->obj, 0 ); close_handle( current->process, req->prev_apc ); release_object( apc ); + current->in_apc = 0; + stop_thread_if_suspended( current ); }
reply->timeout = select_on( &select_op, op_size, req->cookie, req->flags, req->timeout );
[C]: @@ -583,6 +584,7 @@ static void set_thread_info( struct thread *thread, /* stop a thread (at the Unix level) */ void stop_thread( struct thread *thread ) { + if (thread->in_apc) return; /* currently doing apc, will be suspended on return */ if (thread->context) return; /* already inside a debug event, no need for a signal */ /* can't stop a thread while initialisation is in progress */ if (is_process_init_done(thread->process)) send_thread_signal( thread, SIGUSR1 );
When the APC_BREAK_PROCESS executes, a servicing thread calls the select request, which returns through [A], flagging the thread.
It returns from the select request, and does the APC call -which creates the exception thread. Then, I see two possible ordering:
1) The servicing thread calls first the select request to notify APC completion, goes to [B] which wakes the APC caller -resuming DbgBreakProcess on the debugger side, and unflags the thread but it's not suspended yet. The exception thread does the queue_exception_event request, which ultimately goes to [C] when suspending the process. The servicing thread is not flagged, so it is suspended.
2) The exception thread does the queue_exception_event first, which goes to [C], but as the servicing thread is still flagged, do not suspend it. The servicing thread then does the select request, goes to [B] which wakes the APC caller -resuming DbgBreakProcess- then unflags and suspends it as it should be.
Now I agree there could still be some race conditions in case 2):
* The additional thread suspension in [B] is done after the wake_up for the APC caller, it should probably be done before, just to be sure.
* If the debugger is calling WaitForDebugEvent in parallel to the DbgBreakProcess call. In this case, the debugger could maybe be notified of a debug event after the exception thread created it but before the servicing thread notifies of the APC completion. I don't if that's supposed to be supported, or how bad it is to have the servicing thread still alive for a bit.
If we want to have the later scenario be race-free, it would require to link the debug even only when the servicing thread is suspended. Or, to synchronize the servicing and exception thread somehow so that the servicing always does its select request first.
On 1/28/20 2:43 PM, Rémi Bernon wrote:
If we want to have the later scenario be race-free, it would require to link the debug even only when the servicing thread is suspended. Or, to synchronize the servicing and exception thread somehow so that the servicing always does its select request first.
Or maybe, as a simpler solution, hiding the problem by synchronizing DbgBreakProcess and WaitForDebugEvent.
On 28.01.2020 14:43, Rémi Bernon wrote:
Let's name these locations:
[A]: @@ -1591,6 +1595,7 @@ DECL_HANDLER(select) if (apc->call.type != APC_NONE && (reply->apc_handle = alloc_handle( current->process, apc, SYNCHRONIZE, 0 ))) { + current->in_apc = 1; reply->call = apc->call; release_object( apc ); break;
[B]: @@ -1575,6 +1577,8 @@ DECL_HANDLER(select) wake_up( &apc->obj, 0 ); close_handle( current->process, req->prev_apc ); release_object( apc ); + current->in_apc = 0; + stop_thread_if_suspended( current ); }
reply->timeout = select_on( &select_op, op_size, req->cookie, req->flags, req->timeout );
[C]: @@ -583,6 +584,7 @@ static void set_thread_info( struct thread *thread, /* stop a thread (at the Unix level) */ void stop_thread( struct thread *thread ) { + if (thread->in_apc) return; /* currently doing apc, will be suspended on return */ if (thread->context) return; /* already inside a debug event, no need for a signal */ /* can't stop a thread while initialisation is in progress */ if (is_process_init_done(thread->process)) send_thread_signal( thread, SIGUSR1 );
When the APC_BREAK_PROCESS executes, a servicing thread calls the select request, which returns through [A], flagging the thread.
It returns from the select request, and does the APC call -which creates the exception thread. Then, I see two possible ordering:
- The servicing thread calls first the select request to notify APC
completion, goes to [B] which wakes the APC caller -resuming DbgBreakProcess on the debugger side, and unflags the thread but it's not suspended yet. The exception thread does the queue_exception_event request, which ultimately goes to [C] when suspending the process. The servicing thread is not flagged, so it is suspended.
- The exception thread does the queue_exception_event first, which
goes to [C], but as the servicing thread is still flagged, do not suspend it. The servicing thread then does the select request, goes to [B] which wakes the APC caller -resuming DbgBreakProcess- then unflags and suspends it as it should be.
Now I agree there could still be some race conditions in case 2):
- The additional thread suspension in [B] is done after the wake_up
for the APC caller, it should probably be done before, just to be sure.
- If the debugger is calling WaitForDebugEvent in parallel to the
DbgBreakProcess call. In this case, the debugger could maybe be notified of a debug event after the exception thread created it but before the servicing thread notifies of the APC completion. I don't if that's supposed to be supported, or how bad it is to have the servicing thread still alive for a bit.
If we want to have the later scenario be race-free, it would require to link the debug even only when the servicing thread is suspended. Or, to synchronize the servicing and exception thread somehow so that the servicing always does its select request first.
I think the more general problem is that any system APCs may be interrupted and suspended. If DbgBreakProcess is the only concern, we could probably hack it to not wait for APC result at all - it doesn't really need to (except for propagating OOM-like errors). For the general problem, let me use an example with another thread that just happens to suspend the process about the same time as DbgBreakProcess happens:
1. Debuggee thread calls server_select and enters the uninterrupted section in wine_server_call, but does not yet send the request
2. Thread 1 (of any process) requests suspending of debuggee process
3. Debugger's thread 2 calls DbgBreakProcess on debuggee process and waits for APC completion
4. Server processes suspend request from (2) and sends signals to all debuggee threads
5. Server processes break APC request from (3) and queues an APC
6. Debuggee thread from (1) finally sends the request and server replies with queued APC
7. Debuggee thread leaves uninterrupted section, and receives queued signal, which suspends the thread while it's about to process a system APC
You could maybe detect the problem on server side in (6), return pending and let signal handler to handle the APC. I'm a bit sceptical about that solution.
Another solution would be to ensure that system APCs are ran with signals blocked on client side. It's quite tricky through as it would require disabling signals while waiting in server_select (except for user APCs). This could be fine if we passed CPU context in select request, so that server wouldn't need any signal to suspend the thread.
Jacek
Indeed, I didn't consider the case where another exception is raised at the same time the debugger tries to break.
On 1/28/20 3:50 PM, Jacek Caban wrote:
Another solution would be to ensure that system APCs are ran with signals blocked on client side. It's quite tricky through as it would require disabling signals while waiting in server_select (except for user APCs). This could be fine if we passed CPU context in select request, so that server wouldn't need any signal to suspend the thread.
I'm not sure to see why passing the context would make things easier, I understand it's just a matter of extending the size of the uninterrupted section in wine_server_call to stay in it while processing system apcs.
However it still wouldn't solve the issue I described previously, where WaitForDebugEvent would return as soon as the exception debug event is linked, and the debugger could then assume the process is stopped although some threads are still possibly running system apcs.
On 28.01.2020 19:00, Rémi Bernon wrote:
However it still wouldn't solve the issue I described previously, where WaitForDebugEvent would return as soon as the exception debug event is linked, and the debugger could then assume the process is stopped although some threads are still possibly running system apcs.
It should be fine that they are running system APCs. Suspended process should handle system APCs as well, otherwise you wouldn't be able to do calls like VirtualQueryEx in debugger while debuggee is suspended. The important part is that only system APCs are ran at this point, no user code.
Jacek
This flag causes the debug event to be replayed after the target thread continues. It can be used, after suspending the thread, to resume other threads and later return to the breaking.
This will help implementing gdb continue/step packets correctly.
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- dlls/ntdll/signal_arm.c | 8 ++++++-- dlls/ntdll/signal_arm64.c | 15 ++++++++++++--- dlls/ntdll/signal_i386.c | 15 ++++++++++++--- dlls/ntdll/signal_powerpc.c | 8 ++++++-- dlls/ntdll/signal_x86_64.c | 12 +++++++++--- include/ntstatus.h | 1 + include/winnt.h | 1 + 7 files changed, 47 insertions(+), 13 deletions(-)
diff --git a/dlls/ntdll/signal_arm.c b/dlls/ntdll/signal_arm.c index 3de6a0c45a5..a819b2a2f28 100644 --- a/dlls/ntdll/signal_arm.c +++ b/dlls/ntdll/signal_arm.c @@ -681,7 +681,9 @@ static NTSTATUS raise_exception( EXCEPTION_RECORD *rec, CONTEXT *context, BOOL f context->R12, context->Sp, context->Lr, context->Pc, context->Cpsr ); }
- status = send_debug_event( rec, TRUE, context ); + do { status = send_debug_event( rec, TRUE, context ); } + while (status == DBG_REPLY_LATER); + if (status == DBG_CONTINUE || status == DBG_EXCEPTION_HANDLED) return STATUS_SUCCESS;
@@ -694,7 +696,9 @@ static NTSTATUS raise_exception( EXCEPTION_RECORD *rec, CONTEXT *context, BOOL f
/* last chance exception */
- status = send_debug_event( rec, FALSE, context ); + do { status = send_debug_event( rec, FALSE, context ); } + while (status == DBG_REPLY_LATER); + if (status != DBG_CONTINUE) { if (rec->ExceptionFlags & EH_STACK_INVALID) diff --git a/dlls/ntdll/signal_arm64.c b/dlls/ntdll/signal_arm64.c index 6d72938809c..140ac7552c6 100644 --- a/dlls/ntdll/signal_arm64.c +++ b/dlls/ntdll/signal_arm64.c @@ -957,7 +957,9 @@ static NTSTATUS raise_exception( EXCEPTION_RECORD *rec, CONTEXT *context, BOOL f
/* last chance exception */
- status = send_debug_event( rec, FALSE, context ); + do { status = send_debug_event( rec, FALSE, context ); } + while (status == DBG_REPLY_LATER); + if (status != DBG_CONTINUE) { if (rec->ExceptionFlags & EH_STACK_INVALID) @@ -1013,7 +1015,10 @@ static void WINAPI raise_generic_exception( EXCEPTION_RECORD *rec, CONTEXT *cont */ static void setup_raise_exception( ucontext_t *sigcontext, struct stack_layout *stack ) { - NTSTATUS status = send_debug_event( &stack->rec, TRUE, &stack->context ); + NTSTATUS status; + + do { status = send_debug_event( &stack->rec, TRUE, &stack->context ); } + while (status == DBG_REPLY_LATER);
if (status == DBG_CONTINUE || status == DBG_EXCEPTION_HANDLED) { @@ -1910,7 +1915,11 @@ NTSTATUS WINAPI NtRaiseException( EXCEPTION_RECORD *rec, CONTEXT *context, BOOL { if (first_chance) { - NTSTATUS status = send_debug_event( rec, TRUE, context ); + NTSTATUS status; + + do { status = send_debug_event( rec, TRUE, context ); } + while (status == DBG_REPLY_LATER); + if (status == DBG_CONTINUE || status == DBG_EXCEPTION_HANDLED) NtSetContextThread( GetCurrentThread(), context ); } diff --git a/dlls/ntdll/signal_i386.c b/dlls/ntdll/signal_i386.c index e9dd0de2fc3..e69fb9b9977 100644 --- a/dlls/ntdll/signal_i386.c +++ b/dlls/ntdll/signal_i386.c @@ -739,7 +739,9 @@ static NTSTATUS raise_exception( EXCEPTION_RECORD *rec, CONTEXT *context, BOOL f
/* last chance exception */
- status = send_debug_event( rec, FALSE, context ); + do { status = send_debug_event( rec, FALSE, context ); } + while (status == DBG_REPLY_LATER); + if (status != DBG_CONTINUE) { if (rec->ExceptionFlags & EH_STACK_INVALID) @@ -1872,7 +1874,10 @@ static void WINAPI raise_generic_exception( EXCEPTION_RECORD *rec, CONTEXT *cont */ static void setup_raise_exception( ucontext_t *sigcontext, struct stack_layout *stack ) { - NTSTATUS status = send_debug_event( &stack->rec, TRUE, &stack->context ); + NTSTATUS status; + + do { status = send_debug_event( &stack->rec, TRUE, &stack->context ); } + while (status == DBG_REPLY_LATER);
if (status == DBG_CONTINUE || status == DBG_EXCEPTION_HANDLED) { @@ -2508,7 +2513,11 @@ NTSTATUS WINAPI NtRaiseException( EXCEPTION_RECORD *rec, CONTEXT *context, BOOL { if (first_chance) { - NTSTATUS status = send_debug_event( rec, TRUE, context ); + NTSTATUS status; + + do { status = send_debug_event( rec, TRUE, context ); } + while (status == DBG_REPLY_LATER); + if (status == DBG_CONTINUE || status == DBG_EXCEPTION_HANDLED) NtSetContextThread( GetCurrentThread(), context ); } diff --git a/dlls/ntdll/signal_powerpc.c b/dlls/ntdll/signal_powerpc.c index f23265445df..d92127e1432 100644 --- a/dlls/ntdll/signal_powerpc.c +++ b/dlls/ntdll/signal_powerpc.c @@ -687,7 +687,9 @@ static NTSTATUS raise_exception( EXCEPTION_RECORD *rec, CONTEXT *context, BOOL f /* FIXME: dump context */ }
- status = send_debug_event( rec, TRUE, context ); + do { status = send_debug_event( rec, TRUE, context ); } + while (status == DBG_REPLY_LATER); + if (status == DBG_CONTINUE || status == DBG_EXCEPTION_HANDLED) return STATUS_SUCCESS;
@@ -700,7 +702,9 @@ static NTSTATUS raise_exception( EXCEPTION_RECORD *rec, CONTEXT *context, BOOL f
/* last chance exception */
- status = send_debug_event( rec, FALSE, context ); + do { status = send_debug_event( rec, FALSE, context ); } + while (status == DBG_REPLY_LATER); + if (status != DBG_CONTINUE) { if (rec->ExceptionFlags & EH_STACK_INVALID) diff --git a/dlls/ntdll/signal_x86_64.c b/dlls/ntdll/signal_x86_64.c index 04f3854388c..c97b8a1d02f 100644 --- a/dlls/ntdll/signal_x86_64.c +++ b/dlls/ntdll/signal_x86_64.c @@ -2567,7 +2567,9 @@ static NTSTATUS raise_exception( EXCEPTION_RECORD *rec, CONTEXT *context, BOOL f
/* last chance exception */
- status = send_debug_event( rec, FALSE, context ); + do { status = send_debug_event( rec, FALSE, context ); } + while (status == DBG_REPLY_LATER); + if (status != DBG_CONTINUE) { if (rec->ExceptionFlags & EH_STACK_INVALID) @@ -2594,7 +2596,9 @@ NTSTATUS WINAPI NtRaiseException( EXCEPTION_RECORD *rec, CONTEXT *context, BOOL
if (first_chance) { - status = send_debug_event( rec, TRUE, context ); + do { status = send_debug_event( rec, TRUE, context ); } + while (status == DBG_REPLY_LATER); + if (status == DBG_CONTINUE || status == DBG_EXCEPTION_HANDLED) return NtSetContextThread( GetCurrentThread(), context ); } @@ -2727,7 +2731,9 @@ static void setup_raise_exception( ucontext_t *sigcontext, struct stack_layout * stack->context.EFlags &= ~0x100; /* clear single-step flag */ }
- status = send_debug_event( &stack->rec, TRUE, &stack->context ); + do { status = send_debug_event( &stack->rec, TRUE, &stack->context ); } + while (status == DBG_REPLY_LATER); + if (status == DBG_CONTINUE || status == DBG_EXCEPTION_HANDLED) { restore_context( &stack->context, sigcontext ); diff --git a/include/ntstatus.h b/include/ntstatus.h index 682db7a5660..1e46529262b 100644 --- a/include/ntstatus.h +++ b/include/ntstatus.h @@ -1408,6 +1408,7 @@
#define DBG_EXCEPTION_HANDLED ((NTSTATUS) 0x00010001) #define DBG_CONTINUE ((NTSTATUS) 0x00010002) +#define DBG_REPLY_LATER ((NTSTATUS) 0x40010001) #define DBG_TERMINATE_THREAD ((NTSTATUS) 0x40010003) #define DBG_TERMINATE_PROCESS ((NTSTATUS) 0x40010004) #define DBG_CONTROL_C ((NTSTATUS) 0x40010005) diff --git a/include/winnt.h b/include/winnt.h index 41fb6a82897..adc5d3d686c 100644 --- a/include/winnt.h +++ b/include/winnt.h @@ -651,6 +651,7 @@ typedef DWORD FLONG; /* status values for ContinueDebugEvent */ #define DBG_EXCEPTION_HANDLED ((DWORD) 0x00010001) #define DBG_CONTINUE ((DWORD) 0x00010002) +#define DBG_REPLY_LATER ((DWORD) 0x40010001) #define DBG_TERMINATE_THREAD ((DWORD) 0x40010003) #define DBG_TERMINATE_PROCESS ((DWORD) 0x40010004) #define DBG_CONTROL_C ((DWORD) 0x40010005)
Hi Rémi,
On 27.01.2020 13:07, Rémi Bernon wrote:
This flag causes the debug event to be replayed after the target thread continues. It can be used, after suspending the thread, to resume other threads and later return to the breaking.
This will help implementing gdb continue/step packets correctly.
Signed-off-by: Rémi Bernon rbernon@codeweavers.com
dlls/ntdll/signal_arm.c | 8 ++++++-- dlls/ntdll/signal_arm64.c | 15 ++++++++++++--- dlls/ntdll/signal_i386.c | 15 ++++++++++++--- dlls/ntdll/signal_powerpc.c | 8 ++++++-- dlls/ntdll/signal_x86_64.c | 12 +++++++++---
You could just change send_debug_event instead of all its callers. I'd also consider implementing it on server side. Also some tests would be nice.
Thanks,
Jacek
This is currently not working very well because of a lot of missing packets, but at least we can try.
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- programs/winedbg/gdbproxy.c | 39 +++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 15 deletions(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index 052e73b2ad6..f85e09353e2 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -1874,24 +1874,33 @@ static BOOL gdb_exec(const char* wine_path, unsigned port, unsigned flags) fd = mkstemps(buf, 0); if (fd == -1) return FALSE; if ((f = fdopen(fd, "w+")) == NULL) return FALSE; - fprintf(f, "file "%s"\n", wine_path); - fprintf(f, "target remote localhost:%d\n", ntohs(port)); - fprintf(f, "set prompt Wine-gdb>\ \n"); - /* gdb 5.1 seems to require it, won't hurt anyway */ - fprintf(f, "sharedlibrary\n"); - /* This is needed (but not a decent & final fix) - * Without this, gdb would skip our inter-DLL relay code (because - * we don't have any line number information for the relay code) - * With this, we will stop on first instruction of the stub, and - * reusing step, will get us through the relay stub at the actual - * function we're looking at. - */ - fprintf(f, "set step-mode on\n"); - /* tell gdb to delete this file when done handling it... */ - fprintf(f, "shell rm -f "%s"\n", buf); + if (strstr(gdb_path, "lldb")) + { + fprintf(f, "gdb-remote localhost:%d\n", ntohs(port)); + } + else + { + fprintf(f, "file "%s"\n", wine_path); + fprintf(f, "target remote localhost:%d\n", ntohs(port)); + fprintf(f, "set prompt Wine-gdb>\ \n"); + /* gdb 5.1 seems to require it, won't hurt anyway */ + fprintf(f, "sharedlibrary\n"); + /* This is needed (but not a decent & final fix) + * Without this, gdb would skip our inter-DLL relay code (because + * we don't have any line number information for the relay code) + * With this, we will stop on first instruction of the stub, and + * reusing step, will get us through the relay stub at the actual + * function we're looking at. + */ + fprintf(f, "set step-mode on\n"); + /* tell gdb to delete this file when done handling it... */ + fprintf(f, "shell rm -f "%s"\n", buf); + } fclose(f); if (flags & FLAG_WITH_XTERM) execlp("xterm", "xterm", "-e", gdb_path, "-x", buf, NULL); + else if (strstr(gdb_path, "lldb")) + execlp(gdb_path, gdb_path, "-S", buf, NULL); else execlp(gdb_path, gdb_path, "-x", buf, NULL); assert(0); /* never reached */
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- programs/winedbg/gdbproxy.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index f85e09353e2..5efaed0acee 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -120,6 +120,21 @@ static struct be_process_io be_process_gdbproxy_io = * =============================================== * */
+static int snscanf(char* src, size_t len, const char* format, ...) +{ + int n; + char c = src[len]; + va_list args; + + src[len] = '\0'; + va_start(args, format); + n = vsscanf(src, format, args); + va_end(args); + src[len] = c; + + return n; +} + static inline int hex_from0(char ch) { if (ch >= '0' && ch <= '9') return ch - '0'; @@ -1201,8 +1216,7 @@ static enum packet_return packet_read_memory(struct gdb_context* gdbctx) SIZE_T r = 0;
assert(gdbctx->in_trap); - /* FIXME:check in_packet_len for reading %p,%x */ - if (sscanf(gdbctx->in_packet, "%p,%x", &addr, &len) != 2) return packet_error; + if (snscanf(gdbctx->in_packet, gdbctx->in_packet_len, "%p,%x", &addr, &len) != 2) return packet_error; if (len <= 0) return packet_error; TRACE("Read %u bytes at %p\n", len, addr); for (nread = 0; nread < len; nread += r, addr += r) @@ -1240,7 +1254,7 @@ static enum packet_return packet_write_memory(struct gdb_context* gdbctx) } *ptr++ = '\0';
- if (sscanf(gdbctx->in_packet, "%p,%x", &addr, &len) != 2) + if (snscanf(gdbctx->in_packet, gdbctx->in_packet_len, "%p,%x", &addr, &len) != 2) { ERR("Failed to parse %s\n", debugstr_a(gdbctx->in_packet)); return packet_error;
Sometimes multiple packets are received and we were assuming it was some repeated requests due to slow ack. We can ack packets first.
Also, it was dropping some perfectly valid packets so we should process them all. This is for instance the case when using lldb as frontend.
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- programs/winedbg/gdbproxy.c | 148 ++++++++++++++++-------------------- 1 file changed, 65 insertions(+), 83 deletions(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index 5efaed0acee..2d3f1d0c0e7 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -1080,12 +1080,11 @@ static enum packet_return packet_verbose_cont(struct gdb_context* gdbctx) static enum packet_return packet_verbose(struct gdb_context* gdbctx) { if (gdbctx->in_packet_len >= 4 && !memcmp(gdbctx->in_packet, "Cont", 4)) - { return packet_verbose_cont(gdbctx); - }
- WARN("Unhandled verbose packet %s\n", - debugstr_an(gdbctx->in_packet, gdbctx->in_packet_len)); + if (gdbctx->in_packet_len == 14 && !memcmp(gdbctx->in_packet, "MustReplyEmpty", 14)) + return packet_reply(gdbctx, ""); + return packet_error; }
@@ -1761,94 +1760,77 @@ static struct packet_entry packet_entries[] =
static BOOL extract_packets(struct gdb_context* gdbctx) { - char* end; - int plen; - unsigned char in_cksum, loc_cksum; - char* ptr; - enum packet_return ret = packet_error; - int num_packet = 0; + char *ptr, *sum = gdbctx->in_buf, *end = gdbctx->in_buf + gdbctx->in_len; + enum packet_return ret = packet_error; + unsigned char cksum; + int i, len;
- while ((ret & packet_last_f) == 0) + while ((ptr = memchr(sum, '$', end - sum)) && + (sum = memchr(ptr, '#', end - ptr)) && + snscanf(sum, end - sum, "#%02x", &cksum) == 1) { - TRACE("Packet: %s\n", debugstr_an(gdbctx->in_buf, gdbctx->in_len)); - ptr = memchr(gdbctx->in_buf, '$', gdbctx->in_len); - if (ptr == NULL) return FALSE; - if (ptr != gdbctx->in_buf) + len = sum - ptr - 1; + sum += 3; + + if (cksum == checksum(ptr + 1, len)) { - int glen = ptr - gdbctx->in_buf; /* garbage len */ - WARN("Removing garbage: %s\n", debugstr_an(gdbctx->in_buf, glen)); - gdbctx->in_len -= glen; - memmove(gdbctx->in_buf, ptr, gdbctx->in_len); + TRACE("Acking: %s\n", debugstr_an(ptr, sum - ptr)); + write(gdbctx->sock, "+", 1); } - end = memchr(gdbctx->in_buf + 1, '#', gdbctx->in_len); - if (end == NULL) return FALSE; - /* no checksum yet */ - if (end + 3 > gdbctx->in_buf + gdbctx->in_len) return FALSE; - plen = end - gdbctx->in_buf - 1; - hex_from(&in_cksum, end + 1, 1); - loc_cksum = checksum(gdbctx->in_buf + 1, plen); - if (loc_cksum == in_cksum) + else { - if (num_packet == 0) { - int i; - - ret = packet_error; - - write(gdbctx->sock, "+", 1); - assert(plen); - - /* FIXME: should use bsearch if packet_entries was sorted */ - for (i = 0; i < ARRAY_SIZE(packet_entries); i++) - { - if (packet_entries[i].key == gdbctx->in_buf[1]) break; - } - if (i == ARRAY_SIZE(packet_entries)) - WARN("Unhandled packet %s\n", debugstr_an(&gdbctx->in_buf[1], plen)); - else - { - gdbctx->in_packet = gdbctx->in_buf + 2; - gdbctx->in_packet_len = plen - 1; - ret = (packet_entries[i].handler)(gdbctx); - } - switch (ret & ~packet_last_f) - { - case packet_error: packet_reply(gdbctx, ""); break; - case packet_ok: packet_reply(gdbctx, "OK"); break; - case packet_done: break; - } - TRACE("Reply: %s\n", debugstr_an(gdbctx->out_buf, gdbctx->out_len)); - i = write(gdbctx->sock, gdbctx->out_buf, gdbctx->out_len); - assert(i == gdbctx->out_len); - /* if this fails, we'll have to use POLLOUT... - */ - gdbctx->out_len = 0; - num_packet++; - } - else - { - /* FIXME: If we have more than one packet in our input buffer, - * it's very likely that we took too long to answer to a given packet - * and gdb is sending us the same packet again. - * So we simply drop the second packet. This will lower the risk of error, - * but there are still some race conditions here. - * A better fix (yet not perfect) would be to have two threads: - * - one managing the packets for gdb - * - the second one managing the commands... - * This would allow us to send the reply with the '+' character (Ack of - * the command) way sooner than we do now. - */ - ERR("Dropping packet; I was too slow to respond\n"); - } + ERR("Nacking: %s (checksum: %d != %d)\n", debugstr_an(ptr, sum - ptr), + cksum, checksum(ptr + 1, len)); + write(gdbctx->sock, "-", 1); } - else + } + + while ((ret & packet_last_f) == 0 && + (ptr = memchr(gdbctx->in_buf, '$', gdbctx->in_len)) && + (sum = memchr(ptr, '#', end - ptr)) && + snscanf(sum, end - sum, "#%02x", &cksum) == 1) + { + if (ptr != gdbctx->in_buf) + WARN("Ignoring: %s\n", debugstr_an(gdbctx->in_buf, ptr - gdbctx->in_buf)); + + len = sum - ptr - 1; + sum += 3; + + TRACE("Handling: %s\n", debugstr_an(ptr, sum - ptr)); + if (cksum == checksum(ptr + 1, len)) { - write(gdbctx->sock, "+", 1); - ERR("Dropping packet; invalid checksum %d <> %d\n", in_cksum, loc_cksum); + ret = packet_error; + gdbctx->in_packet = ptr + 2; + gdbctx->in_packet_len = len - 1; + + for (i = 0; i < ARRAY_SIZE(packet_entries); i++) + if (packet_entries[i].key == ptr[1]) + break; + + if (i == ARRAY_SIZE(packet_entries)) + WARN("Unhandled: %s\n", debugstr_an(ptr + 1, len)); + else if (((ret = (packet_entries[i].handler)(gdbctx)) & ~packet_last_f) == packet_error) + WARN("Failed: %s\n", debugstr_an(ptr + 1, len)); + + switch (ret & ~packet_last_f) + { + case packet_error: packet_reply(gdbctx, ""); break; + case packet_ok: packet_reply(gdbctx, "OK"); break; + case packet_done: break; + } + + TRACE("Reply: %s\n", debugstr_an(gdbctx->out_buf, gdbctx->out_len)); + i = write(gdbctx->sock, gdbctx->out_buf, gdbctx->out_len); + assert(i == gdbctx->out_len); + gdbctx->out_len = 0; } - gdbctx->in_len -= plen + 4; - memmove(gdbctx->in_buf, end + 3, gdbctx->in_len); + + gdbctx->in_len = end - sum; + memmove(gdbctx->in_buf, sum, end - sum); + end = gdbctx->in_buf + gdbctx->in_len; } - return TRUE; + + return (ret & packet_last_f); }
static int fetch_data(struct gdb_context* gdbctx)
There's a special packet_last_f flag to indicate we should quit, use that on kill packet instead of exiting abruptly.
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- programs/winedbg/gdbproxy.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index 2d3f1d0c0e7..e845485ae98 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -828,15 +828,12 @@ static inline void packet_reply_register_hex_to(struct gdb_context* gdbctx, unsi
static enum packet_return packet_reply_status(struct gdb_context* gdbctx) { - enum packet_return ret = packet_done; - - packet_reply_open(gdbctx); - if (gdbctx->process != NULL) { unsigned char sig; unsigned i;
+ packet_reply_open(gdbctx); packet_reply_add(gdbctx, "T"); sig = gdbctx->last_sig; packet_reply_val(gdbctx, sig, 1); @@ -854,19 +851,17 @@ static enum packet_return packet_reply_status(struct gdb_context* gdbctx) packet_reply_register_hex_to(gdbctx, i); packet_reply_add(gdbctx, ";"); } + + packet_reply_close(gdbctx); + return packet_done; } else { /* Try to put an exit code * Cannot use GetExitCodeProcess, wouldn't fit in a 8 bit value, so * just indicate the end of process and exit */ - packet_reply_add(gdbctx, "W00"); - /*if (!gdbctx->extended)*/ ret |= packet_last_f; + return packet_reply(gdbctx, "W00") | packet_last_f; } - - packet_reply_close(gdbctx); - - return ret; }
#if 0 @@ -1171,10 +1166,7 @@ static enum packet_return packet_kill(struct gdb_context* gdbctx) if (!gdbctx->extended) /* dunno whether GDB cares or not */ #endif - wait(NULL); - exit(0); - /* assume we can't really answer something here */ - /* return packet_done; */ + return packet_ok | packet_last_f; }
static enum packet_return packet_thread(struct gdb_context* gdbctx)
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- programs/winedbg/gdbproxy.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index e845485ae98..fdab2a9755f 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -775,6 +775,35 @@ static void packet_reply_close(struct gdb_context* gdbctx) gdbctx->out_curr_packet = -1; }
+static void packet_reply_open_xfer(struct gdb_context* gdbctx) +{ + packet_reply_open(gdbctx); + packet_reply_add(gdbctx, "m"); +} + +static void packet_reply_close_xfer(struct gdb_context* gdbctx, int off, int len) +{ + int begin = gdbctx->out_curr_packet + 1; + int plen; + + if (begin + off < gdbctx->out_len) + { + gdbctx->out_len -= off; + memmove(gdbctx->out_buf + begin, gdbctx->out_buf + begin + off, gdbctx->out_len); + } + else + { + gdbctx->out_buf[gdbctx->out_curr_packet] = 'l'; + gdbctx->out_len = gdbctx->out_curr_packet + 1; + } + + plen = gdbctx->out_len - begin; + if (len >= 0 && plen > len) gdbctx->out_len -= (plen - len); + else gdbctx->out_buf[gdbctx->out_curr_packet] = 'l'; + + packet_reply_close(gdbctx); +} + static enum packet_return packet_reply(struct gdb_context* gdbctx, const char* packet) { packet_reply_open(gdbctx);
The most important thing here is to report another osabi, so that gdb does not assume the target is GNU/Linux.
The register list is not strictly required for the gdb frontend but lldb can use it. Also it makes things more consistent.
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- programs/winedbg/gdbproxy.c | 185 +++++++++++++++++++++++++++++------- 1 file changed, 149 insertions(+), 36 deletions(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index fdab2a9755f..ca605259ec6 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -192,39 +192,150 @@ static unsigned char checksum(const char* ptr, int len) return cksum; }
+static const char target_xml[] = + "<target>" + "<osabi>Cygwin</osabi>" #ifdef __i386__ -static const char target_xml[] = ""; + "<architecture>i386</architecture>" + "<feature name="org.gnu.gdb.i386.core">" + "<reg name="eax" bitsize="32" type="uint32"/>" + "<reg name="ecx" bitsize="32" type="uint32"/>" + "<reg name="edx" bitsize="32" type="uint32"/>" + "<reg name="ebx" bitsize="32" type="uint32"/>" + "<reg name="esp" bitsize="32" type="uint32"/>" + "<reg name="ebp" bitsize="32" type="uint32"/>" + "<reg name="esi" bitsize="32" type="uint32"/>" + "<reg name="edi" bitsize="32" type="uint32"/>" + "<reg name="eip" bitsize="32" type="uint32"/>" + "<reg name="eflags" bitsize="32" type="uint32"/>" + "<reg name="cs" bitsize="32" type="uint32"/>" + "<reg name="ss" bitsize="32" type="uint32"/>" + "<reg name="ds" bitsize="32" type="uint32"/>" + "<reg name="es" bitsize="32" type="uint32"/>" + "<reg name="fs" bitsize="32" type="uint32"/>" + "<reg name="gs" bitsize="32" type="uint32"/>" + "<reg name="st0" bitsize="80" type="i387_ext"/>" + "<reg name="st1" bitsize="80" type="i387_ext"/>" + "<reg name="st2" bitsize="80" type="i387_ext"/>" + "<reg name="st3" bitsize="80" type="i387_ext"/>" + "<reg name="st4" bitsize="80" type="i387_ext"/>" + "<reg name="st5" bitsize="80" type="i387_ext"/>" + "<reg name="st6" bitsize="80" type="i387_ext"/>" + "<reg name="st7" bitsize="80" type="i387_ext"/>" + "<reg name="fctrl" bitsize="16" type="uint16"/>" + "<reg name="fstat" bitsize="16" type="uint16"/>" + "<reg name="ftag" bitsize="16" type="uint16"/>" + "<reg name="fiseg" bitsize="16" type="uint16"/>" + "<reg name="fioff" bitsize="32" type="uint32"/>" + "<reg name="foseg" bitsize="16" type="uint16"/>" + "<reg name="fooff" bitsize="32" type="uint32"/>" + "<reg name="fop" bitsize="16" type="uint16"/>" + "</feature>" + + "<feature name="org.gnu.gdb.i386.sse">" + "<reg name="xmm0" bitsize="128" type="uint128"/>" + "<reg name="xmm1" bitsize="128" type="uint128"/>" + "<reg name="xmm2" bitsize="128" type="uint128"/>" + "<reg name="xmm3" bitsize="128" type="uint128"/>" + "<reg name="xmm4" bitsize="128" type="uint128"/>" + "<reg name="xmm5" bitsize="128" type="uint128"/>" + "<reg name="xmm6" bitsize="128" type="uint128"/>" + "<reg name="xmm7" bitsize="128" type="uint128"/>" + "<reg name="mxcsr" bitsize="32" type="uint32"/>" + "</feature>" #elif defined(__powerpc__) -static const char target_xml[] = ""; + "<architecture>powerpc:common</architecture>" #elif defined(__x86_64__) -static const char target_xml[] = ""; + "<architecture>i386:x86-64</architecture>" + "<feature name="org.gnu.gdb.i386.core">" + "<reg name="rax" bitsize="64" type="uint64"/>" + "<reg name="rbx" bitsize="64" type="uint64"/>" + "<reg name="rcx" bitsize="64" type="uint64"/>" + "<reg name="rdx" bitsize="64" type="uint64"/>" + "<reg name="rsi" bitsize="64" type="uint64"/>" + "<reg name="rdi" bitsize="64" type="uint64"/>" + "<reg name="rbp" bitsize="64" type="uint64"/>" + "<reg name="rsp" bitsize="64" type="uint64"/>" + "<reg name="r8" bitsize="64" type="uint64"/>" + "<reg name="r9" bitsize="64" type="uint64"/>" + "<reg name="r10" bitsize="64" type="uint64"/>" + "<reg name="r11" bitsize="64" type="uint64"/>" + "<reg name="r12" bitsize="64" type="uint64"/>" + "<reg name="r13" bitsize="64" type="uint64"/>" + "<reg name="r14" bitsize="64" type="uint64"/>" + "<reg name="r15" bitsize="64" type="uint64"/>" + "<reg name="rip" bitsize="64" type="uint64"/>" + "<reg name="eflags" bitsize="32" type="uint32"/>" + "<reg name="cs" bitsize="32" type="uint32"/>" + "<reg name="ss" bitsize="32" type="uint32"/>" + "<reg name="ds" bitsize="32" type="uint32"/>" + "<reg name="es" bitsize="32" type="uint32"/>" + "<reg name="fs" bitsize="32" type="uint32"/>" + "<reg name="gs" bitsize="32" type="uint32"/>" + "<reg name="st0" bitsize="80" type="i387_ext"/>" + "<reg name="st1" bitsize="80" type="i387_ext"/>" + "<reg name="st2" bitsize="80" type="i387_ext"/>" + "<reg name="st3" bitsize="80" type="i387_ext"/>" + "<reg name="st4" bitsize="80" type="i387_ext"/>" + "<reg name="st5" bitsize="80" type="i387_ext"/>" + "<reg name="st6" bitsize="80" type="i387_ext"/>" + "<reg name="st7" bitsize="80" type="i387_ext"/>" + "<reg name="fctrl" bitsize="32" type="uint32"/>" + "<reg name="fstat" bitsize="32" type="uint32"/>" + "<reg name="ftag" bitsize="32" type="uint32"/>" + "<reg name="fiseg" bitsize="32" type="uint32"/>" + "<reg name="fioff" bitsize="32" type="uint32"/>" + "<reg name="foseg" bitsize="32" type="uint32"/>" + "<reg name="fooff" bitsize="32" type="uint32"/>" + "<reg name="fop" bitsize="32" type="uint32"/>" + "</feature>" + + "<feature name="org.gnu.gdb.i386.sse">" + "<reg name="xmm0" bitsize="128" type="uint128"/>" + "<reg name="xmm1" bitsize="128" type="uint128"/>" + "<reg name="xmm2" bitsize="128" type="uint128"/>" + "<reg name="xmm3" bitsize="128" type="uint128"/>" + "<reg name="xmm4" bitsize="128" type="uint128"/>" + "<reg name="xmm5" bitsize="128" type="uint128"/>" + "<reg name="xmm6" bitsize="128" type="uint128"/>" + "<reg name="xmm7" bitsize="128" type="uint128"/>" + "<reg name="xmm8" bitsize="128" type="uint128"/>" + "<reg name="xmm9" bitsize="128" type="uint128"/>" + "<reg name="xmm10" bitsize="128" type="uint128"/>" + "<reg name="xmm11" bitsize="128" type="uint128"/>" + "<reg name="xmm12" bitsize="128" type="uint128"/>" + "<reg name="xmm13" bitsize="128" type="uint128"/>" + "<reg name="xmm14" bitsize="128" type="uint128"/>" + "<reg name="xmm15" bitsize="128" type="uint128"/>" + "<reg name="mxcsr" bitsize="32" type="uint32"/>" + "</feature>" #elif defined(__arm__) -static const char target_xml[] = - "l <target><architecture>arm</architecture>\n" - "<feature name="org.gnu.gdb.arm.core">\n" - " <reg name="r0" bitsize="32" type="uint32"/>\n" - " <reg name="r1" bitsize="32" type="uint32"/>\n" - " <reg name="r2" bitsize="32" type="uint32"/>\n" - " <reg name="r3" bitsize="32" type="uint32"/>\n" - " <reg name="r4" bitsize="32" type="uint32"/>\n" - " <reg name="r5" bitsize="32" type="uint32"/>\n" - " <reg name="r6" bitsize="32" type="uint32"/>\n" - " <reg name="r7" bitsize="32" type="uint32"/>\n" - " <reg name="r8" bitsize="32" type="uint32"/>\n" - " <reg name="r9" bitsize="32" type="uint32"/>\n" - " <reg name="r10" bitsize="32" type="uint32"/>\n" - " <reg name="r11" bitsize="32" type="uint32"/>\n" - " <reg name="r12" bitsize="32" type="uint32"/>\n" - " <reg name="sp" bitsize="32" type="data_ptr"/>\n" - " <reg name="lr" bitsize="32"/>\n" - " <reg name="pc" bitsize="32" type="code_ptr"/>\n" - " <reg name="cpsr" bitsize="32"/>\n" - "</feature></target>\n"; + "<architecture>arm</architecture>" + "<feature name="org.gnu.gdb.arm.core">" + "<reg name="r0" bitsize="32" type="uint32"/>" + "<reg name="r1" bitsize="32" type="uint32"/>" + "<reg name="r2" bitsize="32" type="uint32"/>" + "<reg name="r3" bitsize="32" type="uint32"/>" + "<reg name="r4" bitsize="32" type="uint32"/>" + "<reg name="r5" bitsize="32" type="uint32"/>" + "<reg name="r6" bitsize="32" type="uint32"/>" + "<reg name="r7" bitsize="32" type="uint32"/>" + "<reg name="r8" bitsize="32" type="uint32"/>" + "<reg name="r9" bitsize="32" type="uint32"/>" + "<reg name="r10" bitsize="32" type="uint32"/>" + "<reg name="r11" bitsize="32" type="uint32"/>" + "<reg name="r12" bitsize="32" type="uint32"/>" + "<reg name="sp" bitsize="32" type="data_ptr"/>" + "<reg name="lr" bitsize="32"/>" + "<reg name="pc" bitsize="32" type="code_ptr"/>" + "<reg name="cpsr" bitsize="32"/>" + "</feature>" #elif defined(__aarch64__) -static const char target_xml[] = ""; + "<architecture>aarch64</architecture>" #else # error Define the registers map for your CPU #endif + "</target>";
static inline void* cpu_register_ptr(struct gdb_context *gdbctx, dbg_ctx_t *ctx, unsigned idx) @@ -1564,6 +1675,8 @@ static enum packet_return packet_query_remote_command(struct gdb_context* gdbctx
static enum packet_return packet_query(struct gdb_context* gdbctx) { + int off, len; + switch (gdbctx->in_packet[0]) { case 'f': @@ -1649,15 +1762,10 @@ static enum packet_return packet_query(struct gdb_context* gdbctx) return packet_ok; if (strncmp(gdbctx->in_packet, "Supported", 9) == 0) { - if (*target_xml) - return packet_reply(gdbctx, "PacketSize=400;qXfer:features:read+"); - else - { - /* no features supported */ - packet_reply_open(gdbctx); - packet_reply_close(gdbctx); - return packet_done; - } + packet_reply_open(gdbctx); + packet_reply_add(gdbctx, "qXfer:features:read+;"); + packet_reply_close(gdbctx); + return packet_done; } break; case 'T': @@ -1686,8 +1794,13 @@ static enum packet_return packet_query(struct gdb_context* gdbctx) } break; case 'X': - if (*target_xml && strncmp(gdbctx->in_packet, "Xfer:features:read:target.xml", 29) == 0) - return packet_reply(gdbctx, target_xml); + if (snscanf(gdbctx->in_packet, gdbctx->in_packet_len, "Xfer:features:read:target.xml:%x,%x", &off, &len) == 2) + { + packet_reply_open_xfer(gdbctx); + packet_reply_add(gdbctx, target_xml); + packet_reply_close_xfer(gdbctx, off, len); + return packet_done; + } break; } ERR("Unhandled query %s\n", debugstr_an(gdbctx->in_packet, gdbctx->in_packet_len));
On 1/27/20 1:07 PM, Rémi Bernon wrote:
The most important thing here is to report another osabi, so that gdb does not assume the target is GNU/Linux.
Looks like I was wrong about that, the file instruction makes it load the wine-loader first -not strictly required now with the library list- and gdb assumes from the start the ABI from the file. So this does nothing actually. Removing the file command makes it take this into account and it fails if Cygwin os support is not built in.
The "default" osabi appears to work fine.
I'd send an updated patch series or change this line in some later patch, but I'll wait for the global header changes to be reviewed so I'm not storming the testbot again.
This makes gdb frontend able to list loaded libraries so it can then parse debug information. It seems that it also supports PE/ELF mix, so this is nice. Sometimes debug info is still off, so not 100% accurate yet.
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- programs/winedbg/gdbproxy.c | 58 +++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index ca605259ec6..6e6ff558081 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -1673,6 +1673,55 @@ static enum packet_return packet_query_remote_command(struct gdb_context* gdbctx return packet_reply_error(gdbctx, EINVAL); }
+static BOOL CALLBACK packet_query_libraries_cb(PCSTR mod_name, DWORD64 base, PVOID ctx) +{ + struct gdb_context* gdbctx = ctx; + IMAGEHLP_MODULE64 imh_mod = {sizeof(imh_mod)}; + SymGetModuleInfo64(gdbctx->process->handle, base, &imh_mod); + + packet_reply_add(gdbctx, "<library name=""); + + if (strcmp(mod_name, "[vdso].so") == 0) + packet_reply_add(gdbctx, "linux-vdso.so.1"); + else if (imh_mod.LoadedImageName[0] == '/') + packet_reply_add(gdbctx, imh_mod.LoadedImageName); + else + { + UNICODE_STRING nt_name; + ANSI_STRING ansi_name; + char * unix_path; + + RtlInitAnsiString( &ansi_name, imh_mod.LoadedImageName ); + RtlAnsiStringToUnicodeString( &nt_name, &ansi_name, TRUE ); + + if ((unix_path = wine_get_unix_file_name( nt_name.Buffer ))) + packet_reply_add(gdbctx, unix_path); + else + packet_reply_add(gdbctx, mod_name); + + HeapFree( GetProcessHeap(), 0, unix_path ); + RtlFreeUnicodeString( &nt_name ); + } + + packet_reply_add(gdbctx, ""><segment address="0x"); + packet_reply_val(gdbctx, imh_mod.BaseOfImage, sizeof(imh_mod.BaseOfImage)); + packet_reply_add(gdbctx, ""/></library>"); + + return TRUE; +} + +static void packet_query_libraries(struct gdb_context* gdbctx) +{ + BOOL opt; + /* this will resynchronize builtin dbghelp's internal ELF module list */ + SymLoadModule(gdbctx->process->handle, 0, 0, 0, 0, 0); + packet_reply_add(gdbctx, "<library-list>"); + opt = SymSetExtendedOption(SYMOPT_EX_WINE_NATIVE_MODULES, TRUE); + SymEnumerateModules64(gdbctx->process->handle, packet_query_libraries_cb, gdbctx); + SymSetExtendedOption(SYMOPT_EX_WINE_NATIVE_MODULES, opt); + packet_reply_add(gdbctx, "</library-list>"); +} + static enum packet_return packet_query(struct gdb_context* gdbctx) { int off, len; @@ -1764,6 +1813,7 @@ static enum packet_return packet_query(struct gdb_context* gdbctx) { packet_reply_open(gdbctx); packet_reply_add(gdbctx, "qXfer:features:read+;"); + packet_reply_add(gdbctx, "qXfer:libraries:read+;"); packet_reply_close(gdbctx); return packet_done; } @@ -1801,6 +1851,14 @@ static enum packet_return packet_query(struct gdb_context* gdbctx) packet_reply_close_xfer(gdbctx, off, len); return packet_done; } + + if (snscanf(gdbctx->in_packet, gdbctx->in_packet_len, "Xfer:libraries:read::%x,%x", &off, &len) == 2) + { + packet_reply_open_xfer(gdbctx); + packet_query_libraries(gdbctx); + packet_reply_close_xfer(gdbctx, off, len); + return packet_done; + } break; } ERR("Unhandled query %s\n", debugstr_an(gdbctx->in_packet, gdbctx->in_packet_len));
For whatever reason, there's an 0x1000 error when gdb frontend computes addresses from debug info in PE files -it gets it right for ELFs.
Adding 0x1000 to BaseOfImage for PE files in packet_query_libraries_cb makes it correct and you get complete backtraces and accurate breakpoints... Now, I just need to figure out why.
As we don't report fork/vfork/exec events, this allows gdb to request the list of known threads.
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- programs/winedbg/gdbproxy.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index 6e6ff558081..ab8b9bdd07c 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -1722,6 +1722,24 @@ static void packet_query_libraries(struct gdb_context* gdbctx) packet_reply_add(gdbctx, "</library-list>"); }
+static void packet_query_threads(struct gdb_context* gdbctx) +{ + struct dbg_process* proc = gdbctx->process; + struct dbg_thread* thrd; + + packet_reply_add(gdbctx, "<threads>"); + LIST_FOR_EACH_ENTRY(thrd, &proc->threads, struct dbg_thread, entry) + { + packet_reply_add(gdbctx, "<thread "); + packet_reply_add(gdbctx, "id=""); + packet_reply_val(gdbctx, thrd->tid, 4); + packet_reply_add(gdbctx, "" name=""); + packet_reply_add(gdbctx, thrd->name); + packet_reply_add(gdbctx, ""/>"); + } + packet_reply_add(gdbctx, "</threads>"); +} + static enum packet_return packet_query(struct gdb_context* gdbctx) { int off, len; @@ -1814,6 +1832,7 @@ static enum packet_return packet_query(struct gdb_context* gdbctx) packet_reply_open(gdbctx); packet_reply_add(gdbctx, "qXfer:features:read+;"); packet_reply_add(gdbctx, "qXfer:libraries:read+;"); + packet_reply_add(gdbctx, "qXfer:threads:read+;"); packet_reply_close(gdbctx); return packet_done; } @@ -1859,6 +1878,14 @@ static enum packet_return packet_query(struct gdb_context* gdbctx) packet_reply_close_xfer(gdbctx, off, len); return packet_done; } + + if (snscanf(gdbctx->in_packet, gdbctx->in_packet_len, "Xfer:threads:read::%x,%x", &off, &len) == 2) + { + packet_reply_open_xfer(gdbctx); + packet_query_threads(gdbctx); + packet_reply_close_xfer(gdbctx, off, len); + return packet_done; + } break; } ERR("Unhandled query %s\n", debugstr_an(gdbctx->in_packet, gdbctx->in_packet_len));
We don't have to validate and acknowledge the packets as long as this mode is enabled, this will reduce verbosity especially when tracing.
Also lldb requests that upfront, and may get confused if we don't support it.
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- programs/winedbg/gdbproxy.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index ab8b9bdd07c..160c2df0d20 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -94,6 +94,7 @@ struct gdb_context struct dbg_process* process; /* Unix environment */ unsigned long wine_segs[3]; /* load addresses of the ELF wine exec segments (text, bss and data) */ + BOOL no_ack_mode; };
static BOOL tgt_process_gdbproxy_read(HANDLE hProcess, const void* addr, @@ -1830,6 +1831,7 @@ static enum packet_return packet_query(struct gdb_context* gdbctx) if (strncmp(gdbctx->in_packet, "Supported", 9) == 0) { packet_reply_open(gdbctx); + packet_reply_add(gdbctx, "QStartNoAckMode+;"); packet_reply_add(gdbctx, "qXfer:features:read+;"); packet_reply_add(gdbctx, "qXfer:libraries:read+;"); packet_reply_add(gdbctx, "qXfer:threads:read+;"); @@ -1892,6 +1894,17 @@ static enum packet_return packet_query(struct gdb_context* gdbctx) return packet_error; }
+static enum packet_return packet_set(struct gdb_context* gdbctx) +{ + if (strncmp(gdbctx->in_packet, "StartNoAckMode", 14) == 0) + { + gdbctx->no_ack_mode = TRUE; + return packet_reply(gdbctx, "OK"); + } + + return packet_error; +} + static enum packet_return packet_step(struct gdb_context* gdbctx) { /* FIXME: add support for address in packet */ @@ -1969,7 +1982,7 @@ static struct packet_entry packet_entries[] = {'p', packet_read_register}, {'P', packet_write_register}, {'q', packet_query}, - /* {'Q', packet_set}, */ + {'Q', packet_set}, /* {'R', packet,restart}, only in extended mode ! */ {'s', packet_step}, /*{'S', packet_step_signal}, hard(er) to implement */ @@ -1984,7 +1997,8 @@ static BOOL extract_packets(struct gdb_context* gdbctx) unsigned char cksum; int i, len;
- while ((ptr = memchr(sum, '$', end - sum)) && + while (!gdbctx->no_ack_mode && + (ptr = memchr(sum, '$', end - sum)) && (sum = memchr(ptr, '#', end - ptr)) && snscanf(sum, end - sum, "#%02x", &cksum) == 1) { @@ -2227,6 +2241,7 @@ static BOOL gdb_init_context(struct gdb_context* gdbctx, unsigned flags, unsigne gdbctx->last_sig = 0; gdbctx->in_trap = FALSE; gdbctx->process = NULL; + gdbctx->no_ack_mode = FALSE; for (i = 0; i < ARRAY_SIZE(gdbctx->wine_segs); i++) gdbctx->wine_segs[i] = 0;
This was using some conditional context read and dbg_curr_thread checks, which seems overcomplicated. Just read the context of the selected thread and write it back as needed.
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- programs/winedbg/gdbproxy.c | 96 ++++++++++++++++++------------------- 1 file changed, 47 insertions(+), 49 deletions(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index 160c2df0d20..d0e0cfb3428 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -501,6 +501,7 @@ static void handle_debug_event(struct gdb_context* gdbctx, DEBUG_EVENT* de) WCHAR buffer[256]; } u;
+ gdbctx->other_thread = NULL; dbg_curr_thread = dbg_get_thread(gdbctx->process, de->dwThreadId);
switch (de->dwDebugEventCode) @@ -941,16 +942,16 @@ static enum packet_return packet_reply_error(struct gdb_context* gdbctx, int err return packet_done; }
-static inline void packet_reply_register_hex_to(struct gdb_context* gdbctx, unsigned idx) +static inline void packet_reply_register_hex_to(struct gdb_context* gdbctx, dbg_ctx_t* ctx, unsigned idx) { const struct gdb_register *cpu_register_map = gdbctx->process->be_cpu->gdb_register_map;
if (cpu_register_map[idx].gdb_length == cpu_register_map[idx].ctx_length) - packet_reply_hex_to(gdbctx, cpu_register_ptr(gdbctx, &gdbctx->context, idx), + packet_reply_hex_to(gdbctx, cpu_register_ptr(gdbctx, ctx, idx), cpu_register_map[idx].gdb_length); else { - DWORD64 val = cpu_register(gdbctx, &gdbctx->context, idx); + DWORD64 val = cpu_register(gdbctx, ctx, idx); unsigned i;
for (i = 0; i < cpu_register_map[idx].gdb_length; i++) @@ -969,11 +970,18 @@ static inline void packet_reply_register_hex_to(struct gdb_context* gdbctx, unsi
static enum packet_return packet_reply_status(struct gdb_context* gdbctx) { - if (gdbctx->process != NULL) + struct dbg_process *proc = gdbctx->process; + dbg_ctx_t ctx; + + if (proc != NULL) { + struct backend_cpu *be = proc->be_cpu; unsigned char sig; unsigned i;
+ if (!be->get_context(dbg_curr_thread->handle, &ctx)) + return packet_error; + packet_reply_open(gdbctx); packet_reply_add(gdbctx, "T"); sig = gdbctx->last_sig; @@ -982,14 +990,11 @@ static enum packet_return packet_reply_status(struct gdb_context* gdbctx) packet_reply_val(gdbctx, dbg_curr_thread->tid, 4); packet_reply_add(gdbctx, ";");
- for (i = 0; i < gdbctx->process->be_cpu->gdb_num_regs; i++) + for (i = 0; i < be->gdb_num_regs; i++) { - /* FIXME: this call will also grow the buffer... - * unneeded, but not harmful - */ packet_reply_val(gdbctx, i, 1); packet_reply_add(gdbctx, ":"); - packet_reply_register_hex_to(gdbctx, i); + packet_reply_register_hex_to(gdbctx, &ctx, i); packet_reply_add(gdbctx, ";"); }
@@ -1251,20 +1256,18 @@ static enum packet_return packet_detach(struct gdb_context* gdbctx)
static enum packet_return packet_read_registers(struct gdb_context* gdbctx) { + struct dbg_thread *thrd = gdbctx->other_thread ? gdbctx->other_thread : dbg_curr_thread; + struct backend_cpu *be = thrd->process->be_cpu; int i; dbg_ctx_t ctx;
assert(gdbctx->in_trap); - - if (dbg_curr_thread != gdbctx->other_thread && gdbctx->other_thread) - { - if (!fetch_context(gdbctx, gdbctx->other_thread->handle, &ctx)) - return packet_error; - } + if (!thrd || !be->get_context(thrd->handle, &ctx)) + return packet_error;
packet_reply_open(gdbctx); - for (i = 0; i < gdbctx->process->be_cpu->gdb_num_regs; i++) - packet_reply_register_hex_to(gdbctx, i); + for (i = 0; i < be->gdb_num_regs; i++) + packet_reply_register_hex_to(gdbctx, &ctx, i);
packet_reply_close(gdbctx); return packet_done; @@ -1272,31 +1275,30 @@ static enum packet_return packet_read_registers(struct gdb_context* gdbctx)
static enum packet_return packet_write_registers(struct gdb_context* gdbctx) { - const size_t cpu_num_regs = gdbctx->process->be_cpu->gdb_num_regs; + struct dbg_thread *thrd = gdbctx->other_thread ? gdbctx->other_thread : dbg_curr_thread; + struct backend_cpu *be = thrd->process->be_cpu; + const size_t cpu_num_regs = be->gdb_num_regs; unsigned i; dbg_ctx_t ctx; - dbg_ctx_t *pctx = &gdbctx->context; const char* ptr;
assert(gdbctx->in_trap); - if (dbg_curr_thread != gdbctx->other_thread && gdbctx->other_thread) - { - if (!fetch_context(gdbctx, gdbctx->other_thread->handle, pctx = &ctx)) - return packet_error; - } - if (gdbctx->in_packet_len < cpu_num_regs * 2) return packet_error; + if (!thrd || !be->get_context(thrd->handle, &ctx)) + return packet_error; + if (gdbctx->in_packet_len < cpu_num_regs * 2) + return packet_error;
ptr = gdbctx->in_packet; for (i = 0; i < cpu_num_regs; i++) - cpu_register_hex_from(gdbctx, pctx, i, &ptr); + cpu_register_hex_from(gdbctx, &ctx, i, &ptr);
- if (pctx != &gdbctx->context && - !gdbctx->process->be_cpu->set_context(gdbctx->other_thread->handle, pctx)) + if (!be->set_context(thrd->handle, &ctx)) { ERR("Failed to set context for tid %04x, error %u\n", - gdbctx->other_thread->tid, GetLastError()); + thrd->tid, GetLastError()); return packet_error; } + return packet_ok; }
@@ -1414,42 +1416,42 @@ static enum packet_return packet_write_memory(struct gdb_context* gdbctx)
static enum packet_return packet_read_register(struct gdb_context* gdbctx) { + struct dbg_thread *thrd = gdbctx->other_thread ? gdbctx->other_thread : dbg_curr_thread; + struct backend_cpu *be = thrd->process->be_cpu; unsigned reg; dbg_ctx_t ctx; - dbg_ctx_t *pctx = &gdbctx->context;
assert(gdbctx->in_trap); reg = hex_to_int(gdbctx->in_packet, gdbctx->in_packet_len); - if (reg >= gdbctx->process->be_cpu->gdb_num_regs) + if (reg >= be->gdb_num_regs) { WARN("Unhandled register %u\n", reg); return packet_error; } - if (dbg_curr_thread != gdbctx->other_thread && gdbctx->other_thread) - { - if (!fetch_context(gdbctx, gdbctx->other_thread->handle, pctx = &ctx)) - return packet_error; - }
- TRACE("%u => %s\n", reg, wine_dbgstr_longlong(cpu_register(gdbctx, pctx, reg))); + if (!thrd || !be->get_context(thrd->handle, &ctx)) + return packet_error; + + TRACE("%u => %s\n", reg, wine_dbgstr_longlong(cpu_register(gdbctx, &ctx, reg)));
packet_reply_open(gdbctx); - packet_reply_register_hex_to(gdbctx, reg); + packet_reply_register_hex_to(gdbctx, &ctx, reg); packet_reply_close(gdbctx); return packet_done; }
static enum packet_return packet_write_register(struct gdb_context* gdbctx) { + struct dbg_thread *thrd = gdbctx->other_thread ? gdbctx->other_thread : dbg_curr_thread; + struct backend_cpu *be = thrd->process->be_cpu; unsigned reg; char* ptr; dbg_ctx_t ctx; - dbg_ctx_t *pctx = &gdbctx->context;
assert(gdbctx->in_trap);
reg = strtoul(gdbctx->in_packet, &ptr, 16); - if (ptr == NULL || reg >= gdbctx->process->be_cpu->gdb_num_regs || *ptr++ != '=') + if (ptr == NULL || reg >= be->gdb_num_regs || *ptr++ != '=') { WARN("Unhandled register %s\n", debugstr_an(gdbctx->in_packet, gdbctx->in_packet_len)); @@ -1462,18 +1464,14 @@ static enum packet_return packet_write_register(struct gdb_context* gdbctx) TRACE("%u <= %s\n", reg, debugstr_an(ptr, (int)(gdbctx->in_packet_len - (ptr - gdbctx->in_packet))));
- if (dbg_curr_thread != gdbctx->other_thread && gdbctx->other_thread) - { - if (!fetch_context(gdbctx, gdbctx->other_thread->handle, pctx = &ctx)) - return packet_error; - } + if (!thrd || !be->get_context(thrd->handle, &ctx)) + return packet_error;
- cpu_register_hex_from(gdbctx, pctx, reg, (const char**)&ptr); - if (pctx != &gdbctx->context && - !gdbctx->process->be_cpu->set_context(gdbctx->other_thread->handle, pctx)) + cpu_register_hex_from(gdbctx, &ctx, reg, (const char**)&ptr); + if (!be->set_context(thrd->handle, &ctx)) { ERR("Failed to set context for tid %04x, error %u\n", - gdbctx->other_thread->tid, GetLastError()); + thrd->tid, GetLastError()); return packet_error; }
There was some overcomplicated logic, we only need to iterate over the actions and apply them on the matching threads that didn't match yet.
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- programs/winedbg/debugger.h | 1 + programs/winedbg/gdbproxy.c | 180 ++++++++---------------------------- programs/winedbg/winedbg.c | 1 + 3 files changed, 40 insertions(+), 142 deletions(-)
diff --git a/programs/winedbg/debugger.h b/programs/winedbg/debugger.h index 40d93d669c8..ddac129bab5 100644 --- a/programs/winedbg/debugger.h +++ b/programs/winedbg/debugger.h @@ -207,6 +207,7 @@ struct dbg_thread }* frames; int num_frames; int curr_frame; + BOOL suspended; };
struct dbg_delayed_bp diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index d0e0cfb3428..eb550bdfe41 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -638,12 +638,12 @@ static void resume_debuggee(struct gdb_context* gdbctx, DWORD cont) }
-static void resume_debuggee_thread(struct gdb_context* gdbctx, DWORD cont, unsigned int threadid) +static void resume_debuggee_thread(struct gdb_context* gdbctx, DWORD cont, struct dbg_thread* thread) { - if (dbg_curr_thread) { - if(dbg_curr_thread->tid == threadid){ + if (dbg_curr_thread->tid == thread->tid) + { /* Windows debug and GDB don't seem to work well here, windows only likes ContinueDebugEvent being used on the reporter of the event */ if (!gdbctx->process->be_cpu->set_context(dbg_curr_thread->handle, &gdbctx->context)) ERR("Failed to set context for thread %04x, error %u\n", @@ -1038,39 +1038,14 @@ static enum packet_return packet_continue(struct gdb_context* gdbctx)
static enum packet_return packet_verbose_cont(struct gdb_context* gdbctx) { - int i; - int defaultAction = -1; /* magic non action */ - unsigned char sig; - int actions =0; - int actionIndex[20]; /* allow for up to 20 actions */ - int threadIndex[20]; - int threadCount = 0; - unsigned int threadIDs[100]; /* TODO: Should make this dynamic */ - unsigned int threadID = 0; - struct dbg_thread* thd; - - /* OK we have vCont followed by.. - * ? for query - * c for packet_continue - * Csig for packet_continue_signal - * s for step - * Ssig for step signal - * and then an optional thread ID at the end.. - * *******************************************/ + char *buf = gdbctx->in_packet, *end = gdbctx->in_packet + gdbctx->in_packet_len; + struct dbg_process *proc = gdbctx->process; + struct dbg_thread *thrd;
- /* Query */ if (gdbctx->in_packet[4] == '?') { - /* - Reply: - `vCont[;action]...' - The vCont packet is supported. Each action is a supported command in the vCont packet. - `' - The vCont packet is not supported. (this didn't seem to be obeyed!) - */ packet_reply_open(gdbctx); packet_reply_add(gdbctx, "vCont"); - /* add all the supported actions to the reply (all of them for now) */ packet_reply_add(gdbctx, ";c"); packet_reply_add(gdbctx, ";C"); packet_reply_add(gdbctx, ";s"); @@ -1079,138 +1054,59 @@ static enum packet_return packet_verbose_cont(struct gdb_context* gdbctx) return packet_done; }
- /* go through the packet and identify where all the actions start at */ - for (i = 4; i < gdbctx->in_packet_len - 1; i++) - { - if (gdbctx->in_packet[i] == ';') - { - threadIndex[actions] = 0; - actionIndex[actions++] = i; - } - else if (gdbctx->in_packet[i] == ':') - { - threadIndex[actions - 1] = i; - } - } + LIST_FOR_EACH_ENTRY(thrd, &proc->threads, struct dbg_thread, entry) + thrd->suspended = TRUE;
- /* now look up the default action */ - for (i = 0 ; i < actions; i++) + while (buf < end && (buf = memchr(buf + 1, ';', end - buf - 1))) { - if (threadIndex[i] == 0) - { - if (defaultAction != -1) - { - fprintf(stderr,"Too many default actions specified\n"); - return packet_error; - } - defaultAction = i; - } - } - - /* Now, I have this default action thing that needs to be applied to all non counted threads */ + int tid = -1, sig = 0; + int action, n;
- /* go through all the threads and stick their ids in the to be done list. */ - LIST_FOR_EACH_ENTRY(thd, &gdbctx->process->threads, struct dbg_thread, entry) - { - threadIDs[threadCount++] = thd->tid; - /* check to see if we have more threads than I counted on, and tell the user what to do - * (they're running winedbg, so I'm sure they can fix the problem from the error message!) */ - if (threadCount == 100) + switch ((action = buf[1])) { - fprintf(stderr, "Wow, that's a lot of threads, change threadIDs in wine/programs/winedbg/gdbproxy.c to be higher\n"); + default: + return packet_error; + case 'c': + case 's': + buf += 2; + break; + case 'C': + case 'S': + if (snscanf(buf, end - buf, ";%*c%2x", &sig) <= 0 || + sig != gdbctx->last_sig) + return packet_error; + buf += 4; break; } - }
- /* Ok, now we have... actionIndex full of actions and we know what threads there are, so all - * that remains is to apply the actions to the threads and the default action to any threads - * left */ - if (dbg_curr_thread != gdbctx->exec_thread && gdbctx->exec_thread) - FIXME("Can't continue thread %04x while on thread %04x\n", - gdbctx->exec_thread->tid, dbg_curr_thread->tid); + if (buf > end) + return packet_error; + if (buf < end && *buf == ':' && (n = snscanf(buf, end - buf, ":%x", &tid)) <= 0) + return packet_error;
- /* deal with the threaded stuff first */ - for (i = 0; i < actions ; i++) - { - if (threadIndex[i] != 0) + LIST_FOR_EACH_ENTRY(thrd, &proc->threads, struct dbg_thread, entry) { - int j, idLength = 0; - if (i < actions - 1) - { - idLength = (actionIndex[i+1] - threadIndex[i]) - 1; - } - else - { - idLength = (gdbctx->in_packet_len - threadIndex[i]) - 1; - } + if (tid != -1 && thrd->tid != tid) continue; + if (!thrd->suspended) continue; + thrd->suspended = FALSE;
- threadID = hex_to_int(gdbctx->in_packet + threadIndex[i] + 1 , idLength); - /* process the action */ - switch (gdbctx->in_packet[actionIndex[i] + 1]) + switch (action) { case 's': /* step */ gdbctx->process->be_cpu->single_step(&gdbctx->context, TRUE); - /* fall through*/ + /* fall through */ case 'c': /* continue */ - resume_debuggee_thread(gdbctx, DBG_CONTINUE, threadID); + resume_debuggee_thread(gdbctx, DBG_CONTINUE, thrd); break; - case 'S': /* step Sig, */ + case 'S': gdbctx->process->be_cpu->single_step(&gdbctx->context, TRUE); /* fall through */ case 'C': /* continue sig */ - hex_from(&sig, gdbctx->in_packet + actionIndex[i] + 2, 1); - /* cannot change signals on the fly */ - TRACE("sigs: %u %u\n", sig, gdbctx->last_sig); - if (sig != gdbctx->last_sig) - return packet_error; - resume_debuggee_thread(gdbctx, DBG_EXCEPTION_NOT_HANDLED, threadID); + resume_debuggee_thread(gdbctx, DBG_EXCEPTION_NOT_HANDLED, thrd); break; } - for (j = 0 ; j < threadCount; j++) - { - if (threadIDs[j] == threadID) - { - threadIDs[j] = 0; - break; - } - } - } - } /* for i=0 ; i< actions */ - - /* now we have manage the default action */ - if (defaultAction >= 0) - { - for (i = 0 ; i< threadCount; i++) - { - /* check to see if we've already done something to the thread*/ - if (threadIDs[i] != 0) - { - /* if not apply the default action*/ - threadID = threadIDs[i]; - /* process the action (yes this is almost identical to the one above!) */ - switch (gdbctx->in_packet[actionIndex[defaultAction] + 1]) - { - case 's': /* step */ - gdbctx->process->be_cpu->single_step(&gdbctx->context, TRUE); - /* fall through */ - case 'c': /* continue */ - resume_debuggee_thread(gdbctx, DBG_CONTINUE, threadID); - break; - case 'S': - gdbctx->process->be_cpu->single_step(&gdbctx->context, TRUE); - /* fall through */ - case 'C': /* continue sig */ - hex_from(&sig, gdbctx->in_packet + actionIndex[defaultAction] + 2, 1); - /* cannot change signals on the fly */ - TRACE("sigs: %u %u\n", sig, gdbctx->last_sig); - if (sig != gdbctx->last_sig) - return packet_error; - resume_debuggee_thread(gdbctx, DBG_EXCEPTION_NOT_HANDLED, threadID); - break; - } - } } - } /* if(defaultAction >=0) */ + }
wait_for_debuggee(gdbctx); if (gdbctx->process) diff --git a/programs/winedbg/winedbg.c b/programs/winedbg/winedbg.c index 5dee3b7e5ab..19f30f04e5d 100644 --- a/programs/winedbg/winedbg.c +++ b/programs/winedbg/winedbg.c @@ -500,6 +500,7 @@ struct dbg_thread* dbg_add_thread(struct dbg_process* p, DWORD tid, t->num_frames = 0; t->curr_frame = -1; t->addr_mode = AddrModeFlat; + t->suspended = FALSE;
snprintf(t->name, sizeof(t->name), "%04x", tid);
Using DBG_REPLY_LATER we can now continue/step any thread regardless of whether it is the one that raised the debug event.
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- programs/winedbg/debugger.h | 1 + programs/winedbg/gdbproxy.c | 185 ++++++++++++++++-------------------- 2 files changed, 85 insertions(+), 101 deletions(-)
diff --git a/programs/winedbg/debugger.h b/programs/winedbg/debugger.h index ddac129bab5..97218008283 100644 --- a/programs/winedbg/debugger.h +++ b/programs/winedbg/debugger.h @@ -208,6 +208,7 @@ struct dbg_thread int num_frames; int curr_frame; BOOL suspended; + DWORD continue_mode; };
struct dbg_delayed_bp diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index eb550bdfe41..e53ba2b965d 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -496,13 +496,18 @@ static BOOL handle_exception(struct gdb_context* gdbctx, EXCEPTION_DEBUG_INFO* e
static void handle_debug_event(struct gdb_context* gdbctx, DEBUG_EVENT* de) { + struct dbg_process *proc = gdbctx->process; + struct dbg_thread *thrd; + union { char bufferA[256]; WCHAR buffer[256]; } u;
+ gdbctx->exec_thread = NULL; gdbctx->other_thread = NULL; dbg_curr_thread = dbg_get_thread(gdbctx->process, de->dwThreadId); + if (dbg_curr_thread) dbg_curr_thread->continue_mode = DBG_REPLY_LATER;
switch (de->dwDebugEventCode) { @@ -510,6 +515,8 @@ static void handle_debug_event(struct gdb_context* gdbctx, DEBUG_EVENT* de) gdbctx->process = dbg_add_process(&be_process_gdbproxy_io, de->dwProcessId, de->u.CreateProcessInfo.hProcess); if (!gdbctx->process) break; + proc = gdbctx->process; + memory_get_string_indirect(gdbctx->process, de->u.CreateProcessInfo.lpImageName, de->u.CreateProcessInfo.fUnicode, @@ -620,41 +627,14 @@ static void handle_debug_event(struct gdb_context* gdbctx, DEBUG_EVENT* de) FIXME("%08x:%08x: unknown event (%u)\n", de->dwProcessId, de->dwThreadId, de->dwDebugEventCode); } -}
-static void resume_debuggee(struct gdb_context* gdbctx, DWORD cont) -{ - if (dbg_curr_thread) - { - if (!gdbctx->process->be_cpu->set_context(dbg_curr_thread->handle, &gdbctx->context)) - ERR("Failed to set context for thread %04x, error %u\n", - dbg_curr_thread->tid, GetLastError()); - if (!ContinueDebugEvent(gdbctx->process->pid, dbg_curr_thread->tid, cont)) - ERR("Failed to continue thread %04x, error %u\n", - dbg_curr_thread->tid, GetLastError()); - } - else - ERR("Cannot find last thread\n"); -} + if (!gdbctx->in_trap || !gdbctx->process) return;
- -static void resume_debuggee_thread(struct gdb_context* gdbctx, DWORD cont, struct dbg_thread* thread) -{ - if (dbg_curr_thread) + LIST_FOR_EACH_ENTRY(thrd, &proc->threads, struct dbg_thread, entry) { - if (dbg_curr_thread->tid == thread->tid) - { - /* Windows debug and GDB don't seem to work well here, windows only likes ContinueDebugEvent being used on the reporter of the event */ - if (!gdbctx->process->be_cpu->set_context(dbg_curr_thread->handle, &gdbctx->context)) - ERR("Failed to set context for thread %04x, error %u\n", - dbg_curr_thread->tid, GetLastError()); - if (!ContinueDebugEvent(gdbctx->process->pid, dbg_curr_thread->tid, cont)) - ERR("Failed to continue thread %04x, error %u\n", - dbg_curr_thread->tid, GetLastError()); - } + if (!thrd->suspended) SuspendThread(thrd->handle); + thrd->suspended = TRUE; } - else - ERR("Cannot find last thread\n"); }
static BOOL check_for_interrupt(struct gdb_context* gdbctx) @@ -688,6 +668,9 @@ static void wait_for_debuggee(struct gdb_context* gdbctx) { DEBUG_EVENT de;
+ if (dbg_curr_thread) + ContinueDebugEvent(dbg_curr_thread->process->pid, dbg_curr_thread->tid, dbg_curr_thread->continue_mode); + gdbctx->in_trap = FALSE; for (;;) { @@ -718,11 +701,47 @@ static void wait_for_debuggee(struct gdb_context* gdbctx) } }
+static void thread_set_single_step(struct dbg_thread *thrd, BOOL enable) +{ + struct dbg_process *proc = thrd->process; + struct backend_cpu *be = proc->be_cpu; + dbg_ctx_t ctx; + + if (!be->get_context(thrd->handle, &ctx)) + { + ERR("get_context failed for thread %04x:%04x\n", proc->pid, thrd->tid); + return; + } + be->single_step(&ctx, enable); + if (!be->set_context(thrd->handle, &ctx)) + ERR("set_context failed for thread %04x:%04x\n", proc->pid, thrd->tid); +} + +static void handle_step_or_continue(struct gdb_context* gdbctx, int tid, BOOL step, int sig) +{ + struct dbg_process *proc = gdbctx->process; + struct dbg_thread *thrd; + + if (tid == 0) tid = dbg_curr_thread->tid; + LIST_FOR_EACH_ENTRY(thrd, &proc->threads, struct dbg_thread, entry) + { + if (tid != -1 && thrd->tid != tid) continue; + if (!thrd->suspended) continue; + thrd->suspended = FALSE; + thrd->continue_mode = (sig == -1 ? DBG_CONTINUE : DBG_EXCEPTION_NOT_HANDLED); + + thread_set_single_step(thrd, step); + ResumeThread(thrd->handle); + } +} + static void detach_debuggee(struct gdb_context* gdbctx, BOOL kill) { - assert(gdbctx->process->be_cpu); - gdbctx->process->be_cpu->single_step(&gdbctx->context, FALSE); - resume_debuggee(gdbctx, DBG_CONTINUE); + handle_step_or_continue(gdbctx, -1, FALSE, -1); + + if (dbg_curr_thread) + ContinueDebugEvent(dbg_curr_thread->process->pid, dbg_curr_thread->tid, dbg_curr_thread->continue_mode); + if (!kill) DebugActiveProcessStop(gdbctx->process->pid); dbg_del_process(gdbctx->process); @@ -1026,12 +1045,13 @@ static enum packet_return packet_last_signal(struct gdb_context* gdbctx)
static enum packet_return packet_continue(struct gdb_context* gdbctx) { - /* FIXME: add support for address in packet */ - assert(gdbctx->in_packet_len == 0); - if (dbg_curr_thread != gdbctx->exec_thread && gdbctx->exec_thread) - FIXME("Can't continue thread %04x while on thread %04x\n", - gdbctx->exec_thread->tid, dbg_curr_thread->tid); - resume_debuggee(gdbctx, DBG_CONTINUE); + void *addr; + + if (snscanf(gdbctx->in_packet, gdbctx->in_packet_len, "%p", &addr) == 1) + FIXME("Continue at address %p not supported\n", addr); + + handle_step_or_continue(gdbctx, gdbctx->exec_thread ? gdbctx->exec_thread->tid : -1, FALSE, -1); + wait_for_debuggee(gdbctx); return packet_reply_status(gdbctx); } @@ -1039,8 +1059,6 @@ static enum packet_return packet_continue(struct gdb_context* gdbctx) static enum packet_return packet_verbose_cont(struct gdb_context* gdbctx) { char *buf = gdbctx->in_packet, *end = gdbctx->in_packet + gdbctx->in_packet_len; - struct dbg_process *proc = gdbctx->process; - struct dbg_thread *thrd;
if (gdbctx->in_packet[4] == '?') { @@ -1054,12 +1072,9 @@ static enum packet_return packet_verbose_cont(struct gdb_context* gdbctx) return packet_done; }
- LIST_FOR_EACH_ENTRY(thrd, &proc->threads, struct dbg_thread, entry) - thrd->suspended = TRUE; - while (buf < end && (buf = memchr(buf + 1, ';', end - buf - 1))) { - int tid = -1, sig = 0; + int tid = -1, sig = -1; int action, n;
switch ((action = buf[1])) @@ -1084,33 +1099,10 @@ static enum packet_return packet_verbose_cont(struct gdb_context* gdbctx) if (buf < end && *buf == ':' && (n = snscanf(buf, end - buf, ":%x", &tid)) <= 0) return packet_error;
- LIST_FOR_EACH_ENTRY(thrd, &proc->threads, struct dbg_thread, entry) - { - if (tid != -1 && thrd->tid != tid) continue; - if (!thrd->suspended) continue; - thrd->suspended = FALSE; - - switch (action) - { - case 's': /* step */ - gdbctx->process->be_cpu->single_step(&gdbctx->context, TRUE); - /* fall through */ - case 'c': /* continue */ - resume_debuggee_thread(gdbctx, DBG_CONTINUE, thrd); - break; - case 'S': - gdbctx->process->be_cpu->single_step(&gdbctx->context, TRUE); - /* fall through */ - case 'C': /* continue sig */ - resume_debuggee_thread(gdbctx, DBG_EXCEPTION_NOT_HANDLED, thrd); - break; - } - } + handle_step_or_continue(gdbctx, tid, action == 's' || action == 'S', sig); }
wait_for_debuggee(gdbctx); - if (gdbctx->process) - gdbctx->process->be_cpu->single_step(&gdbctx->context, FALSE); return packet_reply_status(gdbctx); }
@@ -1127,19 +1119,16 @@ static enum packet_return packet_verbose(struct gdb_context* gdbctx)
static enum packet_return packet_continue_signal(struct gdb_context* gdbctx) { - unsigned char sig; + void *addr; + int sig;
- /* FIXME: add support for address in packet */ - assert(gdbctx->in_packet_len == 2); - if (dbg_curr_thread != gdbctx->exec_thread && gdbctx->exec_thread) - FIXME("Can't continue thread %04x while on thread %04x\n", - gdbctx->exec_thread->tid, dbg_curr_thread->tid); - hex_from(&sig, gdbctx->in_packet, 1); - /* cannot change signals on the fly */ - TRACE("sigs: %u %u\n", sig, gdbctx->last_sig); + if (snscanf(gdbctx->in_packet, gdbctx->in_packet_len, "%x;%p", &sig, &addr) == 2) + FIXME("Continue at address %p not supported\n", addr); if (sig != gdbctx->last_sig) return packet_error; - resume_debuggee(gdbctx, DBG_EXCEPTION_NOT_HANDLED); + + handle_step_or_continue(gdbctx, gdbctx->exec_thread ? gdbctx->exec_thread->tid : -1, FALSE, sig); + wait_for_debuggee(gdbctx); return packet_reply_status(gdbctx); } @@ -1801,36 +1790,30 @@ static enum packet_return packet_set(struct gdb_context* gdbctx)
static enum packet_return packet_step(struct gdb_context* gdbctx) { - /* FIXME: add support for address in packet */ - assert(gdbctx->in_packet_len == 0); - if (dbg_curr_thread != gdbctx->exec_thread && gdbctx->exec_thread) - FIXME("Can't single-step thread %04x while on thread %04x\n", - gdbctx->exec_thread->tid, dbg_curr_thread->tid); - gdbctx->process->be_cpu->single_step(&gdbctx->context, TRUE); - resume_debuggee(gdbctx, DBG_CONTINUE); + void *addr; + + if (snscanf(gdbctx->in_packet, gdbctx->in_packet_len, "%p", &addr) == 1) + FIXME("Continue at address %p not supported\n", addr); + + handle_step_or_continue(gdbctx, gdbctx->exec_thread ? gdbctx->exec_thread->tid : -1, TRUE, -1); + wait_for_debuggee(gdbctx); - gdbctx->process->be_cpu->single_step(&gdbctx->context, FALSE); return packet_reply_status(gdbctx); }
#if 0 static enum packet_return packet_step_signal(struct gdb_context* gdbctx) { - unsigned char sig; - - /* FIXME: add support for address in packet */ - assert(gdbctx->in_packet_len == 2); - if (dbg_curr_thread->tid != gdbctx->exec_thread && gdbctx->exec_thread) - if (gdbctx->trace & GDBPXY_TRC_COMMAND_ERROR) - fprintf(stderr, "NIY: step/sig on %u, while last thread is %u\n", - gdbctx->exec_thread, DEBUG_CurrThread->tid); - hex_from(&sig, gdbctx->in_packet, 1); - /* cannot change signals on the fly */ - if (gdbctx->trace & GDBPXY_TRC_COMMAND) - fprintf(stderr, "sigs: %u %u\n", sig, gdbctx->last_sig); + void *addr; + int sig; + + if (snscanf(gdbctx->in_packet, gdbctx->in_packet_len, "%x;%p", &sig, &addr) == 2) + FIXME("Continue at address %p not supported\n", addr); if (sig != gdbctx->last_sig) return packet_error; - resume_debuggee(gdbctx, DBG_EXCEPTION_NOT_HANDLED); + + handle_step_or_continue(gdbctx, gdbctx->exec_thread ? gdbctx->exec_thread->tid : -1, TRUE, sig); + wait_for_debuggee(gdbctx); return packet_reply_status(gdbctx); }
Looking up the thread makes us loose track of any/all (0/-1) tids, we need that for correct continue/step implementation.
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- programs/winedbg/gdbproxy.c | 73 ++++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 34 deletions(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index e53ba2b965d..ccb33e870fd 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -84,8 +84,8 @@ struct gdb_context int out_len; int out_curr_packet; /* generic GDB thread information */ - struct dbg_thread* exec_thread; /* thread used in step & continue */ - struct dbg_thread* other_thread; /* thread to be used in any other operation */ + int exec_tid; /* tid used in step & continue */ + int other_tid; /* tid to be used in any other operation */ /* current Win32 trap env */ unsigned last_sig; BOOL in_trap; @@ -504,10 +504,13 @@ static void handle_debug_event(struct gdb_context* gdbctx, DEBUG_EVENT* de) WCHAR buffer[256]; } u;
- gdbctx->exec_thread = NULL; - gdbctx->other_thread = NULL; dbg_curr_thread = dbg_get_thread(gdbctx->process, de->dwThreadId); - if (dbg_curr_thread) dbg_curr_thread->continue_mode = DBG_REPLY_LATER; + if (dbg_curr_thread) + { + dbg_curr_thread->continue_mode = DBG_REPLY_LATER; + gdbctx->exec_tid = dbg_curr_thread->tid; + gdbctx->other_tid = dbg_curr_thread->tid; + }
switch (de->dwDebugEventCode) { @@ -593,8 +596,6 @@ static void handle_debug_event(struct gdb_context* gdbctx, DEBUG_EVENT* de) de->dwProcessId, de->dwThreadId, de->u.ExitThread.dwExitCode);
assert(dbg_curr_thread); - if (dbg_curr_thread == gdbctx->exec_thread) gdbctx->exec_thread = NULL; - if (dbg_curr_thread == gdbctx->other_thread) gdbctx->other_thread = NULL; dbg_del_thread(dbg_curr_thread); break;
@@ -735,6 +736,19 @@ static void handle_step_or_continue(struct gdb_context* gdbctx, int tid, BOOL st } }
+static struct dbg_thread* thread_from_tid(struct gdb_context* gdbctx, int tid) +{ + struct dbg_process *proc = gdbctx->process; + struct dbg_thread *thrd; + + LIST_FOR_EACH_ENTRY(thrd, &proc->threads, struct dbg_thread, entry) + { + if (tid > 0 && thrd->tid != tid) continue; + return thrd; + } + return dbg_curr_thread; +} + static void detach_debuggee(struct gdb_context* gdbctx, BOOL kill) { handle_step_or_continue(gdbctx, -1, FALSE, -1); @@ -1050,7 +1064,7 @@ static enum packet_return packet_continue(struct gdb_context* gdbctx) if (snscanf(gdbctx->in_packet, gdbctx->in_packet_len, "%p", &addr) == 1) FIXME("Continue at address %p not supported\n", addr);
- handle_step_or_continue(gdbctx, gdbctx->exec_thread ? gdbctx->exec_thread->tid : -1, FALSE, -1); + handle_step_or_continue(gdbctx, gdbctx->exec_tid, FALSE, -1);
wait_for_debuggee(gdbctx); return packet_reply_status(gdbctx); @@ -1127,7 +1141,7 @@ static enum packet_return packet_continue_signal(struct gdb_context* gdbctx) if (sig != gdbctx->last_sig) return packet_error;
- handle_step_or_continue(gdbctx, gdbctx->exec_thread ? gdbctx->exec_thread->tid : -1, FALSE, sig); + handle_step_or_continue(gdbctx, gdbctx->exec_tid, FALSE, sig);
wait_for_debuggee(gdbctx); return packet_reply_status(gdbctx); @@ -1141,7 +1155,7 @@ static enum packet_return packet_detach(struct gdb_context* gdbctx)
static enum packet_return packet_read_registers(struct gdb_context* gdbctx) { - struct dbg_thread *thrd = gdbctx->other_thread ? gdbctx->other_thread : dbg_curr_thread; + struct dbg_thread *thrd = thread_from_tid(gdbctx, gdbctx->other_tid); struct backend_cpu *be = thrd->process->be_cpu; int i; dbg_ctx_t ctx; @@ -1160,7 +1174,7 @@ static enum packet_return packet_read_registers(struct gdb_context* gdbctx)
static enum packet_return packet_write_registers(struct gdb_context* gdbctx) { - struct dbg_thread *thrd = gdbctx->other_thread ? gdbctx->other_thread : dbg_curr_thread; + struct dbg_thread *thrd = thread_from_tid(gdbctx, gdbctx->other_tid); struct backend_cpu *be = thrd->process->be_cpu; const size_t cpu_num_regs = be->gdb_num_regs; unsigned i; @@ -1199,28 +1213,18 @@ static enum packet_return packet_kill(struct gdb_context* gdbctx)
static enum packet_return packet_thread(struct gdb_context* gdbctx) { - char* end; - unsigned thread; - switch (gdbctx->in_packet[0]) { case 'c': + if (snscanf(gdbctx->in_packet, gdbctx->in_packet_len, "c%x", &gdbctx->exec_tid) == 1) + return packet_ok; + gdbctx->exec_tid = -1; + return packet_error; case 'g': - if (gdbctx->in_packet[1] == '-') - thread = -strtol(gdbctx->in_packet + 2, &end, 16); - else - thread = strtol(gdbctx->in_packet + 1, &end, 16); - if (end == NULL || end > gdbctx->in_packet + gdbctx->in_packet_len) - { - ERR("Failed to parse %s\n", - debugstr_an(gdbctx->in_packet, gdbctx->in_packet_len)); - return packet_error; - } - if (gdbctx->in_packet[0] == 'c') - gdbctx->exec_thread = dbg_get_thread(gdbctx->process, thread); - else - gdbctx->other_thread = dbg_get_thread(gdbctx->process, thread); - return packet_ok; + if (snscanf(gdbctx->in_packet, gdbctx->in_packet_len, "g%x", &gdbctx->other_tid) == 1) + return packet_ok; + gdbctx->other_tid = -1; + return packet_error; default: FIXME("Unknown thread sub-command %c\n", gdbctx->in_packet[0]); return packet_error; @@ -1301,7 +1305,7 @@ static enum packet_return packet_write_memory(struct gdb_context* gdbctx)
static enum packet_return packet_read_register(struct gdb_context* gdbctx) { - struct dbg_thread *thrd = gdbctx->other_thread ? gdbctx->other_thread : dbg_curr_thread; + struct dbg_thread *thrd = thread_from_tid(gdbctx, gdbctx->other_tid); struct backend_cpu *be = thrd->process->be_cpu; unsigned reg; dbg_ctx_t ctx; @@ -1327,7 +1331,7 @@ static enum packet_return packet_read_register(struct gdb_context* gdbctx)
static enum packet_return packet_write_register(struct gdb_context* gdbctx) { - struct dbg_thread *thrd = gdbctx->other_thread ? gdbctx->other_thread : dbg_curr_thread; + struct dbg_thread *thrd = thread_from_tid(gdbctx, gdbctx->other_tid); struct backend_cpu *be = thrd->process->be_cpu; unsigned reg; char* ptr; @@ -1795,7 +1799,7 @@ static enum packet_return packet_step(struct gdb_context* gdbctx) if (snscanf(gdbctx->in_packet, gdbctx->in_packet_len, "%p", &addr) == 1) FIXME("Continue at address %p not supported\n", addr);
- handle_step_or_continue(gdbctx, gdbctx->exec_thread ? gdbctx->exec_thread->tid : -1, TRUE, -1); + handle_step_or_continue(gdbctx, gdbctx->exec_tid, TRUE, -1);
wait_for_debuggee(gdbctx); return packet_reply_status(gdbctx); @@ -1812,7 +1816,7 @@ static enum packet_return packet_step_signal(struct gdb_context* gdbctx) if (sig != gdbctx->last_sig) return packet_error;
- handle_step_or_continue(gdbctx, gdbctx->exec_thread ? gdbctx->exec_thread->tid : -1, TRUE, sig); + handle_step_or_continue(gdbctx, gdbctx->exec_tid, TRUE, sig);
wait_for_debuggee(gdbctx); return packet_reply_status(gdbctx); @@ -2114,7 +2118,8 @@ static BOOL gdb_init_context(struct gdb_context* gdbctx, unsigned flags, unsigne gdbctx->out_len = 0; gdbctx->out_curr_packet = -1;
- gdbctx->exec_thread = gdbctx->other_thread = NULL; + gdbctx->exec_tid = -1; + gdbctx->other_tid = -1; gdbctx->last_sig = 0; gdbctx->in_trap = FALSE; gdbctx->process = NULL;
I've cancelled the remaining testbot jobs that were running ntdll tests on every patch of the series because it took way to long.
-
Jacek Caban -
Rémi Bernon