Re: [PATCH 6/4] libbacktrace: Add loaded dlls after initialize

[Eli Zaretskii <eliz@gnu.org>]

[I re-added the other addressees, as I don' think you meant to make
this discussion private between the two of us.]

> Date: Sun, 7 Jan 2024 12:58:29 +0100
> From: Björn Schäpers <gcc@hazardy.de>
> 
> Am 07.01.2024 um 07:50 schrieb Eli Zaretskii:
> >> Date: Sat, 6 Jan 2024 23:15:24 +0100
> >> From: Björn Schäpers <gcc@hazardy.de>
> >> Cc: gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org
> >>
> >> This patch adds libraries which are loaded after backtrace_initialize, like
> >> plugins or similar.
> >>
> >> I don't know what style is preferred for the Win32 typedefs, should the code use
> >> PVOID or void*?
> > 
> > It doesn't matter, at least not if the source file includes the
> > Windows header files (where PVOID is defined).
> > 
> >> +  if (reason != /*LDR_DLL_NOTIFICATION_REASON_LOADED*/1)
> > 
> > IMO, it would be better to supply a #define if undefined:
> > 
> > #ifndef LDR_DLL_NOTIFICATION_REASON_LOADED
> > # define LDR_DLL_NOTIFICATION_REASON_LOADED 1
> > #endif
> > 
> 
> I surely can define it. But the ifndef is not needed, since there are no headers 
> containing the function signatures, structures or the defines:
> https://learn.microsoft.com/en-us/windows/win32/devnotes/ldrregisterdllnotification

OK, I wasn't sure about that.

> >> +  if (!GetModuleHandleEx (GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS
> >> +			  | GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT,
> >> +			  (TCHAR*) notification_data->dll_base,
> > 
> > Is TCHAR correct here? does libbacktrace indeed use TCHAR and relies
> > on compile-time definition of UNICODE?  (I'm not familiar with the
> > internals of libbacktrace, so apologies if this is a silly question.)
> > 
> > Thanks.
> 
> As far as I can see it's the first time for TCHAR, I would've gone for 
> GetModuleHandleExW, but https://gcc.gnu.org/pipermail/gcc/2023-January/240534.html

That was about GetModuleHandle, not about GetModuleHandleEx.  For the
latter, all Windows versions that support it also support "wide" APIs.
So my suggestion is to use GetModuleHandleExW here.  However, you will
need to make sure that notification_data->dll_base is declared as
'wchar_t *', not 'char *'.  If dll_base is declared as 'char *', then
only GetModuleHandleExA will work, and you will lose the ability to
support file names with non-ASCII characters outside of the current
system codepage.

> But I didn't want to force GetModuleHandleExA, so I went for TCHAR and 
> GetModuleHandleEx so it automatically chooses which to use. Same for 
> GetModuleHandle of ntdll.dll.

The considerations for GetModuleHandle and for GetModuleHandleEx are
different: the former is also available on old versions of Windows
that doesn't support "wide" APIs.