<div dir="ltr"><div><br><div class="gmail_extra"><br><div class="gmail_quote">2017-03-10 9:45 GMT+01:00 Nicolas Cellier <span dir="ltr"><<a href="mailto:nicolas.cellier.aka.nice@gmail.com" target="_blank">nicolas.cellier.aka.nice@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><div><div class="gmail-h5"><br><div class="gmail_quote">2017-03-10 3:47 GMT+01:00 David T. Lewis <span dir="ltr"><<a href="mailto:lewis@mail.msen.com" target="_blank">lewis@mail.msen.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><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>> wrote:<br>
><br>
> ><br>
> > In the VM, the millisecond clock wraps within the 32 bit integer 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, 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 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 hours<br>
> ;-)<br>
><br>
<br>
No. The failure is specific to the 64 bit VM. Source code for the primitive<br>
is the same in either case.<br>
<br></blockquote><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
> I suppose we could fix this, but I'm *much* happier to simply not use<br>
> primitiveSignalAtMilliseconds and stay with the simpler and 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 ways<br>
that affect only someone who is attempting to migrate their existing V3 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 who<br>
will want to migrate V3 images to Spur, and there is no point in making the<br>
process needlessly difficult.<br>
<br>
I do not have the solution, but maybe someone else can help. So I am 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 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 usage.<br>
<br>
Thanks,<br>
<br>
Dave<br>
<br>
</blockquote></div><br><div><br></div></div></div><div>Hi David<br></div><div>I just reviewed the generated code for primitiveSignalAtMilliseconds andhere is what i found:<br><br></div><div>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></div><div>So far, so good.<br><br></div><div>Then<br></div><div>                        deltaMsecs = msecs - ((ioMSecs()) & MillisecondClockMask);<br>                        if (deltaMsecs < 0) {<br>                              <wbr>  deltaMsecs = (deltaMsecs + MillisecondClockMask) + 1;<br>                        }<br>                        GIV(nextWakeupUsecs) = (ioUTCMicroseconds()) + (deltaMsecs * 1000);<br><br>Due to the 29 bits Mask, the bitAnd: (&) operation will result into a positive int.<br>0 <= ((ioMSecs()) & MillisecondClockMask) <= 0x1FFFFFFF<br><br></div><div>However, when we do the Roll-over thing:<br>                              <wbr>  deltaMsecs = (deltaMsecs + MillisecondClockMask) + 1;<br></div><div>Then we create a giant delay (the 6 days mentionned by Eliot) which is HIGHLY QUESTIONNABLE whatever 32 or 64 bits!<br><br></div><div>The difference between 32 and 64 bits is in the next line:<br>                        GIV(nextWakeupUsecs) = (ioUTCMicroseconds()) + (deltaMsecs * 1000);<br></div><div>Because<br>    sqInt deltaMsecs;<br><br></div><div>is 32 bits for a 32bits spur, 64bits for 64bits spur, then (deltaMsecs * 1000) will do two different things:<br></div><div>- in 32 bits it will overflow leading to a negative delay and a nextWakeupUsecs in the past (but that's invoking Undefined Behavior)<br></div><div>- in 64 bits it won't overflow leading to he 6 days delay<br></div><div><br></div><div>Does that explain?<br><br>if (deltaMsecs < 0) I would either fail the primitive or set the deltaMsecs to zero.<br></div><div>What do you think?<span class="gmail-HOEnZb"><font color="#888888"><br><br></font></span></div><span class="gmail-HOEnZb"><font color="#888888"><div> Nicolas<br></div> </font></span></div></div>
</blockquote></div><div class="gmail_extra"><br></div>Anyway, the RollOver protection sounds strange: if clock was high, and if we don't apply the MillisecondClockMask to (clock+delay),<br>then (clock+delay) won't overflow (thanks to largeIntegers). Eventually, for delay > 2^32-2^29 msecs, the primitive will fail thru (positive32BitValueOf).<br></div><div class="gmail_extra">That's something like 43 days delay max, more than enough...</div><br></div>Either the code should rather be something like this with roll-over protection<br>                        deltaMsecs = (msecs - ioMSecs()) & MillisecondClockMask;<div><div class="gmail_extra">or we should leave deltaMsecs as is and reject negative delays<br></div><div class="gmail_extra"><br></div><div class="gmail_extra">Also note this in 32 bits:<br></div><div class="gmail_extra">Since *1000 consumes 10bits, then delays above 21 bits msecs will overflow and be signalled immediately.<br></div><div class="gmail_extra">Tha's about 2e6 msecs, 2e3 secs. Thus the maximum delay supported by this primitive is a bit less than 36 minutes (16r80000000/1000/1000/60).<br></div><div class="gmail_extra">So maybe we should also reject large positive "unreasonable" delays, that isanthing above a few minutes and document it.<br></div><br><div class="gmail_extra"><br></div></div></div>