Re: [PATCH v2] Implement debugging of WOW64 processes in gdbserver

[Hannes Domani <ssbssa@yahoo.de>]

 Am Dienstag, 28. April 2020, 17:53:15 MESZ hat Simon Marchi <simark@simark.ca> Folgendes geschrieben:

> On 2020-04-27 9:34 a.m., Hannes Domani via Gdb-patches wrote:
> > @@ -496,7 +601,7 @@ i386_win32_set_pc (struct regcache *regcache, CORE_ADDR pc)
> >
> >  struct win32_target_ops the_low_target = {
> >    i386_arch_setup,
> > -  sizeof (mappings) / sizeof (mappings[0]),
> > +  i386_win32_num_regs,
> >    i386_initial_stuff,
> >    i386_get_thread_context,
> >    i386_prepare_to_resume,
> > diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
> > index 5a6f0df39f..d3fc31914d 100644
> > --- a/gdbserver/win32-low.cc
> > +++ b/gdbserver/win32-low.cc
> > @@ -88,15 +88,32 @@ static int soft_interrupt_requested = 0;
> >    by suspending all the threads.  */
> >  static int faked_breakpoint = 0;
> >
> > +#ifdef __x86_64__
> > +bool wow64_process = false;
> > +#endif
> > +
> >  const struct target_desc *win32_tdesc;
> > +#ifdef __x86_64__
> > +const struct target_desc *wow64_win32_tdesc;
> > +#endif
> >
> > -#define NUM_REGS (the_low_target.num_regs)
> > +#define NUM_REGS (the_low_target.num_regs ())
> >
> >  typedef BOOL (WINAPI *winapi_DebugActiveProcessStop) (DWORD dwProcessId);
> >  typedef BOOL (WINAPI *winapi_DebugSetProcessKillOnExit) (BOOL KillOnExit);
> >  typedef BOOL (WINAPI *winapi_DebugBreakProcess) (HANDLE);
> >  typedef BOOL (WINAPI *winapi_GenerateConsoleCtrlEvent) (DWORD, DWORD);
> >
> > +#ifdef __x86_64__
> > +typedef DWORD (WINAPI *winapi_Wow64SuspendThread) (HANDLE);
> > +typedef BOOL (WINAPI *winapi_Wow64SetThreadContext) (HANDLE,
> > +                            const WOW64_CONTEXT *);
> > +
> > +static winapi_Wow64SuspendThread win32_Wow64SuspendThread;
>
> win32_Wow64SuspendThread is never actually used, is this expected?

Well, in this case, kinda.
I only noticed afterwards that the same Wow64SuspendThread was unused in the
gdb equivalent, because unexpectedly, SuspendThread does work (at least on
my PC).

But I will remove it from this patch, and will try to fix it for both
gdb & gdbserver with a later patch.


> > @@ -195,7 +231,14 @@ child_add_thread (DWORD pid, DWORD tid, HANDLE h, void *tlb)
> >    if ((th = thread_rec (ptid, DONT_INVALIDATE_CONTEXT)))
> >      return th;
> >
> > -  th = new windows_thread_info (tid, h, (CORE_ADDR) (uintptr_t) tlb);
> > +  CORE_ADDR base = (CORE_ADDR) (uintptr_t) tlb;
> > +#ifdef __x86_64__
> > +  /* For WOW64 processes, this is actually the pointer to the 64bit TIB,
> > +    and the 32bit TIB is exactly 2 pages after it.  */
>
> Do you have a reference for this, ideally from MSDN?  If so, it would be nice to
> include the link in the comment.

Sadly no, I could find no official document for this.

There is an alternative way for to get the 32bit TIB, via the address at
offset 0 of the 64bit TIB:
https://docs.microsoft.com/en-us/windows/win32/debug/thread-environment-block--debugging-notes-

But even here it is mentioned that this may change in later Windows versions.


> > +  if (wow64_process)
> > +    base += 0x2000;
>
> Could you make it a PAGE_SIZE macro, and wirte `2 * PAGE_SIZE` here?  It would
> make it more obvious that it matches the comment above.

OK.


> > +#endif
> > +  th = new windows_thread_info (tid, h, base);
> >
> >    add_thread (ptid, th);
> >
> > @@ -345,8 +388,27 @@ do_initial_child_stuff (HANDLE proch, DWORD pid, int attached)
> >
> >    memset (&current_event, 0, sizeof (current_event));
> >
> > +#ifdef __x86_64__
> > +  BOOL wow64;
> > +  if (IsWow64Process (proch, &wow64))
> > +    wow64_process = wow64;
>
> If this function fails, it means we couldn't get the information we requested.  In
> this case, I think we should error out, because we can't continue reliably.  So,
> something like:
>
> BOOL wow64;
> if (!IsWow64Process (proch, &wow64_process))
>   {
>     // Call error() with a message formatted from GetLastError().
>   }

OK.


> > @@ -1096,13 +1197,53 @@ win32_add_all_dlls (void)
> >    if (!DllHandle)
> >      return;
> >
> > -  ok = (*win32_EnumProcessModules) (current_process_handle,
> > -                    DllHandle,
> > -                    cbNeeded,
> > -                    &cbNeeded);
> > +#ifdef __x86_64__
> > +  if (wow64_process)
> > +    ok = (*win32_EnumProcessModulesEx) (current_process_handle,
> > +                    DllHandle,
> > +                    cbNeeded,
> > +                    &cbNeeded,
> > +                    LIST_MODULES_32BIT);
> > +  else
> > +#endif
> > +    ok = (*win32_EnumProcessModules) (current_process_handle,
> > +                      DllHandle,
> > +                      cbNeeded,
> > +                      &cbNeeded);
> >    if (!ok)
> >      return;
> >
> > +  char system_dir[MAX_PATH];
> > +  char syswow_dir[MAX_PATH];
> > +  size_t system_dir_len = 0;
> > +  bool convert_syswow_dir = false;
> > +#ifdef __x86_64__
> > +  if (wow64_process)
> > +#endif
> > +    {
> > +      /* This fails on 32bit Windows because it has no SysWOW64 directory,
> > +    and in this case a path conversion isn't necessary.  */
> > +      UINT len = GetSystemWow64DirectoryA (syswow_dir, sizeof (syswow_dir));
> > +      if (len > 0)
> > +    {
> > +      /* Check that we have passed a large enough buffer.  */
> > +      gdb_assert (len < sizeof (syswow_dir));
> > +
> > +      len = GetSystemDirectoryA (system_dir, sizeof (system_dir));
> > +      /* Error check.  */
> > +      gdb_assert (len != 0);
> > +      /* Check that we have passed a large enough buffer.  */
> > +      gdb_assert (len < sizeof (system_dir));
> > +
> > +      strcat (system_dir, "\\");
> > +      strcat (syswow_dir, "\\");
> > +      system_dir_len = strlen (system_dir);
> > +
> > +      convert_syswow_dir = true;
> > +    }
> > +
> > +    }
>
> Since this is copied from gdb/windows-nat.c, it would be nice to factor it
> out (as well as the small snippet below), so it can be shared.  Otherwise,
> if we make a fix to one, we'll most likely forget to fix the other.
>
> The shared versions would go into gdb/nat/windows-nat.c.

There might be even more code that can be shared now, I planned to do this
with a later patch.


> > +
> >    for (i = 1; i < ((size_t) cbNeeded / sizeof (HMODULE)); i++)
> >      {
> >        MODULEINFO mi;
> > @@ -1118,7 +1259,22 @@ win32_add_all_dlls (void)
> >                      dll_name,
> >                      MAX_PATH) == 0)
> >      continue;
> > -      win32_add_one_solib (dll_name, (CORE_ADDR) (uintptr_t) mi.lpBaseOfDll);
> > +
> > +      const char *name = dll_name;
> > +      /* Convert the DLL path of 32bit processes returned by
> > +    GetModuleFileNameEx from the 64bit system directory to the
> > +    32bit syswow64 directory if necessary.  */
> > +      std::string syswow_dll_path;
> > +      if (convert_syswow_dir
> > +      && strncasecmp (dll_name, system_dir, system_dir_len) == 0
> > +      && strchr (dll_name + system_dir_len, '\\') == nullptr)
> > +    {
> > +      syswow_dll_path = syswow_dir;
> > +      syswow_dll_path += dll_name + system_dir_len;
> > +      name = syswow_dll_path.c_str();
> > +    }
> > +
> > +      win32_add_one_solib (name, (CORE_ADDR) (uintptr_t) mi.lpBaseOfDll);
> >      }
> >  }
> >  #endif
> > @@ -1221,8 +1377,10 @@ maybe_adjust_pc ()
> >    th->stopped_at_software_breakpoint = false;
> >
> >    if (current_event.dwDebugEventCode == EXCEPTION_DEBUG_EVENT
> > -      && (current_event.u.Exception.ExceptionRecord.ExceptionCode
> > -      == EXCEPTION_BREAKPOINT)
> > +      && ((current_event.u.Exception.ExceptionRecord.ExceptionCode
> > +      == EXCEPTION_BREAKPOINT)
> > +      || (current_event.u.Exception.ExceptionRecord.ExceptionCode
> > +          == STATUS_WX86_BREAKPOINT))
> >        && child_initialization_done)
> >      {
> >        th->stopped_at_software_breakpoint = true;
> > @@ -1684,13 +1842,34 @@ win32_process_target::qxfer_siginfo (const char *annex,
> >    if (readbuf == nullptr)
> >      return -1;
> >
> > -  if (offset > sizeof (siginfo_er))
> > +  char *buf = (char *) &siginfo_er;
> > +  size_t bufsize = sizeof (siginfo_er);
> > +
> > +#ifdef __x86_64__
> > +  EXCEPTION_RECORD32 er32;
> > +  if (wow64_process)
> > +    {
> > +      buf = (char *) &er32;
> > +      bufsize = sizeof (er32);
> > +
> > +      er32.ExceptionCode = siginfo_er.ExceptionCode;
> > +      er32.ExceptionFlags = siginfo_er.ExceptionFlags;
> > +      er32.ExceptionRecord = (uintptr_t) siginfo_er.ExceptionRecord;
> > +      er32.ExceptionAddress = (uintptr_t) siginfo_er.ExceptionAddress;
> > +      er32.NumberParameters = siginfo_er.NumberParameters;
> > +      int i;
> > +      for (i = 0; i < EXCEPTION_MAXIMUM_PARAMETERS; i++)
> > +    er32.ExceptionInformation[i] = siginfo_er.ExceptionInformation[i];
> > +    }
> > +#endif
> > +
> > +  if (offset > bufsize)
> >      return -1;
> >
> > -  if (offset + len > sizeof (siginfo_er))
> > -    len = sizeof (siginfo_er) - offset;
> > +  if (offset + len > bufsize)
> > +    len = bufsize - offset;
> >
> > -  memcpy (readbuf, (char *) &siginfo_er + offset, len);
> > +  memcpy (readbuf, buf + offset, len);
> >
> >    return len;
> >  }
> > @@ -1760,4 +1939,11 @@ initialize_low (void)
> >  {
> >    set_target_ops (&the_win32_target);
> >    the_low_target.arch_setup ();
> > +
> > +#ifdef __x86_64__
> > +  HMODULE dll = GetModuleHandle (_T("KERNEL32.DLL"));
> > +  win32_Wow64SuspendThread = GETPROCADDRESS (dll, Wow64SuspendThread);
> > +  win32_Wow64GetThreadContext = GETPROCADDRESS (dll, Wow64GetThreadContext);
> > +  win32_Wow64SetThreadContext = GETPROCADDRESS (dll, Wow64SetThreadContext);
>
> Why do we need to get these functions like this, instead of just calling them?

These functions are not available on WinXP, and the extra check for them
was actually the reason of this v2:

+  if (wow64_process
+      && (win32_Wow64SuspendThread == nullptr
+      || win32_Wow64GetThreadContext == nullptr
+      || win32_Wow64SetThreadContext == nullptr))
+    error ("WOW64 debugging is not supported on this system.\n");


Hannes