Re: [PATCH v2 07/11] math64: New DIV_U64_ROUND_CLOSEST helper

[Willy Tarreau <w@1wt.eu>]

On Fri, Jun 25, 2021 at 05:38:03PM +0200, Pali Rohár wrote:
> On Friday 25 June 2021 17:22:31 Geert Uytterhoeven wrote:
> > > +/*
> > > + * DIV_U64_ROUND_CLOSEST - unsigned 64bit divide with 32bit divisor rounded to nearest integer
> > > + * @dividend: unsigned 64bit dividend
> > > + * @divisor: unsigned 32bit divisor
> > > + *
> > > + * Divide unsigned 64bit dividend by unsigned 32bit divisor
> > > + * and round to closest integer.
> > > + *
> > > + * Return: dividend / divisor rounded to nearest integer
> > > + */
> > > +#define DIV_U64_ROUND_CLOSEST(dividend, divisor)       \
> > > +       ({ u32 _tmp = (divisor); div_u64((u64)(dividend) + _tmp / 2, _tmp); })
> > 
> > Given "dividend" should already be an unsigned 64-bit value, I don't
> > think the cast to "u64" is needed. Similar macros in this file also
> > don't have the cast.
> 
> It is just to ensure that plus operation between dividend and _tmp is
> evaluated in 64-bit context to prevent overflow. Just a case when user
> calls this macro with 32-bit dividend param. As it is a macro (and not
> inline function) type is not automatically enforced.

I agree, a large u32 argument added to _tmp/2 could overflow and remain
32 bits, yielding an incorrect result. The cast is mandatory here (and
will either emit no code, or be useful).

The only trap I'm seeing is if a negative signed int is passed in dividend,
it will be sign-extended and will give a large u64 value. A preliminary
u32 cast could avoid this but would break valid u64 arguments, and I'd
claim we never know what the user wants if this happens in the first place.

Willy