| Message ID | 20210323135801.295407-1-drjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] compiler: Add builtin overflow flag | expand |
On 23/03/2021 14.58, Andrew Jones wrote: > Checking for overflow can 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 > > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > lib/linux/compiler.h | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > 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") > Acked-by: Thomas Huth <thuth@redhat.com> ... but I wonder: 1) Whether we still want to support those old compilers that do not have this built-in functions yet ... maybe it's time to declare the older systems as unsupported now? 2) Whether it would make more sense to provide static-inline functions for these arithmetic operations that take care of the overflow handling, so that we do not have #ifdefs in the .c code later all over the place? Thomas
On Tue, Mar 23, 2021 at 04:22:58PM +0100, Thomas Huth wrote: > On 23/03/2021 14.58, Andrew Jones wrote: > > Checking for overflow can 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 > > > > Signed-off-by: Andrew Jones <drjones@redhat.com> > > --- > > lib/linux/compiler.h | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > 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") > > > > Acked-by: Thomas Huth <thuth@redhat.com> > > ... but I wonder: > > 1) Whether we still want to support those old compilers that do not have > this built-in functions yet ... maybe it's time to declare the older systems > as unsupported now? I think the CentOS7 test is a good one to have. If for nobody else, then the people maintaining and testing RHEL7. So, I'd rather we keep a simple fallback in place, but hope that its use is limited. > > 2) Whether it would make more sense to provide static-inline functions for > these arithmetic operations that take care of the overflow handling, so that > we do not have #ifdefs in the .c code later all over the place? We could add macro wrappers for the arbitrary integral type builtin forms and/or the predicates forms. I can take a stab at that and send a v2. Thanks, drew
On 23/03/2021 16:23, Andrew Jones wrote: > On Tue, Mar 23, 2021 at 04:22:58PM +0100, Thomas Huth wrote: >> On 23/03/2021 14.58, Andrew Jones wrote: >>> Checking for overflow can 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 >>> >>> Signed-off-by: Andrew Jones <drjones@redhat.com> >>> --- >>> lib/linux/compiler.h | 14 ++++++++++++++ >>> 1 file changed, 14 insertions(+) >>> >>> 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") >>> >> >> Acked-by: Thomas Huth <thuth@redhat.com> >> >> ... but I wonder: >> >> 1) Whether we still want to support those old compilers that do not have >> this built-in functions yet ... maybe it's time to declare the older systems >> as unsupported now? > > I think the CentOS7 test is a good one to have. If for nobody else, then > the people maintaining and testing RHEL7. So, I'd rather we keep a simple > fallback in place, but hope that its use is limited. > >> >> 2) Whether it would make more sense to provide static-inline functions for >> these arithmetic operations that take care of the overflow handling, so that >> we do not have #ifdefs in the .c code later all over the place? > > We could add macro wrappers for the arbitrary integral type builtin forms > and/or the predicates forms. > > I can take a stab at that and send a v2. I was about to suggest trivial implementations in compiler.h without support for overflows (always return false), it will made the code on the caller side a lot cleaner and it's not overly complicated; the downside is that the caller will be completely unaware that overflow detection may or may not be supported, adding a #warn would be an option but with -Werror, it's not an option. Thanks, Nikos > > Thanks, > drew > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
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")
Checking for overflow can 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 Signed-off-by: Andrew Jones <drjones@redhat.com> --- lib/linux/compiler.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+)