public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* Subtle problems with "info sharedlibrary" on MS-Windows
@ 2021-03-10 12:36 Eli Zaretskii
  2021-03-10 16:30 ` Hannes Domani
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2021-03-10 12:36 UTC (permalink / raw)
  To: gdb-patches

I recently saw 2 strange issues with "info sharedlibrary" on
MS-Windows:

 . Some DLLs loaded explicitly via LoadLibrary don't show.  I stepped
   through the code which loads them and verified they load
   successfully; moreover, Process Explorer does show them loaded.
   But they are nowhere to be seem in "info sharedlibrary"s display.

 . The "From" address shown by "info sharedlibrary" is different from
   the base address at which the DLL is loaded: it is 4KB higher than
   the base address.

Are these problems known?  I searched Bugzilla, but didn't find
anything pertinent.

TIA


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Subtle problems with "info sharedlibrary" on MS-Windows
  2021-03-10 12:36 Subtle problems with "info sharedlibrary" on MS-Windows Eli Zaretskii
@ 2021-03-10 16:30 ` Hannes Domani
  2021-03-10 16:51   ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Hannes Domani @ 2021-03-10 16:30 UTC (permalink / raw)
  To: gdb-patches, Eli Zaretskii

 Am Mittwoch, 10. März 2021, 13:36:14 MEZ hat Eli Zaretskii via Gdb-patches <gdb-patches@sourceware.org> Folgendes geschrieben:

> I recently saw 2 strange issues with "info sharedlibrary" on
> MS-Windows:
>
> . Some DLLs loaded explicitly via LoadLibrary don't show.  I stepped
>   through the code which loads them and verified they load
>   successfully; moreover, Process Explorer does show them loaded.
>   But they are nowhere to be seem in "info sharedlibrary"s display.
>
> . The "From" address shown by "info sharedlibrary" is different from
>   the base address at which the DLL is loaded: it is 4KB higher than
>   the base address.
>
> Are these problems known?  I searched Bugzilla, but didn't find
> anything pertinent.

I'm not aware of these kind of problems.
Is there any way I can try to reproduce this?


Hannes

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Subtle problems with "info sharedlibrary" on MS-Windows
  2021-03-10 16:30 ` Hannes Domani
@ 2021-03-10 16:51   ` Eli Zaretskii
  2021-03-10 17:35     ` Hannes Domani
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2021-03-10 16:51 UTC (permalink / raw)
  To: Hannes Domani; +Cc: gdb-patches

> Date: Wed, 10 Mar 2021 16:30:30 +0000 (UTC)
> From: Hannes Domani <ssbssa@yahoo.de>
> 
> > . Some DLLs loaded explicitly via LoadLibrary don't show.  I stepped
> >   through the code which loads them and verified they load
> >   successfully; moreover, Process Explorer does show them loaded.
> >   But they are nowhere to be seem in "info sharedlibrary"s display.
> >
> > . The "From" address shown by "info sharedlibrary" is different from
> >   the base address at which the DLL is loaded: it is 4KB higher than
> >   the base address.
> >
> > Are these problems known?  I searched Bugzilla, but didn't find
> > anything pertinent.
> 
> I'm not aware of these kind of problems.
> Is there any way I can try to reproduce this?

The second one is easy: just debug any program that loads DLLs, either
because it requires them or loads them dynamically with LoadLibrary.
For example, debug gdb.exe itself, type "start", and then "info
shared".  You will see that the From address of each DLL ends in
"1000".  Now start Process Explorer and look at the Image Base or Base
address of those same DLLs: you will see it is 1000 hex (4096 decimal)
lower than what GDB shows, i.e. the image start address ends in 0000,
being 64KB aligned.

(I found this because AFAIU the handle returned by LoadLibrary is the
starting address where the DLL is loaded, and I saw the 4K mismatch
between that handle and what GDB was reporting as the starting
address.)

For the first problem, I don't have an easy reproducer.  The only
situation where I saw it was in the native-comp branch of GNU Emacs,
which uses libgccjit to compile Lisp files into DLLs, then loads them
at run time.  If you can build that branch of Emacs, I can tell you
how to reproduce the first problem using that build.  However, maybe
you could see it also in other executables, if you carefully compare
what GDB reports against Process Explorer.

Thanks.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Subtle problems with "info sharedlibrary" on MS-Windows
  2021-03-10 16:51   ` Eli Zaretskii
@ 2021-03-10 17:35     ` Hannes Domani
  2021-04-05 17:51       ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Hannes Domani @ 2021-03-10 17:35 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: gdb-patches

 Am Mittwoch, 10. März 2021, 17:51:43 MEZ hat Eli Zaretskii <eliz@gnu.org> Folgendes geschrieben:

> > Date: Wed, 10 Mar 2021 16:30:30 +0000 (UTC)
> > From: Hannes Domani <ssbssa@yahoo.de>
> >
> > > . Some DLLs loaded explicitly via LoadLibrary don't show.  I stepped
> > >   through the code which loads them and verified they load
> > >   successfully; moreover, Process Explorer does show them loaded.
> > >   But they are nowhere to be seem in "info sharedlibrary"s display.
> > >
> > > . The "From" address shown by "info sharedlibrary" is different from
> > >   the base address at which the DLL is loaded: it is 4KB higher than
> > >   the base address.
> > >
> > > Are these problems known?  I searched Bugzilla, but didn't find
> > > anything pertinent.
> >
> > I'm not aware of these kind of problems.
> > Is there any way I can try to reproduce this?
>
> The second one is easy: just debug any program that loads DLLs, either
> because it requires them or loads them dynamically with LoadLibrary.
> For example, debug gdb.exe itself, type "start", and then "info
> shared".  You will see that the From address of each DLL ends in
> "1000".  Now start Process Explorer and look at the Image Base or Base
> address of those same DLLs: you will see it is 1000 hex (4096 decimal)
> lower than what GDB shows, i.e. the image start address ends in 0000,
> being 64KB aligned.
>
> (I found this because AFAIU the handle returned by LoadLibrary is the
> starting address where the DLL is loaded, and I saw the 4K mismatch
> between that handle and what GDB was reporting as the starting
> address.)

Oh, that's what you mean.
I think it always was like this, so I assumed this was intentional.
It shows the address of the .text section, not of the DLL base.


> For the first problem, I don't have an easy reproducer.  The only
> situation where I saw it was in the native-comp branch of GNU Emacs,
> which uses libgccjit to compile Lisp files into DLLs, then loads them
> at run time.  If you can build that branch of Emacs, I can tell you
> how to reproduce the first problem using that build.  However, maybe
> you could see it also in other executables, if you carefully compare
> what GDB reports against Process Explorer.

Building Emacs is a bit too much for me right now, but I will see if I notice
it when I debug some other programs.


Hannes

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Subtle problems with "info sharedlibrary" on MS-Windows
  2021-03-10 17:35     ` Hannes Domani
@ 2021-04-05 17:51       ` Eli Zaretskii
  2021-04-06 13:16         ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2021-04-05 17:51 UTC (permalink / raw)
  To: Hannes Domani; +Cc: gdb-patches

> From: ssbssa@yahoo.de (Hannes Domani)
> Date: Wed, 10 Mar 2021 17:35:02 +0000 (UTC)
> 
>  Am Mittwoch, 10. März 2021, 17:51:43 MEZ hat Eli Zaretskii <eliz@gnu.org> Folgendes geschrieben:
> 
> > > Date: Wed, 10 Mar 2021 16:30:30 +0000 (UTC)
> > > From: Hannes Domani <ssbssa@yahoo.de>
> > >
> > > > . Some DLLs loaded explicitly via LoadLibrary don't show.? I stepped
> > > > through the code which loads them and verified they load
> > > > successfully; moreover, Process Explorer does show them loaded.
> > > > But they are nowhere to be seem in "info sharedlibrary"s display.
> > > >
> > > > . The "From" address shown by "info sharedlibrary" is different from
> > > > the base address at which the DLL is loaded: it is 4KB higher than
> > > > the base address.
> > > >
> > > > Are these problems known? I searched Bugzilla, but didn't find
> > > > anything pertinent.
> > >
> [...]
> > For the first problem, I don't have an easy reproducer. The only
> > situation where I saw it was in the native-comp branch of GNU Emacs,
> > which uses libgccjit to compile Lisp files into DLLs, then loads them
> > at run time. If you can build that branch of Emacs, I can tell you
> > how to reproduce the first problem using that build. However, maybe
> > you could see it also in other executables, if you carefully compare
> > what GDB reports against Process Explorer.
> 
> Building Emacs is a bit too much for me right now, but I will see if I notice
> it when I debug some other programs.

I found an old bug report in Bugzilla:

  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.

Thanks.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Subtle problems with "info sharedlibrary" on MS-Windows
  2021-04-05 17:51       ` Eli Zaretskii
@ 2021-04-06 13:16         ` Eli Zaretskii
  2021-04-07 21:18           ` Simon Marchi
  2021-04-10 15:03           ` Tom Tromey
  0 siblings, 2 replies; 18+ messages in thread
From: Eli Zaretskii @ 2021-04-06 13:16 UTC (permalink / raw)
  To: ssbssa; +Cc: gdb-patches

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

--- 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)
+    {
+
+      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
+   zero.  LOAD_ADDR NULL means add all DLLs to the list of solibs;
+   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))
+	{
+	  solib_end->next = windows_make_so (name, mi.lpBaseOfDll);
+	  solib_end = solib_end->next;
+	  if (load_addr)
+	    return true;
+	}
     }
+  return false;
 }
 
 void


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Subtle problems with "info sharedlibrary" on MS-Windows
  2021-04-06 13:16         ` Eli Zaretskii
@ 2021-04-07 21:18           ` Simon Marchi
  2021-04-08  7:06             ` Eli Zaretskii
  2021-04-10 15:03           ` Tom Tromey
  1 sibling, 1 reply; 18+ messages in thread
From: Simon Marchi @ 2021-04-07 21:18 UTC (permalink / raw)
  To: Eli Zaretskii, ssbssa; +Cc: gdb-patches

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Subtle problems with "info sharedlibrary" on MS-Windows
  2021-04-07 21:18           ` Simon Marchi
@ 2021-04-08  7:06             ` Eli Zaretskii
  2021-04-08 13:57               ` Simon Marchi
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2021-04-08  7:06 UTC (permalink / raw)
  To: Simon Marchi; +Cc: ssbssa, gdb-patches

> Cc: gdb-patches@sourceware.org
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Date: Wed, 7 Apr 2021 17:18:08 -0400
> 
> > OK to commit to master (with a suitable ChangeLog entry)?
> 
> The patch LGTM, see minor comments below.

Thanks for your prompt review (and for the other help in investigating
this tricky problem).

> According to our style guideline, we would use
> 
>   dll_name != nullptr

Does this also mean the GDB style prefers, e.g.,

    if (load_addr == nullptr)

to

    if (!load_addr)

?  Because you didn't comment on those lines, only on those where the
value is tested for NOT being null.

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

I feel the other way around, but maybe I'm the odd one out here.
Pedro, Joel: what say you?

In any case, I guess I could add a comment there explaining the logic
in plain English, so everyone would understand the intent.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Subtle problems with "info sharedlibrary" on MS-Windows
  2021-04-08  7:06             ` Eli Zaretskii
@ 2021-04-08 13:57               ` Simon Marchi
  2021-04-10  8:46                 ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Simon Marchi @ 2021-04-08 13:57 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: ssbssa, gdb-patches

On 2021-04-08 3:06 a.m., Eli Zaretskii wrote:
>> Cc: gdb-patches@sourceware.org
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>> Date: Wed, 7 Apr 2021 17:18:08 -0400
>>
>>> OK to commit to master (with a suitable ChangeLog entry)?
>>
>> The patch LGTM, see minor comments below.
> 
> Thanks for your prompt review (and for the other help in investigating
> this tricky problem).
> 
>> According to our style guideline, we would use
>>
>>   dll_name != nullptr
> 
> Does this also mean the GDB style prefers, e.g.,
> 
>     if (load_addr == nullptr)
> 
> to
> 
>     if (!load_addr)
> 
> ?  Because you didn't comment on those lines, only on those where the
> value is tested for NOT being null.

Exactly, sorry for not being clear.  We always want explicit comparison
with nullptr for pointers, whether that is == or !=.

Same for integers that are not used as booleans, we want

  if (item_count == 0)

and not

  if (!item_count)

This is described here:

    https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Comparison_With_nullptr_And_Zero

>>> +      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)
> 
> I feel the other way around, but maybe I'm the odd one out here.
> Pedro, Joel: what say you?
> 
> In any case, I guess I could add a comment there explaining the logic
> in plain English, so everyone would understand the intent.

That's alright, leave it the way it is.  But a comment explaining the
intent is always welcome, in my book.

Thanks,

Simon

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Subtle problems with "info sharedlibrary" on MS-Windows
  2021-04-08 13:57               ` Simon Marchi
@ 2021-04-10  8:46                 ` Eli Zaretskii
  0 siblings, 0 replies; 18+ messages in thread
From: Eli Zaretskii @ 2021-04-10  8:46 UTC (permalink / raw)
  To: Simon Marchi; +Cc: ssbssa, gdb-patches

> Cc: ssbssa@yahoo.de, gdb-patches@sourceware.org
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Date: Thu, 8 Apr 2021 09:57:27 -0400
> 
> >> According to our style guideline, we would use
> >>
> >>   dll_name != nullptr
> > 
> > Does this also mean the GDB style prefers, e.g.,
> > 
> >     if (load_addr == nullptr)
> > 
> > to
> > 
> >     if (!load_addr)
> > 
> > ?  Because you didn't comment on those lines, only on those where the
> > value is tested for NOT being null.
> 
> Exactly, sorry for not being clear.  We always want explicit comparison
> with nullptr for pointers, whether that is == or !=.
> 
> Same for integers that are not used as booleans, we want
> 
>   if (item_count == 0)
> 
> and not
> 
>   if (!item_count)
> 
> This is described here:
> 
>     https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Comparison_With_nullptr_And_Zero
> 
> >>> +      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)
> > 
> > I feel the other way around, but maybe I'm the odd one out here.
> > Pedro, Joel: what say you?
> > 
> > In any case, I guess I could add a comment there explaining the logic
> > in plain English, so everyone would understand the intent.
> 
> That's alright, leave it the way it is.  But a comment explaining the
> intent is always welcome, in my book.

OK, thanks.  Below is what I actually pushed:

commit b3885679dd71bed069e7e6fc2ae8b9eb05f90d62
Author:     Eli Zaretskii <eliz@gnu.org>
AuthorDate: Sat Apr 10 11:33:08 2021 +0300
Commit:     Eli Zaretskii <eliz@gnu.org>
CommitDate: Sat Apr 10 11:42:54 2021 +0300

    Fix handling DLL loads at run time
    
    This patch makes handling a DLL load at run time (using LoadLibrary)
    much more reliable when its file name cannot be obtained using the
    lpImageName pointer provided by the DLL load debug event.  The
    solution is to enumerate all the DLLs loaded by the inferior, looking
    for the DLL that's loaded at base address provided by the lpBaseOfDll
    pointer of the debug event.  Correctly resolving the DLL file name is
    important, because without that GDB doesn't record the DLL in the list
    of solibs, and then later is unable to show functions in that DLL in
    the backtraces, which produces corrupted and truncated backtraces.
    See this thread for the problems that causes:
    
      https://sourceware.org/pipermail/gdb-patches/2021-March/177022.html
    
    gdb/ChangeLog:
    
    2021-04-10  Eli Zaretskii  <eliz@gnu.org>
    
            * windows-nat.c (windows_nat::handle_load_dll): Call
            windows_add_dll if get_image_name failed to glean the name of the
            DLL by using the lpImageName pointer.
            (windows_add_all_dlls): Now a thin wrapper around windows_add_dll.
            (windows_add_dll): Now does what windows_add_all_dlls did before,
            but also accepts an argument LOAD_ADDR, which, if non-NULL,
            specifies the address where the DLL was loaded into the inferior,
            and looks for the single DLL loaded at that address.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index c5ed593..5c38a4e 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,14 @@
+2021-04-10  Eli Zaretskii  <eliz@gnu.org>
+
+	* windows-nat.c (windows_nat::handle_load_dll): Call
+	windows_add_dll if get_image_name failed to glean the name of the
+	DLL by using the lpImageName pointer.
+	(windows_add_all_dlls): Now a thin wrapper around windows_add_dll.
+	(windows_add_dll): Now does what windows_add_all_dlls did before,
+	but also accepts an argument LOAD_ADDR, which, if non-NULL,
+	specifies the address where the DLL was loaded into the inferior,
+	and looks for the single DLL loaded at that address.
+
 2021-04-09  Luis Machado  <luis.machado@linaro.org>
 
 	* nat/aarch64-mte-linux-ptrace.c: Update include file order.
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 00f8353..c0f64f8 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -869,6 +869,8 @@ windows_make_so (const char *name, LPVOID load_addr)
   return so;
 }
 
+static bool windows_add_dll (LPVOID);
+
 /* See nat/windows-nat.h.  */
 
 void
@@ -884,11 +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 (!dll_name)
-    return;
+  /* 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 != nullptr)
+    {
 
-  solib_end->next = windows_make_so (dll_name, event->lpBaseOfDll);
-  solib_end = solib_end->next;
+      solib_end->next = windows_make_so (dll_name, event->lpBaseOfDll);
+      solib_end = solib_end->next;
+    }
+  else if (event->lpBaseOfDll != nullptr
+	   && windows_add_dll (event->lpBaseOfDll))
+    dll_name = solib_end->so_name;
+
+  if (dll_name == nullptr)
+    return;
 
   lm_info_windows *li = (lm_info_windows *) solib_end->lm_info;
 
@@ -1899,6 +1911,19 @@ windows_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 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 'true'; otherwise do nothing and return
+   'false'.  LOAD_ADDR NULL means add all DLLs to the list of solibs;
+   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 +1935,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 +1955,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 +2008,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 +2031,17 @@ 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;
+      /* Record the DLL if either LOAD_ADDR is NULL or the address
+	 at which the DLL was loaded is equal to LOAD_ADDR.  */
+      if (!(load_addr != nullptr && mi.lpBaseOfDll != load_addr))
+	{
+	  solib_end->next = windows_make_so (name, mi.lpBaseOfDll);
+	  solib_end = solib_end->next;
+	  if (load_addr != nullptr)
+	    return true;
+	}
     }
+  return load_addr == nullptr ? true : false;
 }
 
 void

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Subtle problems with "info sharedlibrary" on MS-Windows
  2021-04-06 13:16         ` Eli Zaretskii
  2021-04-07 21:18           ` Simon Marchi
@ 2021-04-10 15:03           ` Tom Tromey
  2021-04-10 18:07             ` Eli Zaretskii
  1 sibling, 1 reply; 18+ messages in thread
From: Tom Tromey @ 2021-04-10 15:03 UTC (permalink / raw)
  To: Eli Zaretskii via Gdb-patches

>>>>> "Eli" == Eli Zaretskii via Gdb-patches <gdb-patches@sourceware.org> writes:

Eli> Here's a patch I propose, which completely solves the issue I
Eli> described, and is IMO less complex than the code proposed in Bugzilla
Eli> (it slightly refactors the existing code in windows_add_all_dlls).

I suppose a patch similar to this is needed for gdbserver as well?

Unfortunately this is still an area where we haven't yet merged the code
between gdbserver/win32-low.cc and gdb/windows-nat.c.

To be clear, I think we don't insist on parity here.  It's just a nice
to have, not a reason to reject a patch.  I'm mostly checking in to make
sure I'm understanding the situation correctly.

Maybe someday we can do the last 20% of this merge so these divergences
are harder to come across.

thanks,
Tom

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Subtle problems with "info sharedlibrary" on MS-Windows
  2021-04-10 15:03           ` Tom Tromey
@ 2021-04-10 18:07             ` Eli Zaretskii
  2021-04-10 22:56               ` Simon Marchi
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2021-04-10 18:07 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches, ssbssa

> From: Tom Tromey <tom@tromey.com>
> Cc: ssbssa@yahoo.de,  Eli Zaretskii <eliz@gnu.org>
> Date: Sat, 10 Apr 2021 09:03:26 -0600
> 
> I suppose a patch similar to this is needed for gdbserver as well?

Is the below okay?

--- gdbserver/win32-low.cc~	2021-03-25 03:47:10.000000000 +0200
+++ gdbserver/win32-low.cc	2021-04-10 21:05:48.677425000 +0300
@@ -1165,11 +1165,13 @@ load_psapi (void)
 
 #ifndef _WIN32_WCE
 
-/* Iterate over all DLLs currently mapped by our inferior, and
-   add them to our list of solibs.  */
+/* Iterate over all DLLs currently mapped by our inferior, looking for
+   a DLL loaded at LOAD_ADDR; if found, return its file name,
+   otherwise return NULL.  If LOAD_ADDR is NULL, add all mapped DLLs
+   to our list of solibs.  */
 
-static void
-win32_add_all_dlls (void)
+static char *
+win32_add_dll (LPVOID load_addr)
 {
   size_t i;
   HMODULE dh_buf[1];
@@ -1178,7 +1180,7 @@ win32_add_all_dlls (void)
   BOOL ok;
 
   if (!load_psapi ())
-    return;
+    return NULL;
 
   cbNeeded = 0;
 #ifdef __x86_64__
@@ -1196,11 +1198,11 @@ win32_add_all_dlls (void)
 				      &cbNeeded);
 
   if (!ok || !cbNeeded)
-    return;
+    return NULL;
 
   DllHandle = (HMODULE *) alloca (cbNeeded);
   if (!DllHandle)
-    return;
+    return NULL;
 
 #ifdef __x86_64__
   if (wow64_process)
@@ -1216,7 +1218,7 @@ win32_add_all_dlls (void)
 				      cbNeeded,
 				      &cbNeeded);
   if (!ok)
-    return;
+    return NULL;
 
   char system_dir[MAX_PATH];
   char syswow_dir[MAX_PATH];
@@ -1252,7 +1254,7 @@ win32_add_all_dlls (void)
   for (i = 1; i < ((size_t) cbNeeded / sizeof (HMODULE)); i++)
     {
       MODULEINFO mi;
-      char dll_name[MAX_PATH];
+      static char dll_name[MAX_PATH];
 
       if (!(*win32_GetModuleInformation) (current_process_handle,
 					  DllHandle[i],
@@ -1265,6 +1267,9 @@ win32_add_all_dlls (void)
 					 MAX_PATH) == 0)
 	continue;
 
+      if (load_addr != nullptr && mi.lpBaseOfDll != load_addr)
+	continue;
+
       const char *name = dll_name;
       /* Convert the DLL path of 32bit processes returned by
 	 GetModuleFileNameEx from the 64bit system directory to the
@@ -1280,9 +1285,24 @@ win32_add_all_dlls (void)
 	}
 
       win32_add_one_solib (name, (CORE_ADDR) (uintptr_t) mi.lpBaseOfDll);
+      if (load_addr != nullptr)
+	{
+	  strcpy (dll_name, name);
+	  return dll_name;
+	}
     }
+  return NULL;
 }
-#endif
+
+/* Iterate over all DLLs currently mapped by our inferior, and
+   add them to our list of solibs.  */
+
+static void
+win32_add_all_dlls (void)
+{
+  win32_add_dll (NULL);
+}
+#endif	/* !_WIN32_WCE */
 
 typedef HANDLE (WINAPI *winapi_CreateToolhelp32Snapshot) (DWORD, DWORD);
 typedef BOOL (WINAPI *winapi_Module32First) (HANDLE, LPMODULEENTRY32);
@@ -1298,7 +1318,12 @@ windows_nat::handle_load_dll ()
 
   dll_name = get_image_name (current_process_handle,
 			     event->lpImageName, event->fUnicode);
-  if (!dll_name)
+#ifndef _WIN32_WCE
+  if (dll_name == nullptr
+      && event->lpBaseOfDll != nullptr)
+    dll_name = win32_add_dll (event->lpBaseOfDll);
+#endif
+  if (dll_name == nullptr)
     return;
 
   win32_add_one_solib (dll_name, (CORE_ADDR) (uintptr_t) event->lpBaseOfDll);

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Subtle problems with "info sharedlibrary" on MS-Windows
  2021-04-10 18:07             ` Eli Zaretskii
@ 2021-04-10 22:56               ` Simon Marchi
  2021-04-10 23:11                 ` Simon Marchi
  2021-04-11  7:10                 ` Eli Zaretskii
  0 siblings, 2 replies; 18+ messages in thread
From: Simon Marchi @ 2021-04-10 22:56 UTC (permalink / raw)
  To: Eli Zaretskii, Tom Tromey; +Cc: gdb-patches



On 2021-04-10 2:07 p.m., Eli Zaretskii via Gdb-patches wrote:
>> From: Tom Tromey <tom@tromey.com>
>> Cc: ssbssa@yahoo.de,  Eli Zaretskii <eliz@gnu.org>
>> Date: Sat, 10 Apr 2021 09:03:26 -0600
>>
>> I suppose a patch similar to this is needed for gdbserver as well?
> 
> Is the below okay?
> 
> --- gdbserver/win32-low.cc~	2021-03-25 03:47:10.000000000 +0200
> +++ gdbserver/win32-low.cc	2021-04-10 21:05:48.677425000 +0300
> @@ -1165,11 +1165,13 @@ load_psapi (void)
>  
>  #ifndef _WIN32_WCE

I see that this patch deals with _WIN32_WCE.  We removed support for
WinCE in 84b300de3666 ("gdbserver: remove support for ARM/WinCE").  It
looks like I failed to remove the WinCE-related core from win32-low.cc
(I guess I didn't know about the _WIN32_WCE macro then).  Would you mind
if we first removed that unncecessary code first?  That would make your
patch simpler.

Simon

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Subtle problems with "info sharedlibrary" on MS-Windows
  2021-04-10 22:56               ` Simon Marchi
@ 2021-04-10 23:11                 ` Simon Marchi
  2021-04-11  7:10                 ` Eli Zaretskii
  1 sibling, 0 replies; 18+ messages in thread
From: Simon Marchi @ 2021-04-10 23:11 UTC (permalink / raw)
  To: Eli Zaretskii, Tom Tromey; +Cc: gdb-patches

On 2021-04-10 6:56 p.m., Simon Marchi via Gdb-patches wrote:
> 
> 
> On 2021-04-10 2:07 p.m., Eli Zaretskii via Gdb-patches wrote:
>>> From: Tom Tromey <tom@tromey.com>
>>> Cc: ssbssa@yahoo.de,  Eli Zaretskii <eliz@gnu.org>
>>> Date: Sat, 10 Apr 2021 09:03:26 -0600
>>>
>>> I suppose a patch similar to this is needed for gdbserver as well?
>>
>> Is the below okay?
>>
>> --- gdbserver/win32-low.cc~	2021-03-25 03:47:10.000000000 +0200
>> +++ gdbserver/win32-low.cc	2021-04-10 21:05:48.677425000 +0300
>> @@ -1165,11 +1165,13 @@ load_psapi (void)
>>  
>>  #ifndef _WIN32_WCE
> 
> I see that this patch deals with _WIN32_WCE.  We removed support for
> WinCE in 84b300de3666 ("gdbserver: remove support for ARM/WinCE").  It
> looks like I failed to remove the WinCE-related core from win32-low.cc
> (I guess I didn't know about the _WIN32_WCE macro then).  Would you mind
> if we first removed that unncecessary code first?  That would make your
> patch simpler.
> 
> Simon

I sent a patch doing that here:

https://sourceware.org/pipermail/gdb-patches/2021-April/177658.html

Simon

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Subtle problems with "info sharedlibrary" on MS-Windows
  2021-04-10 22:56               ` Simon Marchi
  2021-04-10 23:11                 ` Simon Marchi
@ 2021-04-11  7:10                 ` Eli Zaretskii
  2021-04-11 12:27                   ` Simon Marchi
  1 sibling, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2021-04-11  7:10 UTC (permalink / raw)
  To: Simon Marchi; +Cc: tom, gdb-patches

> Cc: gdb-patches@sourceware.org
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Date: Sat, 10 Apr 2021 18:56:05 -0400
> 
> > --- gdbserver/win32-low.cc~	2021-03-25 03:47:10.000000000 +0200
> > +++ gdbserver/win32-low.cc	2021-04-10 21:05:48.677425000 +0300
> > @@ -1165,11 +1165,13 @@ load_psapi (void)
> >  
> >  #ifndef _WIN32_WCE
> 
> I see that this patch deals with _WIN32_WCE.  We removed support for
> WinCE in 84b300de3666 ("gdbserver: remove support for ARM/WinCE").  It
> looks like I failed to remove the WinCE-related core from win32-low.cc
> (I guess I didn't know about the _WIN32_WCE macro then).  Would you mind
> if we first removed that unncecessary code first?  That would make your
> patch simpler.

I'd actually prefer it the other way around, for 2 reasons: (a) the
patch for windows-nat.c is already in, so it would make sense to fix
gdbserver ASAP; (b) I built with the patch a recent snapshot of master
where these conditions are still present, and I would like to avoid
the need to build yet another snapshot just for this small change.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Subtle problems with "info sharedlibrary" on MS-Windows
  2021-04-11  7:10                 ` Eli Zaretskii
@ 2021-04-11 12:27                   ` Simon Marchi
  2021-04-11 18:43                     ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Simon Marchi @ 2021-04-11 12:27 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: tom, gdb-patches

On 2021-04-11 3:10 a.m., Eli Zaretskii wrote:
>> Cc: gdb-patches@sourceware.org
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>> Date: Sat, 10 Apr 2021 18:56:05 -0400
>>
>>> --- gdbserver/win32-low.cc~	2021-03-25 03:47:10.000000000 +0200
>>> +++ gdbserver/win32-low.cc	2021-04-10 21:05:48.677425000 +0300
>>> @@ -1165,11 +1165,13 @@ load_psapi (void)
>>>  
>>>  #ifndef _WIN32_WCE
>>
>> I see that this patch deals with _WIN32_WCE.  We removed support for
>> WinCE in 84b300de3666 ("gdbserver: remove support for ARM/WinCE").  It
>> looks like I failed to remove the WinCE-related core from win32-low.cc
>> (I guess I didn't know about the _WIN32_WCE macro then).  Would you mind
>> if we first removed that unncecessary code first?  That would make your
>> patch simpler.
> 
> I'd actually prefer it the other way around, for 2 reasons: (a) the
> patch for windows-nat.c is already in, so it would make sense to fix
> gdbserver ASAP; (b) I built with the patch a recent snapshot of master
> where these conditions are still present, and I would like to avoid
> the need to build yet another snapshot just for this small change.

Ok, now that I re-read the patch, I realize that the _WIN32_WCE macro
doesn't add much complexity, I thought it was worst at first.  So, the
patch LGTM, with a proper commit message and ChangeLog entry of course.

Simon

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Subtle problems with "info sharedlibrary" on MS-Windows
  2021-04-11 12:27                   ` Simon Marchi
@ 2021-04-11 18:43                     ` Eli Zaretskii
  2021-04-12 19:03                       ` Tom Tromey
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2021-04-11 18:43 UTC (permalink / raw)
  To: Simon Marchi; +Cc: tom, gdb-patches

> Cc: tom@tromey.com, gdb-patches@sourceware.org
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Date: Sun, 11 Apr 2021 08:27:55 -0400
> 
> > I'd actually prefer it the other way around, for 2 reasons: (a) the
> > patch for windows-nat.c is already in, so it would make sense to fix
> > gdbserver ASAP; (b) I built with the patch a recent snapshot of master
> > where these conditions are still present, and I would like to avoid
> > the need to build yet another snapshot just for this small change.
> 
> Ok, now that I re-read the patch, I realize that the _WIN32_WCE macro
> doesn't add much complexity, I thought it was worst at first.  So, the
> patch LGTM, with a proper commit message and ChangeLog entry of course.

Thanks, done.  Below is the actual change I pushed:

commit 114ee2a4aef573bf43ab71b9d0b85aaccfd8852e
Author:     Eli Zaretskii <eliz@gnu.org>
AuthorDate: Sun Apr 11 21:37:29 2021 +0300
Commit:     Eli Zaretskii <eliz@gnu.org>
CommitDate: Sun Apr 11 21:37:29 2021 +0300

    Improve support for loading DLLs at run time in gdbserver.
    
    This fixes win32-low.cc in the same way as a recent change in
    windows-nat.c did for GDB: if the lpImageName member of the load-DLL
    debug event doesn't allow us to find the file name of the DLL, then
    loop over all the DLLs mapped into the inferior to find the one loaded
    at the same base address as given by the lpBaseOfDll member of the
    debug event.
    
    gdbserver/ChangeLog:
    
    2021-04-11  Eli Zaretskii  <eliz@gnu.org>
    
            * win32-low.cc (win32_add_dll): New function, with body almost
            identical to what win32_add_all_dlls did.  Accepts one argument;
            if that is non-NULL, returns the file name of the DLL that is
            loaded at the base address equal to that argument, or NULL if not
            found.  If the argument is NULL, add all the DLLs loaded by the
            inferior to the list of solibs and return NULL.
            (win32_add_all_dlls): Now a thin wrapper around win32_add_dll.
            (windows_nat::handle_load_dll) [!_WIN32_WCE]: If get_image_name
            failed to glean the file name of the DLL, call win32_add_dll to
            try harder using the lpBaseOfDll member of the load-DLL event.

diff --git a/gdbserver/ChangeLog b/gdbserver/ChangeLog
index 58ed0f0..029a2e4 100644
--- a/gdbserver/ChangeLog
+++ b/gdbserver/ChangeLog
@@ -1,3 +1,16 @@
+2021-04-11  Eli Zaretskii  <eliz@gnu.org>
+
+	* win32-low.cc (win32_add_dll): New function, with body almost
+	identical to what win32_add_all_dlls did.  Accepts one argument;
+	if that is non-NULL, returns the file name of the DLL that is
+	loaded at the base address equal to that argument, or NULL if not
+	found.  If the argument is NULL, add all the DLLs loaded by the
+	inferior to the list of solibs and return NULL.
+	(win32_add_all_dlls): Now a thin wrapper around win32_add_dll.
+	(windows_nat::handle_load_dll) [!_WIN32_WCE]: If get_image_name
+	failed to glean the file name of the DLL, call win32_add_dll to
+	try harder using the lpBaseOfDll member of the load-DLL event.
+
 2021-03-30  Luis Machado  <luis.machado@linaro.org>
 
 	* server.cc (handle_general_set, handle_query): Update variable
diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
index 1f319a2..f6d35ca 100644
--- a/gdbserver/win32-low.cc
+++ b/gdbserver/win32-low.cc
@@ -1165,11 +1165,13 @@ load_psapi (void)
 
 #ifndef _WIN32_WCE
 
-/* Iterate over all DLLs currently mapped by our inferior, and
-   add them to our list of solibs.  */
+/* Iterate over all DLLs currently mapped by our inferior, looking for
+   a DLL loaded at LOAD_ADDR; if found, return its file name,
+   otherwise return NULL.  If LOAD_ADDR is NULL, add all mapped DLLs
+   to our list of solibs.  */
 
-static void
-win32_add_all_dlls (void)
+static char *
+win32_add_dll (LPVOID load_addr)
 {
   size_t i;
   HMODULE dh_buf[1];
@@ -1178,7 +1180,7 @@ win32_add_all_dlls (void)
   BOOL ok;
 
   if (!load_psapi ())
-    return;
+    return NULL;
 
   cbNeeded = 0;
 #ifdef __x86_64__
@@ -1196,11 +1198,11 @@ win32_add_all_dlls (void)
 				      &cbNeeded);
 
   if (!ok || !cbNeeded)
-    return;
+    return NULL;
 
   DllHandle = (HMODULE *) alloca (cbNeeded);
   if (!DllHandle)
-    return;
+    return NULL;
 
 #ifdef __x86_64__
   if (wow64_process)
@@ -1216,7 +1218,7 @@ win32_add_all_dlls (void)
 				      cbNeeded,
 				      &cbNeeded);
   if (!ok)
-    return;
+    return NULL;
 
   char system_dir[MAX_PATH];
   char syswow_dir[MAX_PATH];
@@ -1252,7 +1254,7 @@ win32_add_all_dlls (void)
   for (i = 1; i < ((size_t) cbNeeded / sizeof (HMODULE)); i++)
     {
       MODULEINFO mi;
-      char dll_name[MAX_PATH];
+      static char dll_name[MAX_PATH];
 
       if (!(*win32_GetModuleInformation) (current_process_handle,
 					  DllHandle[i],
@@ -1265,6 +1267,9 @@ win32_add_all_dlls (void)
 					 MAX_PATH) == 0)
 	continue;
 
+      if (load_addr != nullptr && mi.lpBaseOfDll != load_addr)
+	continue;
+
       const char *name = dll_name;
       /* Convert the DLL path of 32bit processes returned by
 	 GetModuleFileNameEx from the 64bit system directory to the
@@ -1279,10 +1284,27 @@ win32_add_all_dlls (void)
 	  name = syswow_dll_path.c_str();
 	}
 
-      win32_add_one_solib (name, (CORE_ADDR) (uintptr_t) mi.lpBaseOfDll);
+      if (load_addr != nullptr)
+	{
+	  if (name != dll_name)
+	    strcpy (dll_name, name);
+	  return dll_name;
+	}
+      else
+	win32_add_one_solib (name, (CORE_ADDR) (uintptr_t) mi.lpBaseOfDll);
     }
+  return NULL;
 }
-#endif
+
+/* Iterate over all DLLs currently mapped by our inferior, and
+   add them to our list of solibs.  */
+
+static void
+win32_add_all_dlls (void)
+{
+  win32_add_dll (NULL);
+}
+#endif	/* !_WIN32_WCE */
 
 typedef HANDLE (WINAPI *winapi_CreateToolhelp32Snapshot) (DWORD, DWORD);
 typedef BOOL (WINAPI *winapi_Module32First) (HANDLE, LPMODULEENTRY32);
@@ -1298,7 +1320,12 @@ windows_nat::handle_load_dll ()
 
   dll_name = get_image_name (current_process_handle,
 			     event->lpImageName, event->fUnicode);
-  if (!dll_name)
+#ifndef _WIN32_WCE
+  if (dll_name == nullptr
+      && event->lpBaseOfDll != nullptr)
+    dll_name = win32_add_dll (event->lpBaseOfDll);
+#endif
+  if (dll_name == nullptr)
     return;
 
   win32_add_one_solib (dll_name, (CORE_ADDR) (uintptr_t) event->lpBaseOfDll);

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Subtle problems with "info sharedlibrary" on MS-Windows
  2021-04-11 18:43                     ` Eli Zaretskii
@ 2021-04-12 19:03                       ` Tom Tromey
  0 siblings, 0 replies; 18+ messages in thread
From: Tom Tromey @ 2021-04-12 19:03 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Simon Marchi, tom, gdb-patches

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

>> Ok, now that I re-read the patch, I realize that the _WIN32_WCE macro
>> doesn't add much complexity, I thought it was worst at first.  So, the
>> patch LGTM, with a proper commit message and ChangeLog entry of course.

Eli> Thanks, done.  Below is the actual change I pushed:

Thank you for doing this.

Tom

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2021-04-12 19:04 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-10 12:36 Subtle problems with "info sharedlibrary" on MS-Windows Eli Zaretskii
2021-03-10 16:30 ` Hannes Domani
2021-03-10 16:51   ` Eli Zaretskii
2021-03-10 17:35     ` Hannes Domani
2021-04-05 17:51       ` Eli Zaretskii
2021-04-06 13:16         ` Eli Zaretskii
2021-04-07 21:18           ` Simon Marchi
2021-04-08  7:06             ` Eli Zaretskii
2021-04-08 13:57               ` Simon Marchi
2021-04-10  8:46                 ` Eli Zaretskii
2021-04-10 15:03           ` Tom Tromey
2021-04-10 18:07             ` Eli Zaretskii
2021-04-10 22:56               ` Simon Marchi
2021-04-10 23:11                 ` Simon Marchi
2021-04-11  7:10                 ` Eli Zaretskii
2021-04-11 12:27                   ` Simon Marchi
2021-04-11 18:43                     ` Eli Zaretskii
2021-04-12 19:03                       ` Tom Tromey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).