Re: [RESEND PATCH v1] x86/build: add -fno-builtin flag to prevent shadowing

[Nick Desaulniers]

On Mon, May 9, 2022 at 4:12 PM Vincent MAILHOL
<mailhol.vincent@xxxxxxxxxx> wrote:
>
> Hi Nick,
>
> On Tue. 10 May 2022 at 04:50, Nick Desaulniers <ndesaulniers@xxxxxxxxxx> wrote:
> > On Mon, May 9, 2022 at 8:01 AM Vincent MAILHOL
> > <mailhol.vincent@xxxxxxxxxx> wrote:
> > >
> > > Instead, I am thinking of just using -fno-builtin-ffs to remove
> > > the annoying -Wshadow warning. Would that make more sense?
> >
> > Perhaps a pragma would be the best tool to silence this instance of
> > -Wshadow?  I understand what GCC is trying to express, but the kernel
> > does straddle a weird place between -ffreestanding and a "hosted" env.
>
> I was a bit reluctant to propose the use of pragma because I received
> negative feedback in another patch for using the __diag_ignore()
> c.f.:
> https://lore.kernel.org/all/YmhZSZWg9YZZLRHA@yury-laptop/
>
> But the context here is a bit different, I guess. If I receive your support, I
> am fully OK to silence this with some #pragma.
>
> The patch would look as below (I just need to test with clang
> before submitting).

Do you have a sense for how many other functions trigger -Wshadow? For
example, one question I have is:
Why does ffs() trigger this, but not any of the functions defined in
lib/string.c (or declared in include/linux/string.h) which surely also
shadow existing builtins?  I can't see your example being sprinkled
all over include/linux/string.h as being ok.

If it's more than just ffs(), perhaps the GCC developers can split the
shadowing of builtins into a sub flag under -Wshadow that can then be
disabled; we do want to shadow these functions, but -Wno-shadow would
miss warnings on variables being shadowed due to scope.

We've done this in the past with various flags in clang. Rather than
having semantic analysis trigger the same warning flag for different
concerns, we split the flag into distinct concerns, and reuse the
original flag as a group that enables the new flags. This gives
developers fine grain control over enabling/disabling distinct
concerns.

>
> diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h
> index a288ecd230ab..e44911253bdf 100644
> --- a/arch/x86/include/asm/bitops.h
> +++ b/arch/x86/include/asm/bitops.h
> @@ -269,6 +269,9 @@ static __always_inline unsigned long
> __fls(unsigned long word)
>  #undef ADDR
>
>  #ifdef __KERNEL__
> +__diag_push();
> +__diag_ignore_all("-Wshadow",
> +                  "-fno-builtin-foo would remove optimization, just
> silence it instead");
>  /**
>   * ffs - find first set bit in word
>   * @x: the word to search
> @@ -309,6 +312,7 @@ static __always_inline int ffs(int x)
>  #endif
>         return r + 1;
>  }
> +__diag_pop(); /* ignore -Wshadow */
>
>  /**
>   * fls - find last set bit in word



-- 
Thanks,
~Nick Desaulniers