Re: [PATCH] Fix ctrl-c when debugging WOW64 processes

[Simon Marchi <simark@simark.ca>]

On 2020-09-18 10:27 a.m., Hannes Domani wrote:
> I did some experiments.
>
> Seems like GenerateConsoleCtrlEvent() only works if the target process was
> created without CREATE_NEW_CONSOLE.
> And what's worse, it seems to always return TRUE, so it never reaches
> DebugBreakProcess(), even if the target process doesn't have a console at all.
>
> I also tried the "last resort", with soft_interrupt_requested=1, but this
> immediatly crashed gdbserver.
>
> I fixed this crash with:
>
> --- a/gdbserver/win32-low.cc
> +++ b/gdbserver/win32-low.cc
> @@ -1339,6 +1335,7 @@ fake_breakpoint_event (void)
>    faked_breakpoint = 1;
>
>    memset (&current_event, 0, sizeof (current_event));
> +  current_event.dwProcessId = current_process_id;
>    current_event.dwThreadId = main_thread_id;
>    current_event.dwDebugEventCode = EXCEPTION_DEBUG_EVENT;
>    current_event.u.Exception.ExceptionRecord.ExceptionCode
>
> But this didn't work either, I think it's because gdb checks if the current
> instruction is int3, and continues if not.
>
> I'm wondering why this last resort was added, was this done for pre-XP where
> DebugBreakProcess() didn't exist?

I digged a little bit.  It was introduced by this commit by a young
Pedro :)

  https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=4d5d1aaa19ea

The corresponding mailing list post is:

  https://sourceware.org/legacy-ml/gdb-patches/2007-11/msg00217.html

It doesn't explain the rationale though, it just implicitly refers to a
previous thread.  It's probably this one, which gives a bit more
context:

  https://sourceware.org/legacy-ml/gdb-patches/2007-11/msg00035.html

From what I understand, this was for WinCE, which lacked
GenerateConsoleCtrlEvent and DebugBreakProcess.  Since we removed
support for WinCE, I think that code could very well be GC'ed.

>
> So right now ctrl-c doesn't really work that well with gdbserver, and not
> at all when debugging a program without console (WOW64 or not doesn't matter).
>
>
> And the WOW64 fix for DbgUiRemoteBreakin() probably can't be used in
> gdbserver, because find_minimal_symbol_address() is not available.

Using the qSymbol packet, there is a window during which GDBserver can
as GDB to look up minimal symbol values.  On the GDBserver side, this is
done through process_stratum_target::look_up_symbols, you would just
need to implement it for win32_process_target.

> Meanwhile I came up with a possible alternative for DbgUiRemoteBreakin():
>
> --- a/gdb/nat/windows-nat.c
> +++ b/gdb/nat/windows-nat.c
> @@ -240,6 +241,13 @@ handle_exception (struct target_waitstatus *ourstatus, bool debug_exceptions)
>      case EXCEPTION_BREAKPOINT:
>  #ifdef __x86_64__
>        if (ignore_first_breakpoint)
>      {
>        /* For WOW64 processes, there are always 2 breakpoint exceptions
>           on startup, first a BREAKPOINT for the 64bit ntdll.dll,
>           then a WX86_BREAKPOINT for the 32bit ntdll.dll.
>           Here we only care about the WX86_BREAKPOINT's.  */
>            ourstatus->kind = TARGET_WAITKIND_SPURIOUS;
>           ignore_first_breakpoint = false;
>         }
> +      else if (wow64_process)
> +       {
> +         DEBUG_EXCEPTION_SIMPLE ("EXCEPTION_BREAKPOINT");
> +         rec->ExceptionCode = DBG_CONTROL_C;
> +         ourstatus->value.sig = GDB_SIGNAL_INT;
> +         break;
> +       }
>  #endif
>        /* FALLTHROUGH */
>      case STATUS_WX86_BREAKPOINT:
>
> This transforms the (64bit) breakpoint from DebugBreakProcess() into
> SIGINT, then the check for the int3 instruction is not done, and gdb stops
> the target process in any case.

That sounds simple (which is good).  Would this replace completely
spawning the remote thread (which you added in the previous patch)?

Simon