<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">2017-03-10 15:14 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"><div class="gmail-HOEnZb"><div class="gmail-h5"><br>
On Fri, Mar 10, 2017 at 09:45:07AM +0100, Nicolas Cellier wrote:<br>
><br>
> 2017-03-10 3:47 GMT+01:00 David T. Lewis <<a href="mailto:lewis@mail.msen.com">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">lewis@mail.msen.com</a>><br>
> > 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<br>
> > 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<br>
> > 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>
> ><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 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<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 deltaMsecs<br>
> to zero.<br>
> What do you think?<br>
<br>
</div></div>Hi Nicolas,<br>
<br>
Thanks very much for looking into this and for the explanation. I am away<br>
right now so I cannot look at it now, but it does seem likely to be a sign<br>
extension issue at least in part.<br>
<br>
If you have a chance to try recompiling with changes to the generated code,<br>
Juan's test condition is just this:<br>
<br>
s := Semaphore new.<br>
<span class="gmail-"> Delay primSignal: s atMilliseconds: Time primMillisecondClock - 10.<br>
</span> s wait.<br>
<br>
This locks the image on 64-bits, and not on 32-bits.<br>
<br>
Thanks a lot!<br>
Dave<br>
<br></blockquote><div><br></div><div>Hi Dave,<br></div><div>No, it's not a sign extension problem. <br><br></div><div>The code is broken.<br>The rollover protection is undue because we inject (clock+delay) and then subtract next_clock,<br></div><div>In case of clock rollover, next_clock falls to zero and we will have a big delay, not a negative one.<br></div><div>The rollover protection also transform a small negative delay into big delay.<br></div></div><br>Things seems to work in 32 bits just because we have an integer overflow when we convert msecs to usecs.<br></div><div class="gmail_extra">big_delay_in_msec * 1000 => overflow => negative_delay_in_usecs<br></div></div>