lkml.org 
[lkml]   [2022]   [May]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [RESEND PATCH v1] x86/build: add -fno-builtin flag to prevent shadowing
On Tue. 10 May 2022 at 10:10, Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> wrote:
> On Tue. 10 May 2022 at 08:26, Nick Desaulniers <ndesaulniers@google.com> wrote:
> > On Mon, May 9, 2022 at 4:12 PM Vincent MAILHOL
> > <mailhol.vincent@wanadoo.fr> wrote:
> > >
> > > Hi Nick,
> > >
> > > On Tue. 10 May 2022 at 04:50, Nick Desaulniers <ndesaulniers@google.com> wrote:
> > > > On Mon, May 9, 2022 at 8:01 AM Vincent MAILHOL
> > > > <mailhol.vincent@wanadoo.fr> 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?
>
> I only witnessed such -Wshadow warnings for ffs().
>
> > 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.
>
> Thanks, you are touching on a really interesting point.
>
> After checking, the other builtin functions declare the function with
> two leading underscores (e.g. __foo(...)) and then do:
>
> #define foo(...) __foo(...)
>
> Or alternatively, if using the builtin function:
>
> #define foo(...) __builtin_foo(...)
>
> Compilers do not trigger the -Wshadow for such patterns.
>
> Example with memcpy():
> https://elixir.bootlin.com/linux/latest/source/arch/x86/include/asm/string_64.h#L75
>
> So, in light of your comment doing this would be more consistent:
>
> #define ffs(x) _ffs(x)

I created this patch:
https://lore.kernel.org/all/20220510142550.1686866-1-mailhol.vincent@wanadoo.fr/T/#m55da229f67d2c84470a55df32e71d8600c581024

This solves the -Wshadow and also adds some optimizations for when
ffs() is called with constant variables.


Yours sincerely,
Vincent Mailhol

\
 
 \ /
  Last update: 2022-05-10 17:04    [W:0.051 / U:0.280 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site