Re: [PATCH 6/4] libbacktrace: Add loaded dlls after initialize
[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 > > Am 07.01.2024 um 07:50 schrieb Eli Zaretskii: > >> Date: Sat, 6 Jan 2024 23:15:24 +0100 > >> From: Björn Schäpers > >> Cc: gcc-patc...@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.
Stage 4 date
https://gcc.gnu.org/develop.html#timeline The date for stage 4 is listed as the 8th on here, is that date final? There is at least 1 patch pending (mine) that is complete but Jason Merril hasn't been active for a few days. He had expressed to me that he expected the date to be next week on the 14th. Is it possible to push back the date for stage 4 a little bit? I've been working hard on my patch and would be very disappointed if it did not make it in for GCC14. Alex.
Re: [PATCH 6/4] libbacktrace: Add loaded dlls after initialize
Am 07.01.2024 um 15:46 schrieb Eli Zaretskii: [I re-added the other addressees, as I don' think you meant to make this discussion private between the two of us.] Yeah, that was a mistake. Date: Sun, 7 Jan 2024 12:58:29 +0100 From: Björn Schäpers Am 07.01.2024 um 07:50 schrieb Eli Zaretskii: Date: Sat, 6 Jan 2024 23:15:24 +0100 From: Björn Schäpers Cc: gcc-patc...@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. The dll_base is a PVOID. With the GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS flag GetModuleHandleEx does not look for a name, but uses an adress in the module to get the HMODULE, so you cast it to char* or wchar_t* depending on which function you call. Actually one could just cast the dll_base to HMODULE, at least in win32 on x86 the HMODULE of a dll is always its base adress. But to make it safer and future proof I went the way through GetModuleHandeEx. 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.
Re: [PATCH 6/4] libbacktrace: Add loaded dlls after initialize
> Date: Sun, 7 Jan 2024 17:07:06 +0100 > Cc: i...@google.com, gcc-patc...@gcc.gnu.org, gcc@gcc.gnu.org > From: Björn Schäpers > > > 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. > > The dll_base is a PVOID. With the GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS flag > GetModuleHandleEx does not look for a name, but uses an adress in the module > to > get the HMODULE, so you cast it to char* or wchar_t* depending on which > function > you call. Actually one could just cast the dll_base to HMODULE, at least in > win32 on x86 the HMODULE of a dll is always its base adress. But to make it > safer and future proof I went the way through GetModuleHandeEx. In that case, you an call either GetModuleHandeExA or GetModuleHandeExW, the difference is minor.
Re: [PATCH] panic: suppress gnu_printf warning
On Sun, 7 Jan 2024 17:16:41 +0800 Baoquan He wrote:
> with GCC 13.2.1 and W=1, there's compiling warning like this:
>
> kernel/panic.c: In function ‘__warn’:
> kernel/panic.c:676:17: warning: function ‘__warn’ might be a candidate for
> ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
> 676 | vprintk(args->fmt, args->args);
> | ^~~
>
> The normal __printf(x,y) adding can't fix it. So add workaround which
> disables -Wsuggest-attribute=format to mute it.
>
> ...
>
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -666,8 +666,13 @@ void __warn(const char *file, int line, void *caller,
> unsigned taint,
> pr_warn("WARNING: CPU: %d PID: %d at %pS\n",
> raw_smp_processor_id(), current->pid, caller);
>
> +#pragma GCC diagnostic push
> +#ifndef __clang__
> +#pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
> +#endif
> if (args)
> vprintk(args->fmt, args->args);
> +#pragma GCC diagnostic pop
>
> print_modules();
__warn() clearly isn't such a candidate. I'm suspecting that gcc's
implementation of this warning is pretty crude. Is it a new thing in
gcc-13.2?
A bit of context for gcc@gcc.gnu.org:
struct warn_args {
const char *fmt;
va_list args;
};
...
void __warn(const char *file, int line, void *caller, unsigned taint,
struct pt_regs *regs, struct warn_args *args)
{
disable_trace_on_warning();
if (file)
pr_warn("WARNING: CPU: %d PID: %d at %s:%d %pS\n",
raw_smp_processor_id(), current->pid, file, line,
caller);
else
pr_warn("WARNING: CPU: %d PID: %d at %pS\n",
raw_smp_processor_id(), current->pid, caller);
if (args)
vprintk(args->fmt, args->args);
print_modules();
if (regs)
show_regs(regs);
check_panic_on_warn("kernel");
if (!regs)
dump_stack();
print_irqtrace_events(current);
print_oops_end_marker();
trace_error_report_end(ERROR_DETECTOR_WARN, (unsigned long)caller);
/* Just a warning, don't kill lockdep. */
add_taint(taint, LOCKDEP_STILL_OK);
}
Re: Stage 4 date
On 1/7/24 08:48, waffl3x via Gcc wrote: https://gcc.gnu.org/develop.html#timeline The date for stage 4 is listed as the 8th on here, is that date final? There is at least 1 patch pending (mine) that is complete but Jason Merril hasn't been active for a few days. He had expressed to me that he expected the date to be next week on the 14th. Is it possible to push back the date for stage 4 a little bit? I've been working hard on my patch and would be very disappointed if it did not make it in for GCC14. Note there is generally allowances made for patches that were posted before the deadline, but are still going through the review/iterate process. Jeff
Re: Stage 4 date
On Sun, Jan 07, 2024 at 03:12:32PM -0700, Jeff Law via Gcc wrote: > On 1/7/24 08:48, waffl3x via Gcc wrote: > > https://gcc.gnu.org/develop.html#timeline > > The date for stage 4 is listed as the 8th on here, is that date final? > > There is at least 1 patch pending (mine) that is complete but Jason > > Merril hasn't been active for a few days. He had expressed to me that > > he expected the date to be next week on the 14th. Is it possible to > > push back the date for stage 4 a little bit? I've been working hard on > > my patch and would be very disappointed if it did not make it in for > > GCC14. > Note there is generally allowances made for patches that were posted before > the deadline, but are still going through the review/iterate process. Also, not really sure why the stage 4 start is relevant to this patch, it is a new feature, not a bug fix, which admittedly had some versions posted already during stage 1, so if Jason accepts it soon I have no problem with it getting in (in fact I'd appreciate if it got in soon), but end of stage 3 is deadline for patches which are fixing bugs but not regression bugs. Jakub
gcc-14-20240107 is now available
Snapshot gcc-14-20240107 is now available on https://gcc.gnu.org/pub/gcc/snapshots/14-20240107/ and on various mirrors, see https://gcc.gnu.org/mirrors.html for details. This snapshot has been generated from the GCC 14 git branch with the following options: git://gcc.gnu.org/git/gcc.git branch master revision a6b8d8f919c497069caf61c52c5d3b1837129e6b You'll find: gcc-14-20240107.tar.xz Complete GCC SHA256=3b8f00af4744c3694e496151f15ebfa38b08b8fb33a173715a4057f8f8d3526c SHA1=345c4a2ce2f6817bec6fd216bdd587e41ab95bc5 Diffs from 14-20231231 are available in the diffs/ subdirectory. When a particular snapshot is ready for public consumption the LATEST-14 link is updated and a message is sent to the gcc list. Please do not use a snapshot before it has been announced that way.
Re: Stage 4 date
On Sunday, January 7th, 2024 at 3:22 PM, Jakub Jelinek wrote: > > > On Sun, Jan 07, 2024 at 03:12:32PM -0700, Jeff Law via Gcc wrote: > > > On 1/7/24 08:48, waffl3x via Gcc wrote: > > > > > https://gcc.gnu.org/develop.html#timeline > > > The date for stage 4 is listed as the 8th on here, is that date final? > > > There is at least 1 patch pending (mine) that is complete but Jason > > > Merril hasn't been active for a few days. He had expressed to me that > > > he expected the date to be next week on the 14th. Is it possible to > > > push back the date for stage 4 a little bit? I've been working hard on > > > my patch and would be very disappointed if it did not make it in for > > > GCC14. > > > Note there is generally allowances made for patches that were posted before > > the deadline, but are still going through the review/iterate process. > > > Also, not really sure why the stage 4 start is relevant to this patch, > it is a new feature, not a bug fix, which admittedly had some versions > posted already during stage 1, so if Jason accepts it soon I have no problem > with it getting in (in fact I'd appreciate if it got in soon), but end of > stage 3 is deadline for patches which are fixing bugs but not regression bugs. > > Jakub Gotcha, in addition to the other clarifications that were made on IRC this makes sense. I was worried over nothing! Everything has been submitted in so I think it should be in soon. Thanks for the responses, and all the patience I've been shown the past months. Alex
Re: [PATCH] panic: suppress gnu_printf warning
On 01/07/24 at 10:21am, Andrew Morton wrote:
> On Sun, 7 Jan 2024 17:16:41 +0800 Baoquan He wrote:
>
> > with GCC 13.2.1 and W=1, there's compiling warning like this:
> >
> > kernel/panic.c: In function ‘__warn’:
> > kernel/panic.c:676:17: warning: function ‘__warn’ might be a candidate for
> > ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
> > 676 | vprintk(args->fmt, args->args);
> > | ^~~
> >
> > The normal __printf(x,y) adding can't fix it. So add workaround which
> > disables -Wsuggest-attribute=format to mute it.
> >
> > ...
> >
> > --- a/kernel/panic.c
> > +++ b/kernel/panic.c
> > @@ -666,8 +666,13 @@ void __warn(const char *file, int line, void *caller,
> > unsigned taint,
> > pr_warn("WARNING: CPU: %d PID: %d at %pS\n",
> > raw_smp_processor_id(), current->pid, caller);
> >
> > +#pragma GCC diagnostic push
> > +#ifndef __clang__
> > +#pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
> > +#endif
> > if (args)
> > vprintk(args->fmt, args->args);
> > +#pragma GCC diagnostic pop
> >
> > print_modules();
>
> __warn() clearly isn't such a candidate. I'm suspecting that gcc's
> implementation of this warning is pretty crude. Is it a new thing in
> gcc-13.2?
Yeah, this isn't like other warnings in kernel. The compiler seems too
smart by look into the parameter 'args' of 'struct warn_args*'.
>
> A bit of context for gcc@gcc.gnu.org:
>
> struct warn_args {
> const char *fmt;
> va_list args;
> };
>
> ...
>
> void __warn(const char *file, int line, void *caller, unsigned taint,
> struct pt_regs *regs, struct warn_args *args)
> {
> disable_trace_on_warning();
>
> if (file)
> pr_warn("WARNING: CPU: %d PID: %d at %s:%d %pS\n",
> raw_smp_processor_id(), current->pid, file, line,
> caller);
> else
> pr_warn("WARNING: CPU: %d PID: %d at %pS\n",
> raw_smp_processor_id(), current->pid, caller);
>
> if (args)
> vprintk(args->fmt, args->args);
>
> print_modules();
>
> if (regs)
> show_regs(regs);
>
> check_panic_on_warn("kernel");
>
> if (!regs)
> dump_stack();
>
> print_irqtrace_events(current);
>
> print_oops_end_marker();
> trace_error_report_end(ERROR_DETECTOR_WARN, (unsigned long)caller);
>
> /* Just a warning, don't kill lockdep. */
> add_taint(taint, LOCKDEP_STILL_OK);
> }
>
