Messages in this thread | | | From | Vincent MAILHOL <> | Date | Tue, 10 May 2022 23:33:13 +0900 | Subject | Re: [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
| |