Re: Subtle problems with "info sharedlibrary" on MS-Windows

Simon Marchi via Gdb-patches
2021-04-07T21:18:30Z

On 2021-04-06 9:16 a.m., Eli Zaretskii via Gdb-patches wrote:
>> Date: Mon, 05 Apr 2021 20:51:53 +0300
>> From: Eli Zaretskii via Gdb-patches <gdb-patches@sourceware.org>
>> Cc: gdb-patches@sourceware.org
>>
>> https://sourceware.org/bugzilla/show_bug.cgi?id=17659
>>
>> That bug describes the same problem and provides a patch. The bug was
>> closed without applying the because the problem was deemed resolved by
>> the addition of windows_add_all_dlls function to windows-nat.c.
>>
>> However, AFAIU windows_add_all_dlls solves the problem only for DLLs
>> loaded at startup of the debuggee. It cannot solve the problem of
>> DLLs loaded dynamically by the debuggee at run time. Which is what
>> happens in Emacs built with native-compilation capability: it compiles
>> Lisp into shared libraries, and loads those shared libraries as
>> needed.
>>
>> The problem clearly shows itself if you enable debugevents: GDB
>> reports some of the LOAD_DLL_DEBUG_EVENT's without announcing the name
>> of the loaded DLL. Later you can see that the DLL is not in the list
>> shown by "info shared", although Process Explorer shows that DLL as
>> being loaded by the debuggee.
>>
>> So I've reopened that bug, and I hope the patch there can be applied
>> to GDB some time soon.
>
> Here's a patch I propose, which completely solves the issue I
> described, and is IMO less complex than the code proposed in Bugzilla
> (it slightly refactors the existing code in windows_add_all_dlls).
>
> OK to commit to master (with a suitable ChangeLog entry)?

The patch LGTM, see minor comments below.

>
> --- gdb/windows-nat.c~0 2021-03-25 03:47:10.000000000 +0200
> +++ gdb/windows-nat.c 2021-04-06 16:11:14.853125000 +0300
> @@ -869,6 +869,8 @@ windows_make_so (const char *name, LPVOI
> return so;
> }
>
> +static bool windows_add_dll (LPVOID);
> +
> /* See nat/windows-nat.h. */
>
> void
> @@ -884,12 +886,21 @@ windows_nat::handle_load_dll ()
> (source: MSDN LOAD_DLL_DEBUG_INFO structure). */
> dll_name = get_image_name (current_process_handle,
> event->lpImageName, event->fUnicode);
> + /* If the DLL name could not be gleaned via lpImageName, try harder
> + by enumerating all the DLLs loaded into the inferior, looking for
> + one that is loaded at base address = lpBaseOfDll. */
> + if (dll_name)

According to our style guideline, we would use

dll_name != nullptr

(yes, I know the surrounding code doesn't respect that)

> + {
> +
> + solib_end->next = windows_make_so (dll_name, event->lpBaseOfDll);
> + solib_end = solib_end->next;
> + }
> + else if (windows_add_dll (event->lpBaseOfDll))
> + dll_name = solib_end->so_name;
> +
> if (!dll_name)
> return;
>
> - solib_end->next = windows_make_so (dll_name, event->lpBaseOfDll);
> - solib_end = solib_end->next;
> -
> lm_info_windows *li = (lm_info_windows *) solib_end->lm_info;
>
> DEBUG_EVENTS ("Loading dll \"%s\" at %s.", solib_end->so_name,
> @@ -1899,6 +1910,19 @@ windows_nat_target::wait (ptid_t ptid, s
> static void
> windows_add_all_dlls (void)
> {
> + windows_add_dll (NULL);
> +}
> +
> +/* Iterate over all DLLs currently mapped by our inferior, looking for
> + a DLL which is loaded at LOAD_ADDR. If found, add the DLL to our
> + list of solibs and return non-zero; otherwise do nothing and return

non-zero -> true

> + zero. LOAD_ADDR NULL means add all DLLs to the list of solibs;

zero -> false

> + this is used when the inferior finishes its initialization, and all
> + the DLLs it statically depends on are presumed loaded. */
> +
> +static bool
> +windows_add_dll (LPVOID load_addr)
> +{
> HMODULE dummy_hmodule;
> DWORD cb_needed;
> HMODULE *hmodules;
> @@ -1910,18 +1934,18 @@ windows_add_all_dlls (void)
> if (EnumProcessModulesEx (current_process_handle, &dummy_hmodule,
> sizeof (HMODULE), &cb_needed,
> LIST_MODULES_32BIT) == 0)
> - return;
> + return false;
> }
> else
> #endif
> {
> if (EnumProcessModules (current_process_handle, &dummy_hmodule,
> sizeof (HMODULE), &cb_needed) == 0)
> - return;
> + return false;
> }
>
> if (cb_needed < 1)
> - return;
> + return false;
>
> hmodules = (HMODULE *) alloca (cb_needed);
> #ifdef __x86_64__
> @@ -1930,14 +1954,14 @@ windows_add_all_dlls (void)
> if (EnumProcessModulesEx (current_process_handle, hmodules,
> cb_needed, &cb_needed,
> LIST_MODULES_32BIT) == 0)
> - return;
> + return false;
> }
> else
> #endif
> {
> if (EnumProcessModules (current_process_handle, hmodules,
> cb_needed, &cb_needed) == 0)
> - return;
> + return false;
> }
>
> char system_dir[__PMAX];
> @@ -1983,6 +2007,7 @@ windows_add_all_dlls (void)
> if (GetModuleInformation (current_process_handle, hmodules[i],
> &mi, sizeof (mi)) == 0)
> continue;
> +
> if (GetModuleFileNameEx (current_process_handle, hmodules[i],
> dll_name, sizeof (dll_name)) == 0)
> continue;
> @@ -2005,9 +2030,15 @@ windows_add_all_dlls (void)
> name = syswow_dll_path.c_str();
> }
>
> - solib_end->next = windows_make_so (name, mi.lpBaseOfDll);
> - solib_end = solib_end->next;
> + if (!(load_addr && mi.lpBaseOfDll != load_addr))

Perhaps matter of personal preference, but I would understand it better
(less mental steps) as

if (!load_addr || mi.lpBaseOfDll == load_addr)

> + {
> + solib_end->next = windows_make_so (name, mi.lpBaseOfDll);
> + solib_end = solib_end->next;
> + if (load_addr)
> + return true;

Here and above, it should be `load_addr != nullptr`.

Simon