Re: CVS commit: src/lib/libc/stdlib

[Valery Ushakov <uwe%stderr.spb.ru@localhost>]

On Sun, Feb 23, 2020 at 03:35:19 +0100, Kamil Rytarowski wrote:

> On 23.02.2020 03:20, Valery Ushakov wrote:
> > On Sun, Feb 23, 2020 at 02:51:49 +0100, Kamil Rytarowski wrote:
> > 
> >> On 23.02.2020 02:29, Valery Ushakov wrote:
> >>> On Sat, Feb 22, 2020 at 14:07:57 +0000, Kamil Rytarowski wrote:
> >>>
> >>>> Module Name:	src
> >>>> Committed By:	kamil
> >>>> Date:		Sat Feb 22 14:07:57 UTC 2020
> >>>>
> >>>> Modified Files:
> >>>> 	src/lib/libc/stdlib: _rand48.c
> >>>>
> >>>> Log Message:
> >>>> Avoid undefined behavior in the rand48(3) implementation
> >>>>
> >>>> Instead of implicid promotion to signed int,
> >>>> explicitly cast the arguments to unsigned int.
> >>>
> >>> Please, please, please, pay at least some attention to what is going
> >>> on around the code you are changing.
> >>>
> >>> If there's already code in this function that does:
> >>>
> >>>    accu = (unsigned long) __rand48_mult[0] * (unsigned long) xseed[0];
> >>>
> >>> then keep it consistent and don't do casts to a different type
> >>>
> >>>    accu += (unsigned int) __rand48_mult[0] * (unsigned int) xseed[2];
> >>
> >> cast to unsigned long still works, but changes algorithm. My change was
> >> performed deliberately. On the other hand and according to local tests
> >> the end-result for unsigned long produces the same reults as cast to
> >> unsigned int and unsigned char so it does not matter.
> > 
> > I cannot make sense of your answer.  Does the cast to unsigned long
> > there change the algorithm or does it produce the same result?  If it
> > produces the same result, then it should be used to be consistent with
> > the rest of the code (or the rest of the code changed to use unsigned
> > int).  If it does change the result, there should be a comment
> > explaining it.
> 
> Algorithm would be changed from calculating on 32bit numbers with signed
> integer overflows to an algorithm calculating on 64bit numbers. The
> __dorand48() function truncates the result to least significant 16bits
> only so it does not matter. I retained operations on 32bits avoiding
> changes of types for stylistic reasons.

That still doesn't make sense to me.  You took your time to figure out
whats going on in this bit of code.  Then you make a change that looks
extremely unobvious b/c it is inconsistent with the rest of the code,
and you say you did this for stylistic reasons.

The next person looking at that code (in $bignum years) will have to
waste their time puzzling out the reason.  Why not use the knowledge
you've gained of this code for good and change the code properly?  The
90s unsigned long was probably meant to be 32-bit anyway (cf. X11 mess
up with using long for 32 bit quantities in the protocol and then
running into issues when 64-bit happened).  So if doing things in
32-bit here is the right and intended thing to do, then change that
unsigned long to uint32_t.  If you don't want to dive that deep (which
is entirely understandable), then, exactly for stylistic reasons, cast
to unsigned long to be consistent with the old code - as you already
established that it didn't change the result.

-uwe