Re: [PATCH] Makefile: Add '-fno-builtin-bcmp' to CLANG_FLAGS

[Nick Desaulniers <ndesaulniers@google.com>]

On Wed, Mar 13, 2019 at 10:21 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Wed, Mar 13, 2019 at 8:32 AM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > On Wed, Mar 13, 2019 at 02:48:49PM +0100, Arnd Bergmann wrote:
> > > On Wed, Mar 13, 2019 at 2:44 PM Nathan Chancellor
> > > <natechancellor@gmail.com> wrote:
> > > > On Wed, Mar 13, 2019 at 09:13:11AM +0100, Rasmus Villemoes wrote:
> > > > > Wouldn't it be better to just define bcmp as an alias for memcmp? They
> > > > > seem to have compatible prototypes, and then somebody might someday sit
> > > > > down and implement some word-at-a-time version of bcmp making use of the
> > > > > weaker guarantees about the return value to gain some performance. But I
> > > > > suppose that can also be done later.
> > > >
> > > > Thank you much for the review, I didn't even realize this was possible :)
> > > >
> > > > I'd certainly like to explore it as that is what glibc does. How would
> > > > you suggest going about it here?
> > >
> > > I suggested a possible implementation (likely contains bugs) and
> > > an alias for architectures that require strict alignment, see
> > > https://bugs.llvm.org/show_bug.cgi?id=41035#c11
> > >
> > > We could start out with just the alias.
> > >
> > >        Arnd
> >
> > So I've been messing around with this for a bit (forgive me, I'm still
> > learning all of the intricacies around here) and this is what I came up
> > with for when __ARCH_HAVE_MEMCMP is unset (not particularly difficult
> > obviously). I can compile, link, and boot an x86 in QEMU.
> >
> > However, I am not sure how to handle memcmp definitions that are written
> > in assembly like arm64, as the alias attribute only works when the alias
> > is defined in the same translation unit. Thoughts?
>
> I hit this, too:
> ./arch/arm64/include/asm/string.h:40:15: error: alias must point to a
> defined variable
>       or function
>
> since memcmp is only declared (not defined) in that header, clang is
> not happy to alias to memcmp.  If __HAVE_ARCH_MEMCMP is defined, then
> we can just return a call to memcmp.  Thoughts (I need to add comments
> above bcmp, anything else)?  Do we like the typeof trick (stolen from
> glibc) or no?
>
> diff --git a/lib/string.c b/lib/string.c
> index 38e4ca08e757..e6c1954f2716 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -845,7 +845,13 @@ void *memmove(void *dest, const void *src, size_t count)
>  EXPORT_SYMBOL(memmove);
>  #endif
>
> -#ifndef __HAVE_ARCH_MEMCMP
> +#ifdef __HAVE_ARCH_MEMCMP
> +int bcmp(const void *cs, const void *ct, size_t n)
> +{
> +       return memcmp(cs, ct, n);
> +}
> +EXPORT_SYMBOL(bcmp);
> +#else
>  /**
>   * memcmp - Compare two areas of memory
>   * @cs: One area of memory
> @@ -864,6 +870,8 @@ __visible int memcmp(const void *cs, const void
> *ct, size_t count)
>         return res;
>  }
>  EXPORT_SYMBOL(memcmp);
> +__weak __alias(memcmp) typeof(memcmp) bcmp;
> +EXPORT_SYMBOL(bcmp);
>  #endif
>
>  #ifndef __HAVE_ARCH_MEMSCAN

Alternatively, just not worrying about __alias makes this simpler and
seems to work (need to add comments, thoughts?):


diff --git a/lib/string.c b/lib/string.c
index 38e4ca08e757..2112108ecc35 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -866,6 +866,15 @@ __visible int memcmp(const void *cs, const void
*ct, size_t count)
 EXPORT_SYMBOL(memcmp);
 #endif

+#ifndef __HAVE_ARCH_BCMP
+#undef bcmp
+int bcmp(const void *cs, const void *ct, size_t count)
+{
+       return memcmp(cs, ct, count);
+}
+EXPORT_SYMBOL(bcmp);
+#endif
+
 #ifndef __HAVE_ARCH_MEMSCAN
 /**
  * memscan - Find a character in an area of memory.

-- 
Thanks,
~Nick Desaulniers