There's a bunch of callbacks with unused arguments, go ahead and silence
those so "make KCFLAGS=-W init/main.o" is a little quieter.
Here's a little sample:
init/main.c:182:43: warning: unused parameter 'str' [-Wunused-parameter]
static int __init set_reset_devices(char *str)
Signed-off-by: Andrew Halaney <[email protected]>
---
init/main.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/init/main.c b/init/main.c
index eb01e121d2f1..4c26f0ea5073 100644
--- a/init/main.c
+++ b/init/main.c
@@ -179,7 +179,7 @@ EXPORT_SYMBOL_GPL(static_key_initialized);
unsigned int reset_devices;
EXPORT_SYMBOL(reset_devices);
-static int __init set_reset_devices(char *str)
+static int __init set_reset_devices(char *str __always_unused)
{
reset_devices = 1;
return 1;
@@ -229,13 +229,13 @@ static bool __init obsolete_checksetup(char *line)
unsigned long loops_per_jiffy = (1<<12);
EXPORT_SYMBOL(loops_per_jiffy);
-static int __init debug_kernel(char *str)
+static int __init debug_kernel(char *str __always_unused)
{
console_loglevel = CONSOLE_LOGLEVEL_DEBUG;
return 0;
}
-static int __init quiet_kernel(char *str)
+static int __init quiet_kernel(char *str __always_unused)
{
console_loglevel = CONSOLE_LOGLEVEL_QUIET;
return 0;
@@ -478,7 +478,7 @@ static void __init setup_boot_config(void)
get_boot_config_from_initrd(NULL, NULL);
}
-static int __init warn_bootconfig(char *str)
+static int __init warn_bootconfig(char *str __always_unused)
{
pr_warn("WARNING: 'bootconfig' found on the kernel command line but CONFIG_BOOT_CONFIG is not set.\n");
return 0;
@@ -504,7 +504,8 @@ static void __init repair_env_string(char *param, char *val)
/* Anything after -- gets handed straight to init. */
static int __init set_init_arg(char *param, char *val,
- const char *unused, void *arg)
+ const char *unused __always_unused,
+ void *arg __always_unused)
{
unsigned int i;
@@ -529,7 +530,8 @@ static int __init set_init_arg(char *param, char *val,
* unused parameters (modprobe will find them in /proc/cmdline).
*/
static int __init unknown_bootoption(char *param, char *val,
- const char *unused, void *arg)
+ const char *unused __always_unused,
+ void *arg __always_unused)
{
size_t len = strlen(param);
@@ -723,7 +725,8 @@ noinline void __ref rest_init(void)
/* Check for early params. */
static int __init do_early_param(char *param, char *val,
- const char *unused, void *arg)
+ const char *unused __always_unused,
+ void *arg __always_unused)
{
const struct obs_kernel_param *p;
@@ -1301,8 +1304,10 @@ static const char *initcall_level_names[] __initdata = {
"late",
};
-static int __init ignore_unknown_bootoption(char *param, char *val,
- const char *unused, void *arg)
+static int __init ignore_unknown_bootoption(char *param __always_unused,
+ char *val __always_unused,
+ const char *unused __always_unused,
+ void *arg __always_unused)
{
return 0;
}
@@ -1440,7 +1445,7 @@ void __weak free_initmem(void)
free_initmem_default(POISON_FREE_INITMEM);
}
-static int __ref kernel_init(void *unused)
+static int __ref kernel_init(void *unused __always_unused)
{
int ret;
--
2.31.1
On Wed, 19 May 2021 11:23:41 -0500 Andrew Halaney <[email protected]> wrote:
> There's a bunch of callbacks with unused arguments, go ahead and silence
> those so "make KCFLAGS=-W init/main.o" is a little quieter.
> Here's a little sample:
Do we care about -Wunused-parameter? I suppose we do, as it might
point us at small code optimizations.
How voluminous is the warning output from -Wunused-parameter? Small
enough to be useful or large enough to be useless?
On Wed, May 19, 2021 at 09:37:31PM -0700, Andrew Morton wrote:
> On Wed, 19 May 2021 11:23:41 -0500 Andrew Halaney <[email protected]> wrote:
>
> > There's a bunch of callbacks with unused arguments, go ahead and silence
> > those so "make KCFLAGS=-W init/main.o" is a little quieter.
> > Here's a little sample:
>
> Do we care about -Wunused-parameter? I suppose we do, as it might
> point us at small code optimizations.
>
> How voluminous is the warning output from -Wunused-parameter? Small
> enough to be useful or large enough to be useless?
>
That's something I was wondering too. The output from compiling with -W
is _very_ loud, to the point where it is almost pointless to do it. Even
with this patch applied I get 1679 warnings generated when doing a
recompile of init/main.o - all but one of them from headers included.
The motivation was brought up because item 20 in [1] says:
20) Newly-added code has been compiled with ``gcc -W`` (use
``make KCFLAGS=-W``). This will generate lots of noise, but is good
for finding bugs like "warning: comparison between signed and unsigned".
and while none of this is newly added code, I found it pretty hard to
discern in a prior patch here if I was causing extra noise or not.
Thought I'd chip away at the noise.
If we decide we don't care about such warnings then feel free to ignore
this patch, but since I was playing around here anyways I thought I'd
clean it up a little. My preference would be to care, but the output is
so loud it is easy to make the argument that it is too late to start
caring.
Thanks,
Andrew
[1] https://www.kernel.org/doc/html/latest/process/submit-checklist.html#submitchecklist
On 20/05/2021 15.03, Andrew Halaney wrote:
> On Wed, May 19, 2021 at 09:37:31PM -0700, Andrew Morton wrote:
>> On Wed, 19 May 2021 11:23:41 -0500 Andrew Halaney <[email protected]> wrote:
>>
> If we decide we don't care about such warnings then feel free to ignore
> this patch, but since I was playing around here anyways I thought I'd
> clean it up a little. My preference would be to care, but the output is
> so loud it is easy to make the argument that it is too late to start
> caring.
Those always-unused annotations are quite verbose. Could we instead
allow both int (*)(char *) and int (*)(void) functions via some macro
magic, say extending __setup_param to something like (entirely untested)
#define __setup_param(str, unique_id, fn, early) \
static const char __setup_str_##unique_id[] __initconst \
__aligned(1) = str; \
+ static_assert(same_type(&fn, int (*)(char *)) || same_type(&fn,
int (*)(void)));
static struct obs_kernel_param __setup_##unique_id \
__used __section(".init.setup") \
__aligned(__alignof__(struct obs_kernel_param)) \
- = { __setup_str_##unique_id, fn, early }
+ = { __setup_str_##unique_id, (int (*)(char *)fn, early }
That would still require modifying each callback, but then it would be
to the more plain-C
int foo(void) // yeah, this doesn't use any parameters
I checked, the transparent_union trick doesn't work for static
initialization.
But really, the compiler should have some heuristic that said "hm, ok,
the address of this function was taken so it probably has that prototype
because it has to be that way". I bet that would remove 90% of the
Wunused-parameter noise.
Rasmus
On Fri, May 21, 2021 at 09:34:45AM +0200, Rasmus Villemoes wrote:
> On 20/05/2021 15.03, Andrew Halaney wrote:
> > On Wed, May 19, 2021 at 09:37:31PM -0700, Andrew Morton wrote:
> >> On Wed, 19 May 2021 11:23:41 -0500 Andrew Halaney <[email protected]> wrote:
> >>
>
> > If we decide we don't care about such warnings then feel free to ignore
> > this patch, but since I was playing around here anyways I thought I'd
> > clean it up a little. My preference would be to care, but the output is
> > so loud it is easy to make the argument that it is too late to start
> > caring.
>
> Those always-unused annotations are quite verbose. Could we instead
> allow both int (*)(char *) and int (*)(void) functions via some macro
> magic, say extending __setup_param to something like (entirely untested)
>
> #define __setup_param(str, unique_id, fn, early) \
> static const char __setup_str_##unique_id[] __initconst \
> __aligned(1) = str; \
> + static_assert(same_type(&fn, int (*)(char *)) || same_type(&fn,
> int (*)(void)));
> static struct obs_kernel_param __setup_##unique_id \
> __used __section(".init.setup") \
> __aligned(__alignof__(struct obs_kernel_param)) \
> - = { __setup_str_##unique_id, fn, early }
> + = { __setup_str_##unique_id, (int (*)(char *)fn, early }
>
> That would still require modifying each callback, but then it would be
> to the more plain-C
>
> int foo(void) // yeah, this doesn't use any parameters
>
> I checked, the transparent_union trick doesn't work for static
> initialization.
>
> But really, the compiler should have some heuristic that said "hm, ok,
> the address of this function was taken so it probably has that prototype
> because it has to be that way". I bet that would remove 90% of the
> Wunused-parameter noise.
>
> Rasmus
>
Sorry for the way late reply Rasmus, my spam filter has been flagging a
few things incorrectly and this was one.
I'll be completely honest, both the macro magic and teaching the
compiler are things I'm not familiar with. If I get some free
time I'll look more into the macro magic approach to see if anything is
possible, but I wouldn't hold your breath for a speedy update :P
Thanks,
Andrew