> Date: Wed, 23 Mar 2016 10:58:42 -0400 > From: Michael McConville <[email protected]> > > I'm not sure whether avoiding incrementing here is an ideal move, but > this diff definitely works toward a local optimum. Namely, that error > check is technically meaningless because signed overflow is undefined.
Within the kernel, signed overflow actually is well-defined. We don't run on hypethetical one's complement machines (or machines that use some other weird scheme to encode signed integers) and we pass flags to the compiler that make it do the right thing. Yes, the fact that the C standard doesn't want to make assumptions about the encoding of signed inttegers is the real reason why the standard leaves integer overflow undefined. Unfortunately that has lead the language lwayer types in charge of compilers to implement misguided optimizations that break stuff. > ok? Or would people prefer a solution that's robust to changing > *curpps's type? I you absolutely want to "fix" this issue, I'd prefer that you just fixed the check and left the comment in place. Then at least the code continues to document the corner case. So simply: > - if (*curpps + 1 > *curpps) > + if (*curpps != INT_MAX) Of course that breaks if you start out with a negative number... So I think I'd prefer if you simply left this alone. Cheers, Mark > Index: sys/kern/kern_time.c > =================================================================== > RCS file: /cvs/src/sys/kern/kern_time.c,v > retrieving revision 1.96 > diff -u -p -r1.96 kern_time.c > --- sys/kern/kern_time.c 5 Dec 2015 10:11:53 -0000 1.96 > +++ sys/kern/kern_time.c 10 Feb 2016 05:25:35 -0000 > @@ -765,20 +765,8 @@ ppsratecheck(struct timeval *lasttime, i > else > rv = 0; > > -#if 1 /*DIAGNOSTIC?*/ > - /* be careful about wrap-around */ > - if (*curpps + 1 > *curpps) > - *curpps = *curpps + 1; > -#else > - /* > - * assume that there's not too many calls to this function. > - * not sure if the assumption holds, as it depends on *caller's* > - * behavior, not the behavior of this function. > - * IMHO it is wrong to make assumption on the caller's behavior, > - * so the above #if is #if 1, not #ifdef DIAGNOSTIC. > - */ > - *curpps = *curpps + 1; > -#endif > + if (*curpps != INT_MAX) > + (*curpps)++; > > return (rv); > } > >
