From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 0A4AB386F434 for ; Tue, 28 Apr 2020 15:53:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 0A4AB386F434 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.193] (unknown [192.222.164.54]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 9148A1F375; Tue, 28 Apr 2020 11:53:11 -0400 (EDT) Subject: Re: [PATCH v2] Implement debugging of WOW64 processes in gdbserver To: Hannes Domani , gdb-patches@sourceware.org References: <20200427133416.9314-1-ssbssa.ref@yahoo.de> <20200427133416.9314-1-ssbssa@yahoo.de> From: Simon Marchi Message-ID: <7ec0c0b7-5425-d766-326b-9d5b13b755b4@simark.ca> Date: Tue, 28 Apr 2020 11:53:10 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <20200427133416.9314-1-ssbssa@yahoo.de> Content-Type: text/plain; charset=utf-8 Content-Language: tl Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-22.7 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 28 Apr 2020 15:53:14 -0000 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? > @@ -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. > + 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. > +#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 (¤t_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(). } > @@ -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. > + > 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? Simon