Re: [GIT PULL] string fixes for v6.15-rc1

Re: [GIT PULL] string fixes for v6.15-rc1

[Nathan Chancellor]

Hi Linus,

On Sun, Apr 06, 2025 at 07:04:29PM -0700, Linus Torvalds wrote:
> On Sun, 6 Apr 2025 at 18:33, Kees Cook <kees@kernel.org> wrote:
> >
> > I should have said "libcall optimizations". It's not just blindly constructing calls.
> 
> But it's *WRONG*.
> 
> It's stupid. It's not an optimization, it makes things worse.
> 
> > This is the same kind of thing that has been heavily discussed before for bcmp() and stpcpy()
> 
> And it makes a bit more sense at least for stpcpy(), because the
> implementation there is basically "strlen+memcpy". Both of which we
> want the compiler to work on - even if we're not interested in it ever
> using stpcpy().
> 
> IOF, for stpcpy, there's at least a *reason* for the compiler to do it.
> 
> For something like wcslen() the answer is "DON'T DO THIS". Because
> there is absolutely zero upside to trying to recognize this pattern,
> and there is real downside.
> 
> See?
> 
> Don't work around the compiler doing stupid things. Fix the compiler
> options to tell the compiler to "Don'tDoThatThen(tm)".

So I do not necessarily disagree with you in the general sense for these
types of optimizations but I figured that in this case, where this
optimization only gets applied twice in a single translation unit
throughout the entire kernel from what I can tell, the overhead was
unlikely to matter much and it felt less problematic to just add the
function. If this is still genuinely unacceptable in your eyes in spite
of that, so be it.

I will admit I did not actually test if '-fno-builtin-wcslen' would not
work with LTO when I wrote the commit message (I merely drew on the
experience for bcmp() several years ago). Now that I have, it appears
to, at least for the simple arm64 allmodconfig case that I tested.
Looking into it, it looks like '-fno-builtin-*' started being honored
properly for LTO with [1] in LLVM 10 and fixed/adjusted for inlining in
[2] in LLVM 11.

So would the following change be acceptable? I can draft up a commit
message and send it along today if so.

diff --git a/Makefile b/Makefile
index 38689a0c3605..a137de124897 100644
--- a/Makefile
+++ b/Makefile
@@ -1057,6 +1057,10 @@ KBUILD_CFLAGS += $(call cc-option, -fstrict-flex-arrays=3)
 KBUILD_CFLAGS-$(CONFIG_CC_NO_STRINGOP_OVERFLOW) += $(call cc-option, -Wno-stringop-overflow)
 KBUILD_CFLAGS-$(CONFIG_CC_STRINGOP_OVERFLOW) += $(call cc-option, -Wstringop-overflow)
 
+# Ensure clang does not transform certain loops into calls to wcslen() after
+# https://github.com/llvm/llvm-project/commit/9694844d7e36fd5e01011ab56b64f27b867aa72d
+KBUILD_CFLAGS-$(call clang-min-version, 210000) += -fno-builtin-wcslen
+
 # disable invalid "can't wrap" optimizations for signed / pointers
 KBUILD_CFLAGS	+= -fno-strict-overflow
 
---

[1]: https://github.com/llvm/llvm-project/commit/878ab6df033d44430939c02075ee00800995dc3b
[2]: https://github.com/llvm/llvm-project/commit/f9ca75f19bab639988ebbe68c81d07babd952afb

Cheers,
Nathan

Re: [GIT PULL] string fixes for v6.15-rc1

[Linus Torvalds]

On Mon, 7 Apr 2025 at 10:37, Nathan Chancellor <nathan@kernel.org> wrote:
>
> So would the following change be acceptable? I can draft up a commit
> message and send it along today if so.

Absoluterly. That's the right thing to do.

> +# Ensure clang does not transform certain loops into calls to wcslen() after
> +# https://github.com/llvm/llvm-project/commit/9694844d7e36fd5e01011ab56b64f27b867aa72d
> +KBUILD_CFLAGS-$(call clang-min-version, 210000) += -fno-builtin-wcslen

I think you could just use

     KBUILD_CFLAGS += $(call cc-option, -fno-builtin-wcslen)

instead, and not use some version check?

          Linus

Re: [GIT PULL] string fixes for v6.15-rc1

[Nathan Chancellor]

On Mon, Apr 07, 2025 at 12:02:04PM -0700, Linus Torvalds wrote:
> On Mon, 7 Apr 2025 at 10:37, Nathan Chancellor <nathan@kernel.org> wrote:
> > +# Ensure clang does not transform certain loops into calls to wcslen() after
> > +# https://github.com/llvm/llvm-project/commit/9694844d7e36fd5e01011ab56b64f27b867aa72d
> > +KBUILD_CFLAGS-$(call clang-min-version, 210000) += -fno-builtin-wcslen
> 
> I think you could just use
> 
>      KBUILD_CFLAGS += $(call cc-option, -fno-builtin-wcslen)
> 
> instead, and not use some version check?

Sure, I just figured this has not been a problem up until this point so
applying it for older versions of clang should not be necessary (since
the optimization that introduces it does not exist) but also not be
harmful. However, I would rather not call out to the compiler since we
know it is supported with all versions of clang (and GCC but it
obviously does not need it) so maybe just

    KBUILD_CFLAGS-$(CONFIG_CC_IS_CLANG) += -fno-builtin-wcslen

or

    ifdef CONFIG_CC_IS_CLANG
    KBUILD_CFLAGS += -fno-builtin-wcslen
    endif

or if you do want this for GCC too, unconditionally adding it should be
fine too.

    KBUILD_CFLAGS += -fno-builtin-wcslen

No strong opinion, let me know your preference and I will make it so.

Cheers,
Nathan

Re: [GIT PULL] string fixes for v6.15-rc1

[Linus Torvalds]

 On Mon, 7 Apr 2025 at 12:25, Nathan Chancellor <nathan@kernel.org> wrote:
>
> or if you do want this for GCC too, unconditionally adding it should be
> fine too.

I think if unconditionally works, that's probably the best option
simply because it's the simplest option.

But I don't see 'wcslen' in the gcc docs, which was why I was assuming
it wanted that "check if it works" thing with "$(call cc-option,...)"

I don't think we need to call out the particular compiler, since the
argument against using it is not compiler-specific per se.

             Linus

Re: [GIT PULL] string fixes for v6.15-rc1

[Nathan Chancellor]

On Mon, Apr 07, 2025 at 01:25:04PM -0700, Linus Torvalds wrote:
> I think if unconditionally works, that's probably the best option
> simply because it's the simplest option.
> 
> But I don't see 'wcslen' in the gcc docs, which was why I was assuming
> it wanted that "check if it works" thing with "$(call cc-option,...)"

It appears that neither gcc nor clang warn for "invalid" libcall values
to '-fno-builtin-*':

$ echo 'int main(void) { return 0; }' | clang -fno-builtin-ireallydonotexist -o /dev/null -S -x c -
$ echo 'int main(void) { return 0; }' | gcc -fno-builtin-ireallydonotexist -o /dev/null -S -x c -

> I don't think we need to call out the particular compiler, since the
> argument against using it is not compiler-specific per se.

Sounds good, I will ultimately make this:

  # Ensure compilers do not transform certain loops into calls to wcslen()
  KBUILD_CFLAGS += -fno-builtin-wcslen

Cheers,
Nathan