[Vm-dev] VM Maker: VMMaker.oscog-eem.1426.mcz

Eliot Miranda eliot.miranda at gmail.com
Tue Jul 21 06:21:38 UTC 2015


On Mon, Jul 20, 2015 at 7:04 PM, David T. Lewis <lewis at mail.msen.com> wrote:

>
> On Mon, Jul 20, 2015 at 04:48:35PM -0400, David T. Lewis wrote:
> > Eliot,
> >
> > Thanks for this explanation. I had not spotted the discrepancy in the
> > sqComplexEvent struct. Yikes! no way that could work.
>
> ... except that sqComplexEvent does not appear to be used anywhere except
> the iOS code base, either in oscog or trunk platform sources. The
> declaration
> of sqComplexEvent was definitely not right, since it would work only for
> the case of sizeof(sqInt) == 4. But I don't see how that would impact a
> VM that does not make any reference to sqComplexEvent.
>
> I tried applying the change to make event fields long rather than int,
> checking for the effect on a format 68002 image (64 bit, 64-bit interpreter
> VM). Everything still works (as expected), but the annoying problem I have
> been seeing (need to wiggle the mouse to keep background unit tests active,
> hence my hunch that the issue is related to event structures) is not
> affected.
>
> The original declaration of sqComplexEvent was wrong. The approach of
> changing the fields from int to long would be a reasonable fix for that
> problem. But I thing it's a red herring, and I still suspect that the
> 64-bit Spur image issue addressed in VMMaker.oscog-eem.1417 was actually
> caused by time stamp conversion, and fixed in your later
> VMMaker.oscog-eem.1426
> update.
>
> I'm sorry I can't test to confirm, but I'll bet that if you revert the
> sqInputEvent fields to be int (original declaration) rather than long,
> the 64-bit Spur VM will still work just fine.
>

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.


>
> Dave
>
>
> >
> > Thanks,
> > Dave
> >
> > >  Hi David,
> > >
> > > On Sun, Jul 19, 2015 at 8:52 PM, David T. Lewis <lewis at mail.msen.com>
> > > wrote:
> > >
> > >>
> > >> On Sun, Jul 19, 2015 at 11:35:46AM -0400, David T. Lewis wrote:
> > >> >
> > >> > On Sat, Jul 18, 2015 at 09:51:30PM -0700, Eliot Miranda wrote:
> > >> > >
> > >> > > On Sat, Jul 18, 2015 at 8:10 PM, David T. Lewis
> > >> <lewis at mail.msen.com>
> > >> wrote:
> > >> > > >
> > >> > > > I may be missing something, but I do not see anything about the
> > >> Spur
> > >> > > > 64-bit image format that should require special handling for
> this.
> > >> > > >
> > >> > >
> > >> > > Yes, I think you're missing something.  let me take you through
> it.
> > >> We're
> > >> > > talking about evaluating, in a 64-bit C, sizeof(long) == 8,
> > >> sizeof(int) ==
> > >> > > 4, the following two variants:
> > >> > >
> > >> > > #define MillisecondClockMask 0x1FFFFFFF
> > >> > >
> > >> > > (sqInt)((MillisecondClockMask << 3) + 1L)
> > >> > >
> > >> > > (sqInt)(((sqInt)MillisecondClockMask << 3) + 1L)
> > >> > >
> > >> > >
> > >> > > So let's take the first.  The type of MillisecondClockMask is int.
> > >> The
> > >> > > type of MillisecondClockMask << 3 is int.  It's bit-pattern is
> > >> 0xFFFFFFF8,
> > >> > > and hence its value is, incorrectly, -8.  So it evaluates to the
> > >> > > SmallInteger -1, not 16r1FFFFFFF as desired.
> > >> > >
> > >> > > In the second, the type of MillisecondClockMask is int. The type
> of
> > >> > > (sqInt) MillisecondClockMask is long.  The type
> > >> > > of ((sqInt)MillisecondClockMask << 3).  It's bit pattern is also
> > >> 0xFFFFFFF8
> > >> > > but its value is 0xFFFFFFF8, /not/ -8, and so it evaluates to the
> > >> > > SMallInteger 16r1FFFFFFF as desired.
> > >> > >
> > >> > > Does that make sense?
> > >> >
> > >> > Yes, the shift left 3 (rather that 1 in the old object format) is
> what
> > >> I
> > >> > was missing.
> > >> >
> > >> > I suspect that a cast to usqInt would accomplish the same thing
> > >> without
> > >> > the ifTrue: test. Sorry I can't try it right now but it might be
> worth
> > >> a
> > >> > look.
> > >> >
> > >>
> > >> At the risk of further annoyance, here is one more
> question/observation:
> > >>
> > >> In VMMaker.oscog-eem.1417 (and sq.h in platforms sources), we change
> the
> > >> type of the event buffer from int[8] to long[8], which corrects a
> > >> failure
> > >> in starting in the 64-bit Spur image. I don't know exactly what was
> > >> failing,
> > >> but I am suspecting perhaps the time stamp field in the event struct
> was
> > >> being incorrectly converted from its 32 bit int value to an integer
> > >> object
> > >> in primitiveGetNextEvent, and that using long rather than int fields
> > >> prevented this from happening.
> > >>
> > >> Later, in VMMaker.oscog-eem.1426 we have the fix for conversion of
> > >> integer
> > >> value to integer object, which particularly affected conversion of
> > >> millisecond clock values. So I wonder if the fix in
> > >> VMMaker.oscog-eem.1426
> > >> might make the changes in VMMaker.oscog-eem.1417 unnecessary? And if
> so,
> > >> can the fields of the sqInputEvent struct be changed back from long to
> > >> int (possibly with some performance advantage)?
> > >>
> > >
> > > Well, indeed the commit comment on  VMMaker.oscog-eem.1417 is wrong, in
> > > that the size of the evtBuf wasn't the cause of the bug.  But there
> /is/ a
> > > bug here with 64-bits.  Look at platforms/Cross/vm/sq.h:
> > >
> > > The generic input event conforms to int evtBuf[8]:
> > >
> > > /* generic input event */
> > > typedef struct sqInputEvent
> > > {
> > >   int type;         /* type of event; either one of EventTypeXXX */
> > >   unsigned int timeStamp;   /* time stamp */
> > >   /* the interpretation of the following fields depend on the type of
> the
> > > event */
> > >   int unused1;
> > >   int unused2;
> > >   int unused3;
> > >   int unused4;
> > >   int unused5;
> > >   int windowIndex;      /* SmallInteger used in image to identify a
> host
> > > window structure */
> > > } sqInputEvent;
> > >
> > > But the complex event *does not*:
> > >
> > > typedef struct sqComplexEvent
> > >     {
> > >         int type;           /* type of event;  EventTypeComplex */
> > >         unsigned int timeStamp; /* time stamp */
> > >         /* the interpretation of the following fields depend on the
> type
> > >  of the event */
> > >         int action;             /* one of ComplexEventXXX (see below)
> */
> > >         usqInt objectPointer;   /* used to point to object */
> > >         int unused1;            /*  */
> > >         int unused2;            /*  */
> > >         int unused3;            /*  */
> > >         int windowIndex;    /* host window structure */
> > >     } sqComplexEvent;
> > >
> > > In 64-bits usqInt objectPointer occupies 64-bits, not 32, and so
> > > a sqComplexEvent does not conform to int evtBuf[8].
> > >
> > >
> > > Hence my solution is to change evtBuf to long evtBuf[8], and change all
> > > the
> > > fields in the events from (unsigned) int to (unsigned) long.  There is
> no
> > > performance issue with 32-bits; sizeof(long) == sizeof(int), and in 64
> > > bits
> > > it won't make any difference either; events are simply not that
> frequent
> > > for this to be an issue.
> > >
> > > HTH
> > >
> > > _,,,^..^,,,_
> > > best, Eliot
> > >
> >
>



-- 
_,,,^..^,,,_
best, Eliot
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20150720/7ed1ec16/attachment-0001.htm


More information about the Vm-dev mailing list