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

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

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