Hi Arnd,
+CC: Kees Cook
On Sun. 8 May 2022 at 19:27, Arnd Bergmann <[email protected]> wrote:
> On Sun, May 8, 2022 at 12:09 PM Vincent Mailhol
> <[email protected]> wrote:
> >
> > Aside of the __builtin_foo() ones, x86 does not directly rely on any
> > builtin functions.
> >
> > However, such builtin functions are not explicitly deactivated,
> > creating some collisions, concrete example being ffs() from bitops.h,
> > c.f.:
> >
> > | ./arch/x86/include/asm/bitops.h:283:28: warning: declaration of 'ffs' shadows a built-in function [-Wshadow]
> > | 283 | static __always_inline int ffs(int x)
> >
> > This patch adds -fno-builtin to KBUILD_CFLAGS for the x86
> > architectures in order to prevent shadowing of builtin functions.
> >
> > Signed-off-by: Vincent Mailhol <[email protected]>
> > ---
> > FYI, I tested this patch on a "make allyesconfig" for both x86_32 and
> > x86_64.
> >
> > This is a resend. Only difference is that I dropped the RFC flag and
> > added Arnd in CC because he did a similar patch to fix ffs shadow
> > warnings in the past:
> >
> > https://lore.kernel.org/all/[email protected]/
>
> I think this is a correct change, but unfortunately it exposes a clang bug
> with -mregparm=3. Nick should be able to provide more details, I think
> he has a plan.
Interesting. I admittedly did not do extensive tests on clang
but I would have expected the Linux kernel bot to have warned me
on my previous patch.
I did research on mregparm and clang. I found this thread:
https://lore.kernel.org/r/[email protected]
and the associated LLVM issue:
https://github.com/llvm/llvm-project/issues/53645
Those threads mention that some clang builtins become unusable
when combining -mregparm=3 and -m32. But I could not find a
bug reference about -mregparm=3 and -fno-builtin combination.
Could you just double confirm that you indeed saw the issue with
-fno-builtin? If that the case, I am really curious to get the
details :)
Yours sincerely,
Vincent Mailhol
On Sun, May 08, 2022 at 09:37:14PM +0900, Vincent MAILHOL wrote:
> Hi Arnd,
>
> +CC: Kees Cook
>
> On Sun. 8 May 2022 at 19:27, Arnd Bergmann <[email protected]> wrote:
> > On Sun, May 8, 2022 at 12:09 PM Vincent Mailhol
> > <[email protected]> wrote:
> > >
> > > Aside of the __builtin_foo() ones, x86 does not directly rely on any
> > > builtin functions.
> > >
> > > However, such builtin functions are not explicitly deactivated,
> > > creating some collisions, concrete example being ffs() from bitops.h,
> > > c.f.:
> > >
> > > | ./arch/x86/include/asm/bitops.h:283:28: warning: declaration of 'ffs' shadows a built-in function [-Wshadow]
> > > | 283 | static __always_inline int ffs(int x)
> > >
> > > This patch adds -fno-builtin to KBUILD_CFLAGS for the x86
> > > architectures in order to prevent shadowing of builtin functions.
> > >
> > > Signed-off-by: Vincent Mailhol <[email protected]>
> > > ---
> > > FYI, I tested this patch on a "make allyesconfig" for both x86_32 and
> > > x86_64.
> > >
> > > This is a resend. Only difference is that I dropped the RFC flag and
> > > added Arnd in CC because he did a similar patch to fix ffs shadow
> > > warnings in the past:
> > >
> > > https://lore.kernel.org/all/[email protected]/
> >
> > I think this is a correct change, but unfortunately it exposes a clang bug
> > with -mregparm=3. Nick should be able to provide more details, I think
> > he has a plan.
>
> Interesting. I admittedly did not do extensive tests on clang
> but I would have expected the Linux kernel bot to have warned me
> on my previous patch.
>
> I did research on mregparm and clang. I found this thread:
> https://lore.kernel.org/r/[email protected]
>
> and the associated LLVM issue:
> https://github.com/llvm/llvm-project/issues/53645
>
> Those threads mention that some clang builtins become unusable
> when combining -mregparm=3 and -m32. But I could not find a
> bug reference about -mregparm=3 and -fno-builtin combination.
>
> Could you just double confirm that you indeed saw the issue with
> -fno-builtin? If that the case, I am really curious to get the
> details :)
-ffreestanding implies -fno-builtin; removing -ffreestanding from
arch/x86/Makefile for 32-bit x86 caused the problem so I don't think
that your patch would cause any issue but I could be missing something.
However, doesn't -fno-builtin remove the ability for GCC and clang to
perform certain libcall optimizations? I seem to recall this coming up
in previous threads but I am having a hard time finding the exact
language that I was looking for. This thread seems to be the most recent
one that I can remember:
https://lore.kernel.org/CAHk-=whn91ar+EbcBXQb9UXad00Q5WjU-TCB6UBzVba682a4ew@mail.gmail.com/
Cheers,
Nathan
On Mon. 9 May 2022 at 08:51, Nathan Chancellor <[email protected]> wrote:
> On Sun, May 08, 2022 at 09:37:14PM +0900, Vincent MAILHOL wrote:
> > Hi Arnd,
> >
> > +CC: Kees Cook
> >
> > On Sun. 8 May 2022 at 19:27, Arnd Bergmann <[email protected]> wrote:
> > > On Sun, May 8, 2022 at 12:09 PM Vincent Mailhol
> > > <[email protected]> wrote:
> > > >
> > > > Aside of the __builtin_foo() ones, x86 does not directly rely on any
> > > > builtin functions.
> > > >
> > > > However, such builtin functions are not explicitly deactivated,
> > > > creating some collisions, concrete example being ffs() from bitops.h,
> > > > c.f.:
> > > >
> > > > | ./arch/x86/include/asm/bitops.h:283:28: warning: declaration of 'ffs' shadows a built-in function [-Wshadow]
> > > > | 283 | static __always_inline int ffs(int x)
> > > >
> > > > This patch adds -fno-builtin to KBUILD_CFLAGS for the x86
> > > > architectures in order to prevent shadowing of builtin functions.
> > > >
> > > > Signed-off-by: Vincent Mailhol <[email protected]>
> > > > ---
> > > > FYI, I tested this patch on a "make allyesconfig" for both x86_32 and
> > > > x86_64.
> > > >
> > > > This is a resend. Only difference is that I dropped the RFC flag and
> > > > added Arnd in CC because he did a similar patch to fix ffs shadow
> > > > warnings in the past:
> > > >
> > > > https://lore.kernel.org/all/[email protected]/
> > >
> > > I think this is a correct change, but unfortunately it exposes a clang bug
> > > with -mregparm=3. Nick should be able to provide more details, I think
> > > he has a plan.
> >
> > Interesting. I admittedly did not do extensive tests on clang
> > but I would have expected the Linux kernel bot to have warned me
> > on my previous patch.
> >
> > I did research on mregparm and clang. I found this thread:
> > https://lore.kernel.org/r/[email protected]
> >
> > and the associated LLVM issue:
> > https://github.com/llvm/llvm-project/issues/53645
> >
> > Those threads mention that some clang builtins become unusable
> > when combining -mregparm=3 and -m32. But I could not find a
> > bug reference about -mregparm=3 and -fno-builtin combination.
> >
> > Could you just double confirm that you indeed saw the issue with
> > -fno-builtin? If that the case, I am really curious to get the
> > details :)
>
> -ffreestanding implies -fno-builtin; removing -ffreestanding from
> arch/x86/Makefile for 32-bit x86 caused the problem so I don't think
> that your patch would cause any issue but I could be missing something.
>
> However, doesn't -fno-builtin remove the ability for GCC and clang to
> perform certain libcall optimizations? I seem to recall this coming up
> in previous threads but I am having a hard time finding the exact
> language that I was looking for.
I was not aware. I did the test with a dummy memset implementation:
| void foo(char *s, unsigned int count)
| {
| while (count--)
| *s++ = 0;
| }
Before this patch (i.e. with builtins), GCC does this:
| foo:
| testl %esi, %esi # count
| je .L7 #,
| pushq %rbp #
| movl %esi, %edx # count, count
| xorl %esi, %esi #
| movq %rsp, %rbp #,
| call memset #
| popq %rbp #
| ret
| .L7:
| ret
Here, we can clearly see that the function is optimized to a call
to memset.
After this patch (i.e. without builtins), the call disappears:
| foo:
| testl %esi, %esi # count
| je .L1 #,
| movl %esi, %esi # count, count
| leaq (%rdi,%rsi), %rax #, _12
| .L3:
| addq $1, %rdi #, s
| movb $0, -1(%rdi) #, MEM[(char *)s_8 + -1B]
| cmpq %rax, %rdi # _12, s
| jne .L3 #,
| .L1:
| ret
So yes, the -fno-builtin will remove the optimizations at least
for GCC (not tested for clang).
> This thread seems to be the most recent
> one that I can remember:
>
> https://lore.kernel.org/CAHk-=whn91ar+EbcBXQb9UXad00Q5WjU-TCB6UBzVba682a4ew@mail.gmail.com/
What is funny is that the thread you are pointing at mostly
complains about the compiler doing optimization in the user's
back.
There will be some cases in which the compiler will find valid
optimizations and some cases it will do some silly
one (e.g. transform a very small loop to a function call). The
problem is to know whether the clever optimizations outweigh the silly
ones or not. I wonder if any benchmark exists on that.
If compiler optimizations are indeed worth it, we should then have
a look at the other architecture which uses the -fno-builtin flag
(or the -ffreestanding).
Regarding this patch, I do not think it should be applied anymore
unless proven that "optimizations" are detrimental and thus
unwanted.
Instead, I am thinking of just using -fno-builtin-ffs to remove
the annoying -Wshadow warning. Would that make more sense?
Thank you.
Yours sincerely,
Vincent Mailhol
On Mon, May 9, 2022 at 8:01 AM Vincent MAILHOL
<[email protected]> 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.
--
Thanks,
~Nick Desaulniers
Hi Nick,
On Tue. 10 May 2022 at 04:50, Nick Desaulniers <[email protected]> wrote:
> On Mon, May 9, 2022 at 8:01 AM Vincent MAILHOL
> <[email protected]> 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).
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
On Mon, May 9, 2022 at 4:12 PM Vincent MAILHOL
<[email protected]> wrote:
>
> Hi Nick,
>
> On Tue. 10 May 2022 at 04:50, Nick Desaulniers <[email protected]> wrote:
> > On Mon, May 9, 2022 at 8:01 AM Vincent MAILHOL
> > <[email protected]> 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
On Tue. 10 May 2022 at 08:26, Nick Desaulniers <[email protected]> wrote:
> On Mon, May 9, 2022 at 4:12 PM Vincent MAILHOL
> <[email protected]> wrote:
> >
> > Hi Nick,
> >
> > On Tue. 10 May 2022 at 04:50, Nick Desaulniers <[email protected]> wrote:
> > > On Mon, May 9, 2022 at 8:01 AM Vincent MAILHOL
> > > <[email protected]> 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)
> 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
On Tue. 10 May 2022 at 10:10, Vincent MAILHOL
<[email protected]> wrote:
> On Tue. 10 May 2022 at 08:26, Nick Desaulniers <[email protected]> wrote:
> > On Mon, May 9, 2022 at 4:12 PM Vincent MAILHOL
> > <[email protected]> wrote:
> > >
> > > Hi Nick,
> > >
> > > On Tue. 10 May 2022 at 04:50, Nick Desaulniers <[email protected]> wrote:
> > > > On Mon, May 9, 2022 at 8:01 AM Vincent MAILHOL
> > > > <[email protected]> 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/[email protected]/T/#m55da229f67d2c84470a55df32e71d8600c581024
This solves the -Wshadow and also adds some optimizations for when
ffs() is called with constant variables.
Yours sincerely,
Vincent Mailhol