[Vm-dev] VM Maker: VMMaker.oscog-eem.1426.mcz
David T. Lewis
lewis at mail.msen.com
Tue Jul 21 13:05:08 UTC 2015
On Mon, Jul 20, 2015 at 11:21:38PM -0700, Eliot Miranda wrote:
>
> 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.
I agree, and I'm sorry for wording it like like that. You definitely should
not revert anything in version control. I was speculating as to the source
of the original symptoms.
I'll try to do the experiment on my own and report back if I find anything
interesting. I am temporarily without a 32 bit build environment due to a
recent computer failure, so I fear I am doing entirely too much guessing
here, sorry about that.
Dave
>
>
> >
> > 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
More information about the Vm-dev
mailing list