Date:   Tue, 10 May 2022 00:00:48 +0900
From:   Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To:     Nathan Chancellor <nathan@...nel.org>
Cc:     Arnd Bergmann <arnd@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        "H . Peter Anvin" <hpa@...or.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        clang-built-linux <llvm@...ts.linux.dev>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Tom Rix <trix@...hat.com>, Kees Cook <keescook@...omium.org>
Subject: Re: [RESEND PATCH v1] x86/build: add -fno-builtin flag to prevent shadowing

On Mon. 9 May 2022 at 08:51, Nathan Chancellor <nathan@...nel.org> 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 <arnd@...nel.org> wrote:
> > > On Sun, May 8, 2022 at 12:09 PM Vincent Mailhol
> > > <mailhol.vincent@...adoo.fr> 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 <mailhol.vincent@...adoo.fr>
> > > > ---
> > > > 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/20201026160006.3704027-1-arnd@kernel.org/
> > >
> > > 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/20220208225350.1331628-9-keescook@chromium.org
> >
> > 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

Powered by blists - more mailing lists