Re: [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul

[Andre Przywara <andre.przywara@arm.com>]

On Tue, 23 Mar 2021 14:41:21 +0100
Andrew Jones <drjones@redhat.com> wrote:

Hi,

> On Tue, Mar 23, 2021 at 01:00:01PM +0000, Andre Przywara wrote:
> > On Tue, 23 Mar 2021 13:14:15 +0100
> > Andrew Jones <drjones@redhat.com> wrote:
> > 
> > Hi,
> >   
> > > On Mon, Mar 22, 2021 at 09:35:23AM +0100, Andrew Jones wrote:  
> > > > @@ -208,23 +209,46 @@ unsigned long int strtoul(const char *nptr, char **endptr, int base)
> > > >              c = *s - 'A' + 10;
> > > >          else
> > > >              break;
> > > > -        acc = acc * base + c;
> > > > +
> > > > +        if (is_signed) {
> > > > +            long __acc = (long)acc;
> > > > +            overflow = __builtin_smull_overflow(__acc, base, &__acc);
> > > > +            assert(!overflow);
> > > > +            overflow = __builtin_saddl_overflow(__acc, c, &__acc);
> > > > +            assert(!overflow);
> > > > +            acc = (unsigned long)__acc;
> > > > +        } else {
> > > > +            overflow = __builtin_umull_overflow(acc, base, &acc);
> > > > +            assert(!overflow);
> > > > +            overflow = __builtin_uaddl_overflow(acc, c, &acc);
> > > > +            assert(!overflow);
> > > > +        }
> > > > +    
> > > 
> > > Unfortunately my use of these builtins isn't loved by older compilers,
> > > like the one used by the build-centos7 pipeline in our gitlab CI. I
> > > could wrap them in an #if GCC_VERSION >= 50100 and just have the old
> > > 'acc = acc * base + c' as the fallback, but that's not pretty and
> > > would also mean that clang would use the fallback too. Maybe we can
> > > try and make our compiler.h more fancy in order to provide a
> > > COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW define like linux does for
> > > both gcc and clang. Or, we could just forgot the overflow checking.  
> > 
> > In line with my email from yesterday:
> > Before we go down the path of all evil (premature optimisation!), can't
> > we just copy
> > https://git.kernel.org/pub/scm/libs/klibc/klibc.git/tree/usr/klibc/strntoumax.c
> > and have a tested version that works everywhere? This is BSD/GPL dual
> > licensed, IIUC.
> > I don't really see the reason to performance optimise strtol in the
> > context of kvm-unit-tests.
> >  
> 
> Using the builtin isn't to optimize, it's to simplify. Checking for
> overflow on multiplication is ugly business. As I said yesterday,
> klibc doesn't do any error checking.

Argh, sorry, I missed your reply yesterday in a bunch of other emails!

> We could choose to go that
> way too, but I'd prefer we give a best effort to making the test
> framework robust.

I agree, klibc was just some example, I didn't look too closely into
it. If it lacks, we should indeed not use it.

I just felt we are going through all the special cases of those
functions again, when people elsewhere checked all of them already. I
had some unpleasant experience with implementing a seemingly simple
memcpy() last year, with some surprising corner cases, so grew a bit
wary about re-implementing standard stuff and hoping it's all good.

Cheers,
Andre

> I quick pulled together the diff below. This gives us the overflow
> checking when not using old compilers, but just falls back to the
> simple math otherwise. Unless people have strong opinions about
> that, then I'm inclined to go with it.
> 
> Thanks,
> drew
> 
> 
> diff --git a/lib/linux/compiler.h b/lib/linux/compiler.h
> index 2d72f18c36e5..311da9807932 100644
> --- a/lib/linux/compiler.h
> +++ b/lib/linux/compiler.h
> @@ -8,6 +8,20 @@
>  
>  #ifndef __ASSEMBLY__
>  
> +#define GCC_VERSION (__GNUC__ * 10000           \
> +                    + __GNUC_MINOR__ * 100     \
> +                    + __GNUC_PATCHLEVEL__)
> +
> +#ifdef __clang__
> +#if __has_builtin(__builtin_mul_overflow) && \
> +    __has_builtin(__builtin_add_overflow) && \
> +    __has_builtin(__builtin_sub_overflow)
> +#define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
> +#endif
> +#elif GCC_VERSION >= 50100
> +#define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
> +#endif
> +
>  #include <stdint.h>
>  
>  #define barrier()      asm volatile("" : : : "memory")
> diff --git a/lib/string.c b/lib/string.c
> index b684271bb18f..e323908fe24e 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -7,6 +7,7 @@
>  
>  #include "libcflat.h"
>  #include "stdlib.h"
> +#include "linux/compiler.h"
>  
>  size_t strlen(const char *buf)
>  {
> @@ -171,7 +172,6 @@ static unsigned long __strtol(const char *nptr, char **endptr,
>                                int base, bool is_signed) {
>      unsigned long acc = 0;
>      const char *s = nptr;
> -    bool overflow;
>      int neg, c;
>  
>      assert(base == 0 || (base >= 2 && base <= 36));
> @@ -210,19 +210,23 @@ static unsigned long __strtol(const char *nptr, char **endptr,
>          else
>              break;
>  
> +#ifdef COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW
>          if (is_signed) {
>              long __acc = (long)acc;
> -            overflow = __builtin_smull_overflow(__acc, base, &__acc);
> +            bool overflow = __builtin_smull_overflow(__acc, base, &__acc);
>              assert(!overflow);
>              overflow = __builtin_saddl_overflow(__acc, c, &__acc);
>              assert(!overflow);
>              acc = (unsigned long)__acc;
>          } else {
> -            overflow = __builtin_umull_overflow(acc, base, &acc);
> +            bool overflow = __builtin_umull_overflow(acc, base, &acc);
>              assert(!overflow);
>              overflow = __builtin_uaddl_overflow(acc, c, &acc);
>              assert(!overflow);
>          }
> +#else
> +        acc = acc * base + c;
> +#endif
>  
>          s++;
>      }
>