linux-kernel - Re: [PATCH] sched/core: silence a warning in sched

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 24 Jun 2019 18:24:37 +0100
From:   Valentin Schneider <valentin.schneider@....com>
To:     Qian Cai <cai@....pw>, mingo@...hat.com, peterz@...radead.org
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH] sched/core: silence a warning in sched_init()

On 24/06/2019 14:59, Qian Cai wrote:
> Compiling a kernel with both FAIR_GROUP_SCHED=n and RT_GROUP_SCHED=n
> will generate a warning using W=1,
> 
> kernel/sched/core.c: In function 'sched_init':
> kernel/sched/core.c:5906:32: warning: variable 'ptr' set but not used
> [-Wunused-but-set-variable]
>   unsigned long alloc_size = 0, ptr;
>                                 ^~~
> 
> It apparently the maintainers don't like the proper fix [1] which
> contains ugly idefs, so silence it by appending the __maybe_unused
> attribute for it instead.
> 

As you say, ifdefs are ugly. I try to stay away from them as much as
possible (as you may have noticed with the NOHZ stuff). They just make
code harder to read (I know someone who'd say "it's free code obfuscation"),
and that is especially true in the patch you reference where it conditions
variable declaration.

So it may not have been the "proper" fix after all...

> [1] https://lore.kernel.org/lkml/1559681162-5385-1-git-send-email-cai@lca.pw/
> 
> Signed-off-by: Qian Cai <cai@....pw>
> ---
>  kernel/sched/core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 874c427742a9..9bbad91b3f01 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -5903,7 +5903,10 @@ int in_sched_functions(unsigned long addr)
>  void __init sched_init(void)
>  {
>  	int i, j;
> -	unsigned long alloc_size = 0, ptr;
> +	unsigned long alloc_size = 0;
> +
> +	/* To silence a -Wunused-but-set-variable warning. */
> +	unsigned long ptr __maybe_unused;
>  

To be super nitpicky, I'd argue the __maybe_unused attribute should be
placed before the variable name - the kernel/sched/* code goes with
prefixing rather than postfixing (there is *one* exception in deadline.c),
and I personally prefer prefixing as well (it just reads better).

To be (still, but slightly less) nitpicky, I'd remove the added newline
(keep the variable declaration as a single block) and even remove the
comment - it's redundant with the attribute.

With those changes:
Reviewed-by: Valentin Schneider <valentin.schneider@....com>

>  	wait_bit_init();
>  
>