| Message ID | 20210323175424.368223-1-drjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests,v2] compiler: Add builtin overflow flag and predicate wrappers | expand |
On Tue, Mar 23, 2021 at 06:54:24PM +0100, Andrew Jones wrote: > Checking for overflow can be difficult, but doing so may be a good > idea to avoid difficult to debug problems. Compilers that provide > builtins for overflow checking allow the checks to be simple > enough that we can use them more liberally. The idea for this > flag is to wrap a calculation that should have overflow checking, > allowing compilers that support it to give us some extra robustness. > For example, > > #ifdef COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW > bool overflow = __builtin_mul_overflow(x, y, &z); > assert(!overflow); > #else > /* Older compiler, hopefully we don't overflow... */ > z = x * y; > #endif > > This is a bit ugly though, so when possible we can just use the > predicate wrappers, which have an always-false fallback, e.g. > > /* Old compilers won't assert on overflow. Oh, well... */ > assert(!check_mul_overflow(x, y)); > z = x * y; > > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > > v2: Added predicate wrappers > > lib/linux/compiler.h | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/lib/linux/compiler.h b/lib/linux/compiler.h > index 2d72f18c36e5..aa2e3710cf1d 100644 > --- a/lib/linux/compiler.h > +++ b/lib/linux/compiler.h > @@ -8,6 +8,39 @@ > > #ifndef __ASSEMBLY__ > > +#define GCC_VERSION (__GNUC__ * 10000 \ > + + __GNUC_MINOR__ * 100 \ > + + __GNUC_PATCHLEVEL__) > + > +#ifdef __clang__ > +#if __has_builtin(__builtin_add_overflow) && \ > + __has_builtin(__builtin_sub_overflow) && \ > + __has_builtin(__builtin_mul_overflow) > +#define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1 > +#define check_add_overflow(a, b) ({ \ > + typeof((a) + (b)) __d; \ > + __builtin_add_overflow(a, b, &__d); \ > +}) > +#define check_sub_overflow(a, b) ({ \ > + typeof((a) - (b)) __d; \ > + __builtin_sub_overflow(a, b, &__d); \ > +}) > +#define check_mul_overflow(a, b) ({ \ > + typeof((a) * (b)) __d; \ > + __builtin_mul_overflow(a, b, &__d); \ > +}) > +#endif > +#elif GCC_VERSION >= 50100 > +#define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1 > +#define check_add_overflow(a, b) __builtin_add_overflow_p(a, b, (typeof((a) + (b)))0) > +#define check_sub_overflow(a, b) __builtin_add_overflow_p(a, b, (typeof((a) - (b)))0) > +#define check_mul_overflow(a, b) __builtin_add_overflow_p(a, b, (typeof((a) * (b)))0) > +#else > +#define check_add_overflow(a, b) (0) > +#define check_sub_overflow(a, b) (0) > +#define check_mul_overflow(a, b) (0) > +#endif > + > #include <stdint.h> > > #define barrier() asm volatile("" : : : "memory") > -- > 2.26.3 > I just wanted to point out that with this patch the relevant part of strtoul becomes if (is_signed) { long sacc = (long)acc; assert(!check_mul_overflow(sacc, base)); assert(!check_add_overflow(sacc * base, c)); } else { assert(!check_mul_overflow(acc, base)); assert(!check_add_overflow(acc * base, c)); } acc = acc * base + c; which looks pretty good to me (if I do say so myself). Unless somebody shouts I'll queue this patch in arm/queue tomorrow. I'll need to rebase arm/queue to squash in the fixup to strtoul (I hope nobody thinks that the arm/queue branch is stable, because it's not!) I also plan to grab another series from Alexandru, do final testing, and send Paolo an MR for the whole lot tomorrow. Thanks, drew
On Tue, Mar 23, 2021 at 06:54:24PM +0100, Andrew Jones wrote: > Checking for overflow can be difficult, but doing so may be a good > idea to avoid difficult to debug problems. Compilers that provide > builtins for overflow checking allow the checks to be simple > enough that we can use them more liberally. The idea for this > flag is to wrap a calculation that should have overflow checking, > allowing compilers that support it to give us some extra robustness. > For example, > > #ifdef COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW > bool overflow = __builtin_mul_overflow(x, y, &z); > assert(!overflow); > #else > /* Older compiler, hopefully we don't overflow... */ > z = x * y; > #endif > > This is a bit ugly though, so when possible we can just use the > predicate wrappers, which have an always-false fallback, e.g. > > /* Old compilers won't assert on overflow. Oh, well... */ > assert(!check_mul_overflow(x, y)); > z = x * y; > > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > > v2: Added predicate wrappers > > lib/linux/compiler.h | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/lib/linux/compiler.h b/lib/linux/compiler.h > index 2d72f18c36e5..aa2e3710cf1d 100644 > --- a/lib/linux/compiler.h > +++ b/lib/linux/compiler.h > @@ -8,6 +8,39 @@ > > #ifndef __ASSEMBLY__ > > +#define GCC_VERSION (__GNUC__ * 10000 \ > + + __GNUC_MINOR__ * 100 \ > + + __GNUC_PATCHLEVEL__) > + > +#ifdef __clang__ > +#if __has_builtin(__builtin_add_overflow) && \ > + __has_builtin(__builtin_sub_overflow) && \ > + __has_builtin(__builtin_mul_overflow) > +#define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1 > +#define check_add_overflow(a, b) ({ \ > + typeof((a) + (b)) __d; \ > + __builtin_add_overflow(a, b, &__d); \ > +}) > +#define check_sub_overflow(a, b) ({ \ > + typeof((a) - (b)) __d; \ > + __builtin_sub_overflow(a, b, &__d); \ > +}) > +#define check_mul_overflow(a, b) ({ \ > + typeof((a) * (b)) __d; \ > + __builtin_mul_overflow(a, b, &__d); \ > +}) > +#endif > +#elif GCC_VERSION >= 50100 > +#define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1 > +#define check_add_overflow(a, b) __builtin_add_overflow_p(a, b, (typeof((a) + (b)))0) > +#define check_sub_overflow(a, b) __builtin_add_overflow_p(a, b, (typeof((a) - (b)))0) > +#define check_mul_overflow(a, b) __builtin_add_overflow_p(a, b, (typeof((a) * (b)))0) > +#else > +#define check_add_overflow(a, b) (0) > +#define check_sub_overflow(a, b) (0) > +#define check_mul_overflow(a, b) (0) > +#endif > + > #include <stdint.h> > > #define barrier() asm volatile("" : : : "memory") > -- > 2.26.3 > Applied to arm/queue https://gitlab.com/rhdrjones/kvm-unit-tests/-/commits/arm/queue but with another fixup. When the builins are not available and we want to fallback to always false, we also need to ensure any parameters passed to the check_* functions do not result in unused variable warnings. To do that, those macros have been changed to #define check_add_overflow(a, b) ({ (void)((int)(a) == (int)(b)); 0; }) #define check_sub_overflow(a, b) ({ (void)((int)(a) == (int)(b)); 0; }) #define check_mul_overflow(a, b) ({ (void)((int)(a) == (int)(b)); 0; }) Thanks, drew
diff --git a/lib/linux/compiler.h b/lib/linux/compiler.h index 2d72f18c36e5..aa2e3710cf1d 100644 --- a/lib/linux/compiler.h +++ b/lib/linux/compiler.h @@ -8,6 +8,39 @@ #ifndef __ASSEMBLY__ +#define GCC_VERSION (__GNUC__ * 10000 \ + + __GNUC_MINOR__ * 100 \ + + __GNUC_PATCHLEVEL__) + +#ifdef __clang__ +#if __has_builtin(__builtin_add_overflow) && \ + __has_builtin(__builtin_sub_overflow) && \ + __has_builtin(__builtin_mul_overflow) +#define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1 +#define check_add_overflow(a, b) ({ \ + typeof((a) + (b)) __d; \ + __builtin_add_overflow(a, b, &__d); \ +}) +#define check_sub_overflow(a, b) ({ \ + typeof((a) - (b)) __d; \ + __builtin_sub_overflow(a, b, &__d); \ +}) +#define check_mul_overflow(a, b) ({ \ + typeof((a) * (b)) __d; \ + __builtin_mul_overflow(a, b, &__d); \ +}) +#endif +#elif GCC_VERSION >= 50100 +#define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1 +#define check_add_overflow(a, b) __builtin_add_overflow_p(a, b, (typeof((a) + (b)))0) +#define check_sub_overflow(a, b) __builtin_add_overflow_p(a, b, (typeof((a) - (b)))0) +#define check_mul_overflow(a, b) __builtin_add_overflow_p(a, b, (typeof((a) * (b)))0) +#else +#define check_add_overflow(a, b) (0) +#define check_sub_overflow(a, b) (0) +#define check_mul_overflow(a, b) (0) +#endif + #include <stdint.h> #define barrier() asm volatile("" : : : "memory")
Checking for overflow can be difficult, but doing so may be a good idea to avoid difficult to debug problems. Compilers that provide builtins for overflow checking allow the checks to be simple enough that we can use them more liberally. The idea for this flag is to wrap a calculation that should have overflow checking, allowing compilers that support it to give us some extra robustness. For example, #ifdef COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW bool overflow = __builtin_mul_overflow(x, y, &z); assert(!overflow); #else /* Older compiler, hopefully we don't overflow... */ z = x * y; #endif This is a bit ugly though, so when possible we can just use the predicate wrappers, which have an always-false fallback, e.g. /* Old compilers won't assert on overflow. Oh, well... */ assert(!check_mul_overflow(x, y)); z = x * y; Signed-off-by: Andrew Jones <drjones@redhat.com> --- v2: Added predicate wrappers lib/linux/compiler.h | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)