<div dir="ltr"><div>Sorry to resurrect such an old thread but now that we declare<br> usqLong deltaMsecs;<br><br></div>the compiler barks (and will eliminate dead code):<br><div><br>../../spurstacksrc/vm/gcc3x-interp.c:67892:19: warning: comparison of unsigned expression < 0 is always false [-Wtautological-compare]<br> if (deltaMsecs < 0) {<br><br></div><div>So we might have to come back to this one.<br></div></div><div class="gmail_extra"><br><div class="gmail_quote">2017-03-13 2:47 GMT+01:00 Eliot Miranda <span dir="ltr"><<a href="mailto:eliot.miranda@gmail.com" target="_blank">eliot.miranda@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br><div dir="ltr">Thanks David!</div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Mar 12, 2017 at 3:52 PM, David T. Lewis <span dir="ltr"><<a href="mailto:lewis@mail.msen.com" target="_blank">lewis@mail.msen.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="m_-6181726143125149231HOEnZb"><div class="m_-6181726143125149231h5"><br>
On Fri, Mar 10, 2017 at 11:49:43PM -0500, David T. Lewis wrote:<br>
><br>
> On Fri, Mar 10, 2017 at 10:19:42AM +0100, Nicolas Cellier wrote:<br>
> ><br>
> > 2017-03-10 9:45 GMT+01:00 Nicolas Cellier <<br>
> > <a href="mailto:nicolas.cellier.aka.nice@gmail.com" target="_blank">nicolas.cellier.aka.nice@gmail<wbr>.com</a>>:<br>
> ><br>
> > ><br>
> > ><br>
> > > 2017-03-10 3:47 GMT+01:00 David T. Lewis <<a href="mailto:lewis@mail.msen.com" target="_blank">lewis@mail.msen.com</a>>:<br>
> > ><br>
> > >><br>
> > >> On Thu, Mar 09, 2017 at 12:41:44PM -0800, Eliot Miranda wrote:<br>
> > >> ><br>
> > >> > Hi David,<br>
> > >> ><br>
> > >> > On Mon, Mar 6, 2017 at 5:50 PM, David T. Lewis <<a href="mailto:lewis@mail.msen.com" target="_blank">lewis@mail.msen.com</a>><br>
> > >> wrote:<br>
> > >> ><br>
> > >> > ><br>
> > >> > > In the VM, the millisecond clock wraps within the 32 bit integer<br>
> > >> range:<br>
> > >> > ><br>
> > >> > > #define MillisecondClockMask 0x1FFFFFFF<br>
> > >> > ><br>
> > >> > > In the Cuis image, Delay class>>handleTimerEvent does this:<br>
> > >> > ><br>
> > >> > > nextTick := nextTick min: SmallInteger maxVal.<br>
> > >> > ><br>
> > >> > > On a 64-bit Spur image, SmallInteger maxVal is 16rFFFFFFFFFFFFFFF,<br>
> > >> but on<br>
> > >> > > a 32-bit image it is 16r3FFFFFFF.<br>
> > >> > ><br>
> > >> > > Could that be it?<br>
> > >> > ><br>
> > >> > > I don't really know how to test in Squeak. As you say, Squeak is now<br>
> > >> > > using the microsecond clock in #handleTimerEvent. I do not see<br>
> > >> anything<br>
> > >> > > in primitiveSignalAtMilliseconds that would behave any differently on<br>
> > >> > > a 64 bit versus 32 bit image or VM, but I do not know how to test to<br>
> > >> > > be sure.<br>
> > >> > ><br>
> > >> ><br>
> > >> > I bet that the following code from primitiveSignalAtMilliseconds ends up<br>
> > >> > wrapping when given<br>
> > >> > Delay primSignal: s atMilliseconds: Time primMillisecondClock - 10.<br>
> > >> ><br>
> > >> > deltaMsecs := msecs - (self ioMSecs bitAnd: MillisecondClockMask).<br>
> > >> > deltaMsecs < 0 ifTrue:<br>
> > >> > [deltaMsecs := deltaMsecs + MillisecondClockMask + 1].<br>
> > >> > nextWakeupUsecs := self ioUTCMicroseconds + (deltaMsecs * 1000).<br>
> > >> ><br>
> > >> > and I bet you'll find that the VM will wake up in about 6 days and 5<br>
> > >> hours<br>
> > >> > ;-)<br>
> > >> ><br>
> > >><br>
> > >> No. The failure is specific to the 64 bit VM. Source code for the<br>
> > >> primitive<br>
> > >> is the same in either case.<br>
> > >><br>
> > >><br>
> > > ><br>
> > >> > I suppose we could fix this, but I'm *much* happier to simply not use<br>
> > >> > primitiveSignalAtMilliseconds and stay with the simpler and<br>
> > >> wrapping-immune<br>
> > >> > primitiveSignalAtUTCMicrosecon<wbr>ds<br>
> > >><br>
> > >> Fair enough, but given that there is a demonstrated bug that affects only<br>
> > >> the 64-bit VM, and given that it expresses itself intermittently and in<br>
> > >> ways<br>
> > >> that affect only someone who is attempting to migrate their existing V3<br>
> > >> image<br>
> > >> to Spur, then I would say that it makes very good sense to take the time<br>
> > >> to fix it if we are able to do so. After all, there may be other people<br>
> > >> who<br>
> > >> will want to migrate V3 images to Spur, and there is no point in making<br>
> > >> the<br>
> > >> process needlessly difficult.<br>
> > >><br>
> > >> I do not have the solution, but maybe someone else can help. So I am<br>
> > >> asking<br>
> > >> for help here. Can someone with a working 64-bit build environment please<br>
> > >> check and see if what I said in an earlier email might make a difference:<br>
> > >><br>
> > >> I see that ioMSecs() is declared as signed long (32 bits), but it is<br>
> > >> used<br>
> > >> in expression with a 64 bit usqInt. So maybe it needs a cast, or maybe<br>
> > >> the variables like msecs and deltaMsecs in primitiveSignalAtMilliseconds<br>
> > >> should be declared as 32 bit long and unsigned long to match the actual<br>
> > >> usage.<br>
> > >><br>
> > >> Thanks,<br>
> > >><br>
> > >> Dave<br>
> > >><br>
> > >><br>
> > ><br>
> > > Hi David<br>
> > > I just reviewed the generated code for primitiveSignalAtMilliseconds<br>
> > > andhere is what i found:<br>
> > ><br>
> > > The primitive will simply fail if fed with a negative integer, thanks to:<br>
> > ><br>
> > > msecs = positive32BitValueOf(msecsObj)<wbr>;<br>
> > ><br>
> > > msecs is allways positive, since declared as<br>
> > > usqInt msecs;<br>
> > ><br>
> > > So far, so good.<br>
> > ><br>
> > > Then<br>
> > > deltaMsecs = msecs - ((ioMSecs()) &<br>
> > > MillisecondClockMask);<br>
> > > if (deltaMsecs < 0) {<br>
> > > deltaMsecs = (deltaMsecs +<br>
> > > MillisecondClockMask) + 1;<br>
> > > }<br>
> > > GIV(nextWakeupUsecs) = (ioUTCMicroseconds()) +<br>
> > > (deltaMsecs * 1000);<br>
> > ><br>
> > > Due to the 29 bits Mask, the bitAnd: (&) operation will result into a<br>
> > > positive int.<br>
> > > 0 <= ((ioMSecs()) & MillisecondClockMask) <= 0x1FFFFFFF<br>
> > ><br>
> > > However, when we do the Roll-over thing:<br>
> > > deltaMsecs = (deltaMsecs +<br>
> > > MillisecondClockMask) + 1;<br>
> > > Then we create a giant delay (the 6 days mentionned by Eliot) which is<br>
> > > HIGHLY QUESTIONNABLE whatever 32 or 64 bits!<br>
> > ><br>
> > > The difference between 32 and 64 bits is in the next line:<br>
> > > GIV(nextWakeupUsecs) = (ioUTCMicroseconds()) +<br>
> > > (deltaMsecs * 1000);<br>
> > > Because<br>
> > > sqInt deltaMsecs;<br>
> > ><br>
> > > is 32 bits for a 32bits spur, 64bits for 64bits spur, then (deltaMsecs *<br>
> > > 1000) will do two different things:<br>
> > > - in 32 bits it will overflow leading to a negative delay and a<br>
> > > nextWakeupUsecs in the past (but that's invoking Undefined Behavior)<br>
> > > - in 64 bits it won't overflow leading to he 6 days delay<br>
> > ><br>
> > > Does that explain?<br>
> > ><br>
> > > if (deltaMsecs < 0) I would either fail the primitive or set the<br>
> > > deltaMsecs to zero.<br>
> > > What do you think?<br>
> > ><br>
> > > Nicolas<br>
> > ><br>
> > ><br>
> ><br>
> > Anyway, the RollOver protection sounds strange: if clock was high, and if<br>
> > we don't apply the MillisecondClockMask to (clock+delay),<br>
> > then (clock+delay) won't overflow (thanks to largeIntegers). Eventually,<br>
> > for delay > 2^32-2^29 msecs, the primitive will fail thru<br>
> > (positive32BitValueOf).<br>
> > That's something like 43 days delay max, more than enough...<br>
> ><br>
> > Either the code should rather be something like this with roll-over<br>
> > protection<br>
> > deltaMsecs = (msecs - ioMSecs()) &<br>
> > MillisecondClockMask;<br>
> > or we should leave deltaMsecs as is and reject negative delays<br>
> ><br>
> > Also note this in 32 bits:<br>
> > Since *1000 consumes 10bits, then delays above 21 bits msecs will overflow<br>
> > and be signalled immediately.<br>
> > Tha's about 2e6 msecs, 2e3 secs. Thus the maximum delay supported by this<br>
> > primitive is a bit less than 36 minutes (16r80000000/1000/1000/60).<br>
> > So maybe we should also reject large positive "unreasonable" delays, that<br>
> > isanthing above a few minutes and document it.<br>
><br>
> I think I understand what you are saying now. Let me see if I can restate<br>
> the problem:<br>
><br>
> Before the change to microsecond clock, the interpreter VM did the<br>
> rollover check only in #checkForInterrupts, and the arithmetic did not<br>
> require a 64 bit microsecond range. Now we have rollover logic in<br>
> #primitiveSignalAtMilliseconds<wbr>, which leads to two issues, one of which<br>
> masks the other on 32 bits.<br>
><br>
> Issue #1: If we pass to the primitive a millisecond count that is slightly<br>
> in the past (possibly due to clock skew or other timing issues), then the<br>
> primitive will test for deltaMsecs less than zero, and will incorrectly<br>
> assume that a millisecond clock rollover has happened.<br>
><br>
> Issue #2: When adjusting for clock rollover on 32 bits, deltaMsecs is<br>
> declared as 32 bit sqInt, which results in overflow when converting to<br>
> microseconds. For 64 bit sqInt, there is no overflow. This error masks<br>
> the first problem of Issue #1, which makes the primiitive appear to work<br>
> correctly on 32 bits, even though the clock rollover logic is wrong.<br>
><br>
> I do not think that we can simply fail the primitive, or set deltaMSeconds<br>
> to zero, in the case of deltaMSeconds being less than zero. We still need<br>
> to be able to schedule delays over a time period when the millisecond<br>
> clock rolls over, so that needs to work.<br>
><br>
> We do need to fix the arithmetic overflow condition, which probably just<br>
> requires declaring deltaMsecs as usqLong. This matches the declaration<br>
> for nextWakeupUsecs and should prevent arithmetic overflow. But fixing<br>
> this for issue #2 will unmask issue #1.<br>
><br>
> For issue #1, the solution probably requires a heuristic. If deltaMsecs<br>
> is a big negative value, then the clock has rolled over. If it is a small<br>
> negative value (maybe less than a second, just to pick a number), then<br>
> it is probably clock skew and the MillisecondClockMask + 1 adjustment<br>
> should not be applied.<br>
><br>
> Does this sound right?<br>
><br>
<br>
</div></div>I now have a working Cog/Spur build on my Ubuntu box, so I was able to<br>
try a few things. It turns out that the arithmetic overflow that Nicolas<br>
spotted seems to fully account for the problem that Juan originally<br>
reported on the 64 but Cuis image. The overflow happened here:<br>
<span><br>
nextWakeupUsecs := self ioUTCMicroseconds + (deltaMsecs * 1000).<br>
<br>
</span>Declaring deltaMsecs as a 64 bit unsigned to match the declaration of<br>
nextWakeupUsecs resolved the image lockup problem that Juan identified.<br>
I committed the fix in VMMaker.oscog-dtl.2147.<br>
<br>
Dave<br>
<br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="m_-6181726143125149231gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><span style="font-size:small;border-collapse:separate"><div>_,,,^..^,,,_<br></div><div>best, Eliot</div></span></div></div></div>
</div>
<br></blockquote></div><br></div>