Received-SPF: pass (google.com: domain of linux-kernel+bounces-132225-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 4 Apr 2024 15:53:49 -0700
Message-ID: <CAHk-=whHWjKK1TOMT1XvxFj8e-_uctJnXPxM=SyWHmW63B_EDw@mail.gmail.com>
Subject: More annoying code generation by clang
To: Ingo Molnar <mingo@kernel.org>, Thomas Gleixner <tglx@linutronix.de>, Peter Anvin <hpa@zytor.com>
Cc: "the arch/x86 maintainers" <x86@kernel.org>, 
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: multipart/mixed; boundary="00000000000067b57c06154d378e"

--00000000000067b57c06154d378e
Content-Type: text/plain; charset="UTF-8"

So this doesn't really matter in any real life situation, but it
really grated on me.

Clang has this nasty habit of taking our nice asm constraints, and
turning them into worst-case garbage. It's been reported a couple of
times where we use "g" to tell the compiler that pretty much any
source to the asm works, and then clang takes that to mean "I will
take that to use 'memory'" even when that makes no sense what-so-ever.

See for example

    https://lore.kernel.org/all/CAHk-=wgobnShg4c2yyMbk2p=U-wmnOmX_0=b3ZY_479Jjey2xw@mail.gmail.com/

where I was ranting about clang just doing pointlessly stupid things.

However, I found a case where yes, clang does pointlessly stupid
things, but it's at least _partly_ our fault, and gcc can't generate
optimal code either.

We have this fairly critical code in __fget_files_rcu() to look up a
'struct file *' from an fd, and it does this:

                /* Mask is a 0 for invalid fd's, ~0 for valid ones */
                nospec_mask = array_index_mask_nospec(fd, fdt->max_fds);

and clang makes a *horrid* mess of it, generating this code:

        movl    %edi, %r14d
        movq    32(%rbx), %rdx
        movl    (%rdx), %eax
        movq    %rax, 8(%rsp)
        cmpq    8(%rsp), %r14
        sbbq    %rcx, %rcx

which is just crazy. Notice how it does that "move rax to stack, then
do the compare against the stack", instead of just using %rax.

In fact, that function shouldn't have a stack frame at all, and the
only reason it is generated is because of this whole oddity.

All clang's fault, right?

Yeah, mostly. But it turns out that what really messes with clangs
little head is that the x86 array_index_mask_nospec() function is
being a bit annoying.

This is what we do:

  static __always_inline unsigned long
array_index_mask_nospec(unsigned long index,
                unsigned long size)
  {
        unsigned long mask;

        asm volatile ("cmp %1,%2; sbb %0,%0;"
                        :"=r" (mask)
                        :"g"(size),"r" (index)
                        :"cc");
        return mask;
  }

and look at the use again:

        nospec_mask = array_index_mask_nospec(fd, fdt->max_fds);

here all the values are actually 'unsigned int'. So what happens is
that clang can't just use the fdt->max_fds value *directly* from
memory, because it needs to be expanded from 32-bit to 64-bit because
we've made our array_index_mask_nospec() function only work on 64-bit
'unsigned long' values.

So it turns out that by massaging this a bit, and making it just be a
macro - so that the asm can decide that "I can do this in 32-bit" - I
can get clang to generate much better code.

Clang still absolutely hates the "g" constraint, so to get clang to
really get this right I have to use "ir" instead of "g". Which is
wrong. Because gcc does this right, and could use the memory op
directly. But even gcc cannot do that with our *current* function,
because of that "the memory value is 32-bit, we require a 64-bit
value"

Anyway, I can get gcc to generate the right code:

        movq    32(%r13), %rdx
        cmp (%rdx),%ebx
        sbb %esi,%esi

which is basically the right code for the six crazy instructions clang
generates. And if I make the "g" be "ir", I can get clang to generate

        movq    32(%rdi), %rcx
        movl    (%rcx), %eax
        cmpl    %eax, %esi
        sbbl    %esi, %esi

which is the same thing, but with that (pointless) load to a register.

And now clang doesn't generate that stack frame at all.

Anyway, this was a long email to explain the odd attached patch.

Comments? Note that this patch is *entirely* untested, I have done
this purely by looking at the code generation in fs/file.c.

                Linus

--00000000000067b57c06154d378e
Content-Type: text/x-patch; charset="US-ASCII"; name="patch.diff"
Content-Disposition: attachment; filename="patch.diff"
Content-Transfer-Encoding: base64
Content-ID: <f_lulu0arv0>
X-Attachment-Id: f_lulu0arv0

IGFyY2gveDg2L2luY2x1ZGUvYXNtL2JhcnJpZXIuaCB8IDIzICsrKysrKysrKy0tLS0tLS0tLS0t
LS0tCiAxIGZpbGUgY2hhbmdlZCwgOSBpbnNlcnRpb25zKCspLCAxNCBkZWxldGlvbnMoLSkKCmRp
ZmYgLS1naXQgYS9hcmNoL3g4Ni9pbmNsdWRlL2FzbS9iYXJyaWVyLmggYi9hcmNoL3g4Ni9pbmNs
dWRlL2FzbS9iYXJyaWVyLmgKaW5kZXggNjZlNTdjMDEwMzkyLi42MTU5ZDJjYmJmZGUgMTAwNjQ0
Ci0tLSBhL2FyY2gveDg2L2luY2x1ZGUvYXNtL2JhcnJpZXIuaAorKysgYi9hcmNoL3g4Ni9pbmNs
dWRlL2FzbS9iYXJyaWVyLmgKQEAgLTMzLDIwICszMywxNSBAQAogICogUmV0dXJuczoKICAqICAg
ICAwIC0gKGluZGV4IDwgc2l6ZSkKICAqLwotc3RhdGljIF9fYWx3YXlzX2lubGluZSB1bnNpZ25l
ZCBsb25nIGFycmF5X2luZGV4X21hc2tfbm9zcGVjKHVuc2lnbmVkIGxvbmcgaW5kZXgsCi0JCXVu
c2lnbmVkIGxvbmcgc2l6ZSkKLXsKLQl1bnNpZ25lZCBsb25nIG1hc2s7Ci0KLQlhc20gdm9sYXRp
bGUgKCJjbXAgJTEsJTI7IHNiYiAlMCwlMDsiCi0JCQk6Ij1yIiAobWFzaykKLQkJCToiZyIoc2l6
ZSksInIiIChpbmRleCkKLQkJCToiY2MiKTsKLQlyZXR1cm4gbWFzazsKLX0KLQotLyogT3ZlcnJp
ZGUgdGhlIGRlZmF1bHQgaW1wbGVtZW50YXRpb24gZnJvbSBsaW51eC9ub3NwZWMuaC4gKi8KLSNk
ZWZpbmUgYXJyYXlfaW5kZXhfbWFza19ub3NwZWMgYXJyYXlfaW5kZXhfbWFza19ub3NwZWMKKyNk
ZWZpbmUgYXJyYXlfaW5kZXhfbWFza19ub3NwZWMoaWR4LHN6KSAoewlcCisJdHlwZW9mKChpZHgp
KyhzeikpIF9faWR4ID0gKGlkeCk7CVwKKwl0eXBlb2YoX19pZHgpIF9fc3ogPSAoc3opOwkJXAor
CXR5cGVvZihfX2lkeCkgX19tYXNrOwkJCVwKKwlhc20gdm9sYXRpbGUgKCJjbXAgJTEsJTI7IHNi
YiAlMCwlMCIJXAorCQkJOiI9ciIgKF9fbWFzaykJCVwKKwkJCToiaXIiKF9fc3opLCJyIiAoX19p
ZHgpCVwKKwkJCToiY2MiKTsJCQlcCisJX19tYXNrOyB9KQogCiAvKiBQcmV2ZW50IHNwZWN1bGF0
aXZlIGV4ZWN1dGlvbiBwYXN0IHRoaXMgYmFycmllci4gKi8KICNkZWZpbmUgYmFycmllcl9ub3Nw
ZWMoKSBhc20gdm9sYXRpbGUoImxmZW5jZSI6OjoibWVtb3J5IikK
--00000000000067b57c06154d378e--