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

[Andrew Jones <drjones@xxxxxxxxxx>]

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@xxxxxxxxxx> 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. We could choose to go that
way too, but I'd prefer we give a best effort to making the test
framework robust.

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++;
     }