svn commit: r193941 - head/sys/netinet

[John Baldwin]

On Thursday 11 June 2009 11:12:58 pm Bruce Evans wrote:
> On Thu, 11 Jun 2009, John Baldwin wrote:
> 
> > On Wednesday 10 June 2009 11:53:16 pm Bruce Evans wrote:
> >> On Wed, 10 Jun 2009, John Baldwin wrote:
> >>
> >>> Log:
> >>>  Change a few members of tcpcb that store cached copies of ticks to be ints
> >>>  instead of unsigned longs.  This fixes a few overflow edge cases on 64-bit
> >>>  platforms.  Specifically, if an idle connection receives a packet shortly
> >>
> >> I think the variables should still be unsigned (ints now).  Otherwise there
> >> is at best benign undefined behaviour when the variables overflow at
> >> INT_MAX, while the code is apparently designed to work with unsigned
> >> values (it casts to int to look at differences between the unsigned values).
> >
> > I wanted to match 'ticks' and figured changing 'ticks' was a far larger
> > headache.
> 
> By changing the signedness you get undefined behaviour for the other types
> too, and risk new and/or different sign extension bugs.

FWIW, the variables were signed before they were changed to unsigned and are now
back as signed again.  It just seems really odd to have the types not match
'ticks' especially since many of the values are just cached copies of 'ticks'.

> > Yes, I noticed the t_badrxtwin breakage but wasn't sure how best to fix it
> > (or at least thought that it should be a separate followup since it was
> > already broken with wraparound).  I considered making it work more like
> > the other variables such as t_rcvtime that merely store a cached value of
> > 'ticks' and then use 'ticks - foo' and compare it with the given interval.
> > This would entail renaming 't_badrxtwin' to 't_badrxttime' or some such.
> > That seems clearer to me than '(int)(ticks - t_badrxtwin) < 0'.
> 
> `ticks < t_limitvar' is more optimal than `ticks - t_startvar < max', but
> harder to get right.  Compilers even try to transform the latter to the
> former (e.g., for loop counters), but they cannot do this if overflow
> or wraparound is a possibility (except overflow gives undefined behaviour
> so compilers can do anything).  After fixing the former to
> `(int)(ticks - t_limitvar) < 0' it has the same number of operations as
> the latter and would only be more efficient if 0 is easier to load and/or
> compare with than `max'.  But casting to (int) seems clear to me -- it is
> idiomatic for recovering signed differences from circular counters.

Hmm, that is an idiom I am not familiar with I guess.

-- 
John Baldwin