On Mon, Jul 09, 2018 at 03:15:50PM -0600, Jason Gunthorpe wrote:
> On Mon, Jul 09, 2018 at 10:34:38PM +0300, Leon Romanovsky wrote:
> > On Mon, Jul 09, 2018 at 12:21:41PM -0600, Jason Gunthorpe wrote:
> > > On Mon, Jul 09, 2018 at 06:14:13PM +0000, Bart Van Assche wrote:
> > > > On Sun, 2018-07-08 at 13:38 +0300, Leon Romanovsky wrote:
> > > > > From: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> > > > >
> > > > > Add shift_overflow() helper to help driver authors to ensure that
> > > > > shift operand doesn't cause to overflow.
> > > > >
> > > > > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> > > > > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > > > include/linux/overflow.h | 23 +++++++++++++++++++++++
> > > > > 1 file changed, 23 insertions(+)
> > > > >
> > > > > diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> > > > > index 8712ff70995f..21ff032773e0 100644
> > > > > +++ b/include/linux/overflow.h
> > > > > @@ -202,6 +202,29 @@
> > > > >
> > > > > #endif /* COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW */
> > > > >
> > > > > +/*
> > > > > + * Compute *d = (a << s)
> > > > > + *
> > > > > + * Returns true if '*d' cannot hold the result or 'a << s' doesn't make sense.
> > > > > + * - 'a << s' causes bits to be lost when stored in d
> > > > > + * - 's' is garbage (eg negative) or so large that a << s is guaranteed to be 0
> > > > > + * - 'a' is negative
> > > > > + * - 'a << s' sets the sign bit, if any, in '*d'
> > > > > + * *d is not defined if false is returned.
> > > > > + */
> > > > > +#define check_shift_overflow(a, s, d) ({ \
> > > > > + typeof(a) _a = a; \
> > > > > + typeof(s) _s = s; \
> > > > > + typeof(d) _d = d; \
> > > > > + u64 _a_full = _a; \
> > > > > + unsigned int _to_shift = \
> > > > > + _s >= 0 && _s < 8 * sizeof(*d) ? _s : 0; \
> > > > > + *_d = (_a_full << _to_shift); \
> > > > > + *d = *_d; \
> > > > > + (_to_shift != _s || *_d < 0 || _a < 0 || \
> > > > > + (*_d >> _to_shift) != _a); \
> > > > > +})
> > > >
> > > > The comment "Compute *d = (a << s)" looks misleading to me because what the macro
> > > > computes is *d = ((u64)a << s). Shouldn't sizeof(*d) be changed into sizeof(a) to
> > > > make this macro compute (a << s)?
> > >
> > > Not sure. The definition is to 'compute a << s on infinite precision'
> > > mixing in the type of 'a' seems to just complicate things.
> > >
> > > sizeof(_a_full) might be the clearest option though?
> > >
> > > > The assignment "*d = *_d" looks superfluous to me. Since _d == d, how could that
> > > > assignment be useful?
> > >
> > > Indeed, that doesn't seem useful. Leon? That was not in my original draft?
> >
> > I needed it for lib/test_overflow.c
>
> How? Why?
>
> check_one_op just does this:
>
> t _r; \
> bool _of; \
> \
> _of = check_ ## op ## _overflow(a, b, &_r);
>
> why would it need to have
> t* _d = &_r;
> *&_r = *_d;
>
> as part of the macro expansion?
See my answer to Rasmus.
Thanks
>
> Jason
Attachment:
signature.asc
Description: PGP signature
