Re: [PATCH rdma-next 08/12] overflow.h: Add arithmetic shift helper

[Jason Gunthorpe <jgg@mellanox.com>]

On Wed, Aug 01, 2018 at 11:36:03AM +0200, Peter Zijlstra wrote:
> On Tue, Jun 26, 2018 at 11:54:35AM -0600, Jason Gunthorpe wrote:
> 
> > What about more like this?
> > 
> >           check_shift_overflow(a, s, d) ({
> 
> Should that not be: check_shl_overflow() ? Just 'shift' lacks a
> direction.

Yes, that makes sense.

> > 	      // Shift is always performed on the machine's largest unsigned
> >               u64 _a = a;
> > 	      typeof(s) _s = s;
> >               typeof(d) _d = d;
> > 
> > 	      // Make s safe against UB
> > 	      unsigned int _to_shift = _s >= 0 && _s < 8*sizeof(*d) : _s ? 0;
> 
> Should we not do a gcc-plugin or something that fixes that particular
> UB? Shift acting all retarded like that is just annoying. I feel we
> should eliminate UBs from the language where possible, like
> -fno-strict-overflow mandates 2s complement.

No idea, if someone does this they can remove the above overhead..

> >               *_d = (_a << _to_shift);
> > 
> > 	       // s is malformed
> >               (_to_shift != _s ||
> 
> Not strictly an overflow though, just not expected behaviour.

'overflow' here means the math didn't work, ie
   C = A << B
has a C that does not match A << B done on infinite precision. It is
not limited to checking overflow.

> > 	       // d is a signed type and became negative
> > 	       *_d < 0 ||
> 
> Only a problem if it wasn't negative to start out with.

> > 	       // a is a signed type and was negative
> > 	       _a < 0 ||
>
> Why would that be a problem? You can shift left negative values just
> fine. The only problem is when you run out of sign bits.

These are both a problem because of how the macro is setup, nobody had
an idea how to make this work with different types and allow for
negatives to work properly.

You could define this, but since there is no usecase..

> > 	       // Not invertable means a was truncated during shifting
> > 	       (*_d >> _to_shift) != a))
> >           })
> 
> And I'm not exactly seeing the use case for this macro. What's the point
> of a shift-left if you cannot truncate bits. I suppose it's in the name
> _overflow, but still.

It is basically a specialized case of check_mul_overflow where the
multiply is known to be a power of 2.

Jason