<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jul 20, 2015 at 7:04 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"><br>
On Mon, Jul 20, 2015 at 04:48:35PM -0400, David T. Lewis wrote:<br>
> Eliot,<br>
><br>
> Thanks for this explanation. I had not spotted the discrepancy in the<br>
> sqComplexEvent struct. Yikes! no way that could work.<br>
<br>
... except that sqComplexEvent does not appear to be used anywhere except<br>
the iOS code base, either in oscog or trunk platform sources. The declaration<br>
of sqComplexEvent was definitely not right, since it would work only for<br>
the case of sizeof(sqInt) == 4. But I don't see how that would impact a<br>
VM that does not make any reference to sqComplexEvent.<br>
<br>
I tried applying the change to make event fields long rather than int,<br>
checking for the effect on a format 68002 image (64 bit, 64-bit interpreter<br>
VM). Everything still works (as expected), but the annoying problem I have<br>
been seeing (need to wiggle the mouse to keep background unit tests active,<br>
hence my hunch that the issue is related to event structures) is not affected.<br>
<br>
The original declaration of sqComplexEvent was wrong. The approach of<br>
changing the fields from int to long would be a reasonable fix for that<br>
problem. But I thing it's a red herring, and I still suspect that the<br>
64-bit Spur image issue addressed in VMMaker.oscog-eem.1417 was actually<br>
caused by time stamp conversion, and fixed in your later VMMaker.oscog-eem.1426<br>
update.<br>
<br>
I'm sorry I can't test to confirm, but I'll bet that if you revert the<br>
sqInputEvent fields to be int (original declaration) rather than long,<br>
the 64-bit Spur VM will still work just fine.<br></blockquote><div><br></div><div>That's right. But that doesn't mean evtBuf shouldn't be changed. Since I need an iOS VM, and the change is correct, I'm not about to revert this change.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Dave<br>
<br>
<br>
><br>
> Thanks,<br>
> Dave<br>
><br>
> > Hi David,<br>
> ><br>
> > On Sun, Jul 19, 2015 at 8:52 PM, David T. Lewis <<a href="mailto:lewis@mail.msen.com">lewis@mail.msen.com</a>><br>
> > wrote:<br>
> ><br>
> >><br>
> >> On Sun, Jul 19, 2015 at 11:35:46AM -0400, David T. Lewis wrote:<br>
> >> ><br>
> >> > On Sat, Jul 18, 2015 at 09:51:30PM -0700, Eliot Miranda wrote:<br>
> >> > ><br>
> >> > > On Sat, Jul 18, 2015 at 8:10 PM, David T. Lewis<br>
> >> <<a href="mailto:lewis@mail.msen.com">lewis@mail.msen.com</a>><br>
> >> wrote:<br>
> >> > > ><br>
> >> > > > I may be missing something, but I do not see anything about the<br>
> >> Spur<br>
> >> > > > 64-bit image format that should require special handling for this.<br>
> >> > > ><br>
> >> > ><br>
> >> > > Yes, I think you're missing something. let me take you through it.<br>
> >> We're<br>
> >> > > talking about evaluating, in a 64-bit C, sizeof(long) == 8,<br>
> >> sizeof(int) ==<br>
> >> > > 4, the following two variants:<br>
> >> > ><br>
> >> > > #define MillisecondClockMask 0x1FFFFFFF<br>
> >> > ><br>
> >> > > (sqInt)((MillisecondClockMask << 3) + 1L)<br>
> >> > ><br>
> >> > > (sqInt)(((sqInt)MillisecondClockMask << 3) + 1L)<br>
> >> > ><br>
> >> > ><br>
> >> > > So let's take the first. The type of MillisecondClockMask is int.<br>
> >> The<br>
> >> > > type of MillisecondClockMask << 3 is int. It's bit-pattern is<br>
> >> 0xFFFFFFF8,<br>
> >> > > and hence its value is, incorrectly, -8. So it evaluates to the<br>
> >> > > SmallInteger -1, not 16r1FFFFFFF as desired.<br>
> >> > ><br>
> >> > > In the second, the type of MillisecondClockMask is int. The type of<br>
> >> > > (sqInt) MillisecondClockMask is long. The type<br>
> >> > > of ((sqInt)MillisecondClockMask << 3). It's bit pattern is also<br>
> >> 0xFFFFFFF8<br>
> >> > > but its value is 0xFFFFFFF8, /not/ -8, and so it evaluates to the<br>
> >> > > SMallInteger 16r1FFFFFFF as desired.<br>
> >> > ><br>
> >> > > Does that make sense?<br>
> >> ><br>
> >> > Yes, the shift left 3 (rather that 1 in the old object format) is what<br>
> >> I<br>
> >> > was missing.<br>
> >> ><br>
> >> > I suspect that a cast to usqInt would accomplish the same thing<br>
> >> without<br>
> >> > the ifTrue: test. Sorry I can't try it right now but it might be worth<br>
> >> a<br>
> >> > look.<br>
> >> ><br>
> >><br>
> >> At the risk of further annoyance, here is one more question/observation:<br>
> >><br>
> >> In VMMaker.oscog-eem.1417 (and sq.h in platforms sources), we change the<br>
> >> type of the event buffer from int[8] to long[8], which corrects a<br>
> >> failure<br>
> >> in starting in the 64-bit Spur image. I don't know exactly what was<br>
> >> failing,<br>
> >> but I am suspecting perhaps the time stamp field in the event struct was<br>
> >> being incorrectly converted from its 32 bit int value to an integer<br>
> >> object<br>
> >> in primitiveGetNextEvent, and that using long rather than int fields<br>
> >> prevented this from happening.<br>
> >><br>
> >> Later, in VMMaker.oscog-eem.1426 we have the fix for conversion of<br>
> >> integer<br>
> >> value to integer object, which particularly affected conversion of<br>
> >> millisecond clock values. So I wonder if the fix in<br>
> >> VMMaker.oscog-eem.1426<br>
> >> might make the changes in VMMaker.oscog-eem.1417 unnecessary? And if so,<br>
> >> can the fields of the sqInputEvent struct be changed back from long to<br>
> >> int (possibly with some performance advantage)?<br>
> >><br>
> ><br>
> > Well, indeed the commit comment on VMMaker.oscog-eem.1417 is wrong, in<br>
> > that the size of the evtBuf wasn't the cause of the bug. But there /is/ a<br>
> > bug here with 64-bits. Look at platforms/Cross/vm/sq.h:<br>
> ><br>
> > The generic input event conforms to int evtBuf[8]:<br>
> ><br>
> > /* generic input event */<br>
> > typedef struct sqInputEvent<br>
> > {<br>
> > int type; /* type of event; either one of EventTypeXXX */<br>
> > unsigned int timeStamp; /* time stamp */<br>
> > /* the interpretation of the following fields depend on the type of the<br>
> > event */<br>
> > int unused1;<br>
> > int unused2;<br>
> > int unused3;<br>
> > int unused4;<br>
> > int unused5;<br>
> > int windowIndex; /* SmallInteger used in image to identify a host<br>
> > window structure */<br>
> > } sqInputEvent;<br>
> ><br>
> > But the complex event *does not*:<br>
> ><br>
> > typedef struct sqComplexEvent<br>
> > {<br>
> > int type; /* type of event; EventTypeComplex */<br>
> > unsigned int timeStamp; /* time stamp */<br>
> > /* the interpretation of the following fields depend on the type<br>
> > of the event */<br>
> > int action; /* one of ComplexEventXXX (see below) */<br>
> > usqInt objectPointer; /* used to point to object */<br>
> > int unused1; /* */<br>
> > int unused2; /* */<br>
> > int unused3; /* */<br>
> > int windowIndex; /* host window structure */<br>
> > } sqComplexEvent;<br>
> ><br>
> > In 64-bits usqInt objectPointer occupies 64-bits, not 32, and so<br>
> > a sqComplexEvent does not conform to int evtBuf[8].<br>
> ><br>
> ><br>
> > Hence my solution is to change evtBuf to long evtBuf[8], and change all<br>
> > the<br>
> > fields in the events from (unsigned) int to (unsigned) long. There is no<br>
> > performance issue with 32-bits; sizeof(long) == sizeof(int), and in 64<br>
> > bits<br>
> > it won't make any difference either; events are simply not that frequent<br>
> > for this to be an issue.<br>
> ><br>
> > HTH<br>
> ><br>
> > _,,,^..^,,,_<br>
> > best, Eliot<br>
> ><br>
><br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="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></div>