Re: [PATCH 2/2] string: retire bcmp()

[Nathan Chancellor <nathan@kernel.org>]

Hi David,

Thanks for the CC.

On Sat, Nov 23, 2024 at 03:13:09PM +0000, David Laight wrote:
> From: Mateusz Guzik
> > Sent: 23 November 2024 09:47
> > 
> > While architectures could override it thanks to __HAVE_ARCH_BCMP, none
> > of them did. Instead it was implemented as a call to memcmp().
> > 
> > These routines differ in the API contract: memcmp()'s result indicates
> > which way the difference goes (making it usable for sorting), whereas
> > bcmp()'s result merely states whether the buffers differ in any way.
> > 
> > This means that a dedicated optimized bcmp() is cheaper to execute than
> > memcmp() for differing buffers as there is no need to compute the return
> > value.
> > 
> > However, per the above nobody bothered to write one and it is unclear if
> > it makes sense to do it.
> > 
> > Users which really want to compare stuff may want to handle it
> > differently (like e.g., the path lookup).
> > 
> > As there are no users and the code is merely a wrapper around memcmp(),
> > just whack it.
> > 
> ...
> > 
> > -/*
> > - * Clang may lower `memcmp == 0` to `bcmp == 0`.
> > - */
> > -int bcmp(const void *s1, const void *s2, size_t len)
> > -{
> > -	return memcmp(s1, s2, len);
> > -}
> > -
> 
> As per the comment I thought that clang would sometimes generate
> calls to bcmp().
> 
> So while the two symbols could refer to the same code I don't
> think it can be removed.

Right, commit 5f074f3e192f ("lib/string.c: implement a basic bcmp")
explicitly added bcmp() to lib/string.c because LLVM will emit calls to
bcmp instead of memcmp in certain circumstances [1], an optimization
that still exists, thus this patch would trigger new errors at link or
modpost time:

  ERROR: modpost: "bcmp" [arch/x86/kvm/kvm.ko] undefined!
  ERROR: modpost: "bcmp" [arch/x86/kvm/kvm-intel.ko] undefined!
  ERROR: modpost: "bcmp" [fs/quota/quota_v2.ko] undefined!
  ERROR: modpost: "bcmp" [fs/dlm/dlm.ko] undefined!
  ERROR: modpost: "bcmp" [fs/netfs/netfs.ko] undefined!
  ERROR: modpost: "bcmp" [fs/ext4/ext4.ko] undefined!
  ERROR: modpost: "bcmp" [fs/minix/minix.ko] undefined!
  ERROR: modpost: "bcmp" [fs/fat/fat.ko] undefined!
  ERROR: modpost: "bcmp" [fs/isofs/isofs.ko] undefined!
  ERROR: modpost: "bcmp" [fs/nfs/nfs.ko] undefined!
  WARNING: modpost: suppressed 254 unresolved symbol warnings because there were too many)

  ld.lld: error: undefined symbol: bcmp
  >>> referenced by fortify-string.h:715 (include/linux/fortify-string.h:715)
  >>>               vmlinux.o:(load_pdptrs)
  >>> referenced by fortify-string.h:715 (include/linux/fortify-string.h:715)
  >>>               vmlinux.o:(kvm_arch_irqfd_route_changed)
  >>> referenced by fortify-string.h:715 (include/linux/fortify-string.h:715)
  >>>               vmlinux.o:(vmx_check_processor_compat)
  >>> referenced 438 more times
  >>> did you mean: bacmp
  >>> defined in: vmlinux.o

Please do not apply this patch. If we need to shore up the comment to
make this explicit, I am happy to do so.

[1]: https://github.com/llvm/llvm-project/commit/8e16d73346f8091461319a7dfc4ddd18eedcff13

Cheers,
Nathan