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

[Hannes Domani <ssbssa@yahoo.de>]

 Am Freitag, 18. September 2020, 17:48:58 MESZ hat Simon Marchi <simark@simark.ca> Folgendes geschrieben:

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

OK, I will look into this (even though we maybe won't need it).


> > 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)?

Yes, this would replace the remote thread.


Hannes