On Sat, Jul 18, 2015 at 08:55:27PM +0000, commits@source.squeak.org wrote:
Eliot Miranda uploaded a new version of VMMaker to project VM Maker: http://source.squeak.org/VMMaker/VMMaker.oscog-eem.1426.mcz
==================== Summary ====================
Name: VMMaker.oscog-eem.1426 Author: eem Time: 18 July 2015, 1:54:29.051 pm UUID: 94ab92ba-c5c4-4953-8566-a4cd9c38df1f Ancestors: VMMaker.oscog-eem.1425
Fix at least one set of 64-bit issues caused by Slang. In particular the integerObjectOf: code resulted in (objectMemory integerObjectOf: MillisecondClockMask) evaluating to the -1 object, instead of the 16r1FFFFFFF object, which was the cause of the initially nresponsive 64-bit image on the real VM (no problem in the simulator).
I can't test now to verify, but I wonder if this change is fixing the right problem.
The CCodeGenerator>>generateIntegerObjectOf:on:indent: in VMM trunk has been in use long enough that it has no author initials. I have found it to work correctly on all combinations of 32/64 bit image and VM. If it does not work correctly, I would be inclined to suspect type declaration issues elsewhere.
Original implementation:
CCodeGenerator>>generateIntegerObjectOf: msgNode on: aStream indent: level "Generate the C code for this message onto the given stream."
aStream nextPutAll: '(('. self emitCExpression: msgNode args first on: aStream. aStream nextPutAll: ' << 1) | 1)'.
Dave
=============== Diff against VMMaker.oscog-eem.1425 ===============
Item was changed: ----- Method: CCodeGenerator>>generateIntegerObjectOf:on:indent: (in category 'C translation') ----- generateIntegerObjectOf: msgNode on: aStream indent: level "Generate the C code for this message onto the given stream."
- | expr castToSqint |
- expr := msgNode args first.
- aStream nextPutAll: '(('.
- "Note that the default type of an integer constant in C is int. Hence we /must/
cast constants to long if in the 64-bit world, since e.g. in 64-bits(int)(16r1FFFFF << 3) = (int)16rFFFFFFF8 = -8whereas(long)(16r1FFFFF << 3) = (long) 16rFFFFFFF8 = 4294967288."- castToSqint := expr isConstant and: [vmClass isNil or: [vmClass objectMemoryClass wordSize = 8]].
- castToSqint ifTrue:
[aStream nextPutAll: '(sqInt)'].- self emitCExpression: expr on: aStream.
- aStream
nextPutAll: '(('.- self emitCExpression: msgNode args first on: aStream.
- aStream nextPutAll: ' << '; print: vmClass objectMemoryClass numSmallIntegerTagBits; nextPutAll: ') | 1)'!
Hi David,
On Sat, Jul 18, 2015 at 3:49 PM, David T. Lewis lewis@mail.msen.com wrote:
On Sat, Jul 18, 2015 at 08:55:27PM +0000, commits@source.squeak.org wrote:
Eliot Miranda uploaded a new version of VMMaker to project VM Maker: http://source.squeak.org/VMMaker/VMMaker.oscog-eem.1426.mcz
==================== Summary ====================
Name: VMMaker.oscog-eem.1426 Author: eem Time: 18 July 2015, 1:54:29.051 pm UUID: 94ab92ba-c5c4-4953-8566-a4cd9c38df1f Ancestors: VMMaker.oscog-eem.1425
Fix at least one set of 64-bit issues caused by Slang. In particular
the integerObjectOf: code resulted in (objectMemory integerObjectOf: MillisecondClockMask) evaluating to the -1 object, instead of the 16r1FFFFFFF object, which was the cause of the initially nresponsive 64-bit image on the real VM (no problem in the simulator).
I can't test now to verify, but I wonder if this change is fixing the right problem.
I'm pretty sure it is. It only bites in a 64-bit implementation with > 31-bit integers. In the "standard" 64-bit image SmallIntegers are still only 31-bits so the issue never occurs. The issue is that the default type of an integer constant in C is int. So if one has to shift any constant such that a non-zero bit will occupy bit 31 (0 relative), it must be cast to a long type to avoid sign extension.
Now of course I could generate all Integer constants with the L or UL suffix, e.g.
#define MillisecondClockMask 0x1FFFFFFFL
instead of
#define MillisecondClockMask 0x1FFFFFFF
but that's a much more pervasive change than only generating the cast in integerObjectOf when on 64-bits. So I'm happy with the change that I've made. Experience can of course prove me wrong...
The CCodeGenerator>>generateIntegerObjectOf:on:indent: in VMM trunk has been in use long enough that it has no author initials. I have found it to work correctly on all combinations of 32/64 bit image and VM. If it does not work correctly, I would be inclined to suspect type declaration issues elsewhere.
Original implementation:
CCodeGenerator>>generateIntegerObjectOf: msgNode on: aStream indent: level "Generate the C code for this message onto the given stream."
aStream nextPutAll: '(('. self emitCExpression: msgNode args first on: aStream. aStream nextPutAll: ' << 1) | 1)'.Dave
=============== Diff against VMMaker.oscog-eem.1425 ===============
Item was changed: ----- Method: CCodeGenerator>>generateIntegerObjectOf:on:indent: (in
category 'C translation') -----
generateIntegerObjectOf: msgNode on: aStream indent: level "Generate the C code for this message onto the given stream."
| expr castToSqint |expr := msgNode args first.aStream nextPutAll: '(('."Note that the default type of an integer constant in C is int.Hence we /must/
cast constants to long if in the 64-bit world, since e.g. in64-bits
(int)(16r1FFFFF << 3) = (int)16rFFFFFFF8 = -8whereas(long)(16r1FFFFF << 3) = (long) 16rFFFFFFF8 = 4294967288."castToSqint := expr isConstant and: [vmClass isNil or: [vmClassobjectMemoryClass wordSize = 8]].
castToSqint ifTrue:[aStream nextPutAll: '(sqInt)'].self emitCExpression: expr on: aStream.
aStreamnextPutAll: '(('.self emitCExpression: msgNode args first on: aStream.aStream nextPutAll: ' << '; print: vmClass objectMemoryClass numSmallIntegerTagBits; nextPutAll: ') | 1)'!
On Sat, Jul 18, 2015 at 06:09:22PM -0700, Eliot Miranda wrote:
Hi David,
On Sat, Jul 18, 2015 at 3:49 PM, David T. Lewis lewis@mail.msen.com wrote:
On Sat, Jul 18, 2015 at 08:55:27PM +0000, commits@source.squeak.org wrote:
Eliot Miranda uploaded a new version of VMMaker to project VM Maker: http://source.squeak.org/VMMaker/VMMaker.oscog-eem.1426.mcz
==================== Summary ====================
Name: VMMaker.oscog-eem.1426 Author: eem Time: 18 July 2015, 1:54:29.051 pm UUID: 94ab92ba-c5c4-4953-8566-a4cd9c38df1f Ancestors: VMMaker.oscog-eem.1425
Fix at least one set of 64-bit issues caused by Slang. In particular
the integerObjectOf: code resulted in (objectMemory integerObjectOf: MillisecondClockMask) evaluating to the -1 object, instead of the 16r1FFFFFFF object, which was the cause of the initially nresponsive 64-bit image on the real VM (no problem in the simulator).
I can't test now to verify, but I wonder if this change is fixing the right problem.
I'm pretty sure it is. It only bites in a 64-bit implementation with > 31-bit integers. In the "standard" 64-bit image SmallIntegers are still only 31-bits so the issue never occurs. The issue is that the default type of an integer constant in C is int. So if one has to shift any constant such that a non-zero bit will occupy bit 31 (0 relative), it must be cast to a long type to avoid sign extension.
I don't see it. The size of SmallInteger is enforced by the image, not by the object memory. There is nothing in the "standard" 64-bit image format that requires small integers to be restricted to the same limits imposed by a 32-bit object memory.
The #integerObjectOf: method answers a sqInt, which may be either 32 or 64 bits. This should just work, and I don't see a need for conditional logic other than what might be needed to define the size of sqInt in the first place.
Now of course I could generate all Integer constants with the L or UL suffix, e.g.
#define MillisecondClockMask 0x1FFFFFFFL
instead of
#define MillisecondClockMask 0x1FFFFFFF
but that's a much more pervasive change than only generating the cast in integerObjectOf when on 64-bits. So I'm happy with the change that I've made. Experience can of course prove me wrong...
I don't see the problem. Interpreter class>>initialize does this: MillisecondClockMask := 16r1FFFFFFF.
This works for all combinations or 32 and 64 bit image and host platform.
There should be no need to generate all of the integer constants with L or UL suffix. It is perfectly acceptable to assign 16r1FFFFFFF to a 64 bit signed or unsigned variable, and no special handling should be required.
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.
Dave
On Sat, Jul 18, 2015 at 8:10 PM, David T. Lewis lewis@mail.msen.com wrote:
On Sat, Jul 18, 2015 at 06:09:22PM -0700, Eliot Miranda wrote:
Hi David,
On Sat, Jul 18, 2015 at 3:49 PM, David T. Lewis lewis@mail.msen.com
wrote:
On Sat, Jul 18, 2015 at 08:55:27PM +0000, commits@source.squeak.org
wrote:
Eliot Miranda uploaded a new version of VMMaker to project VM Maker: http://source.squeak.org/VMMaker/VMMaker.oscog-eem.1426.mcz
==================== Summary ====================
Name: VMMaker.oscog-eem.1426 Author: eem Time: 18 July 2015, 1:54:29.051 pm UUID: 94ab92ba-c5c4-4953-8566-a4cd9c38df1f Ancestors: VMMaker.oscog-eem.1425
Fix at least one set of 64-bit issues caused by Slang. In particular
the integerObjectOf: code resulted in (objectMemory integerObjectOf: MillisecondClockMask) evaluating to the -1 object, instead of the 16r1FFFFFFF object, which was the cause of the initially nresponsive
64-bit
image on the real VM (no problem in the simulator).
I can't test now to verify, but I wonder if this change is fixing the right problem.
I'm pretty sure it is. It only bites in a 64-bit implementation with > 31-bit integers. In the "standard" 64-bit image SmallIntegers are still only 31-bits so the issue never occurs. The issue is that the default
type
of an integer constant in C is int. So if one has to shift any constant such that a non-zero bit will occupy bit 31 (0 relative), it must be cast to a long type to avoid sign extension.
I don't see it. The size of SmallInteger is enforced by the image, not by the object memory. There is nothing in the "standard" 64-bit image format that requires small integers to be restricted to the same limits imposed by a 32-bit object memory.
The #integerObjectOf: method answers a sqInt, which may be either 32 or 64 bits. This should just work, and I don't see a need for conditional logic other than what might be needed to define the size of sqInt in the first place.
Now of course I could generate all Integer constants with the L or UL suffix, e.g.
#define MillisecondClockMask 0x1FFFFFFFL
instead of
#define MillisecondClockMask 0x1FFFFFFF
but that's a much more pervasive change than only generating the cast in integerObjectOf when on 64-bits. So I'm happy with the change that I've made. Experience can of course prove me wrong...
I don't see the problem. Interpreter class>>initialize does this: MillisecondClockMask := 16r1FFFFFFF.
This works for all combinations or 32 and 64 bit image and host platform.
There should be no need to generate all of the integer constants with L or UL suffix. It is perfectly acceptable to assign 16r1FFFFFFF to a 64 bit signed or unsigned variable, and no special handling should be required.
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?
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@mail.msen.com wrote:
On Sat, Jul 18, 2015 at 06:09:22PM -0700, Eliot Miranda wrote:
Hi David,
On Sat, Jul 18, 2015 at 3:49 PM, David T. Lewis lewis@mail.msen.com
wrote:
On Sat, Jul 18, 2015 at 08:55:27PM +0000, commits@source.squeak.org
wrote:
Eliot Miranda uploaded a new version of VMMaker to project VM Maker: http://source.squeak.org/VMMaker/VMMaker.oscog-eem.1426.mcz
==================== Summary ====================
Name: VMMaker.oscog-eem.1426 Author: eem Time: 18 July 2015, 1:54:29.051 pm UUID: 94ab92ba-c5c4-4953-8566-a4cd9c38df1f Ancestors: VMMaker.oscog-eem.1425
Fix at least one set of 64-bit issues caused by Slang. In particular
the integerObjectOf: code resulted in (objectMemory integerObjectOf: MillisecondClockMask) evaluating to the -1 object, instead of the 16r1FFFFFFF object, which was the cause of the initially nresponsive
64-bit
image on the real VM (no problem in the simulator).
I can't test now to verify, but I wonder if this change is fixing the right problem.
I'm pretty sure it is. It only bites in a 64-bit implementation with > 31-bit integers. In the "standard" 64-bit image SmallIntegers are still only 31-bits so the issue never occurs. The issue is that the default
type
of an integer constant in C is int. So if one has to shift any constant such that a non-zero bit will occupy bit 31 (0 relative), it must be cast to a long type to avoid sign extension.
I don't see it. The size of SmallInteger is enforced by the image, not by the object memory. There is nothing in the "standard" 64-bit image format that requires small integers to be restricted to the same limits imposed by a 32-bit object memory.
The #integerObjectOf: method answers a sqInt, which may be either 32 or 64 bits. This should just work, and I don't see a need for conditional logic other than what might be needed to define the size of sqInt in the first place.
Now of course I could generate all Integer constants with the L or UL suffix, e.g.
#define MillisecondClockMask 0x1FFFFFFFL
instead of
#define MillisecondClockMask 0x1FFFFFFF
but that's a much more pervasive change than only generating the cast in integerObjectOf when on 64-bits. So I'm happy with the change that I've made. Experience can of course prove me wrong...
I don't see the problem. Interpreter class>>initialize does this: MillisecondClockMask := 16r1FFFFFFF.
This works for all combinations or 32 and 64 bit image and host platform.
There should be no need to generate all of the integer constants with L or UL suffix. It is perfectly acceptable to assign 16r1FFFFFFF to a 64 bit signed or unsigned variable, and no special handling should be required.
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.
Dave
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@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)?
Dave
Hi David,
On Sun, Jul 19, 2015 at 8:52 PM, David T. Lewis lewis@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@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
Eliot,
Thanks for this explanation. I had not spotted the discrepancy in the sqComplexEvent struct. Yikes! no way that could work.
Thanks, Dave
Hi David,
On Sun, Jul 19, 2015 at 8:52 PM, David T. Lewis lewis@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@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
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.
Dave
Thanks, Dave
Hi David,
On Sun, Jul 19, 2015 at 8:52 PM, David T. Lewis lewis@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@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
On Mon, Jul 20, 2015 at 7:04 PM, David T. Lewis lewis@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@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@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
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@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@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@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
Hi David,
one more time with fewer typos.
On Sat, Jul 18, 2015 at 8:10 PM, David T. Lewis lewis@mail.msen.com wrote:
On Sat, Jul 18, 2015 at 06:09:22PM -0700, Eliot Miranda wrote:
Hi David,
On Sat, Jul 18, 2015 at 3:49 PM, David T. Lewis lewis@mail.msen.com
wrote:
On Sat, Jul 18, 2015 at 08:55:27PM +0000, commits@source.squeak.org
wrote:
Eliot Miranda uploaded a new version of VMMaker to project VM Maker: http://source.squeak.org/VMMaker/VMMaker.oscog-eem.1426.mcz
==================== Summary ====================
Name: VMMaker.oscog-eem.1426 Author: eem Time: 18 July 2015, 1:54:29.051 pm UUID: 94ab92ba-c5c4-4953-8566-a4cd9c38df1f Ancestors: VMMaker.oscog-eem.1425
Fix at least one set of 64-bit issues caused by Slang. In particular
the integerObjectOf: code resulted in (objectMemory integerObjectOf: MillisecondClockMask) evaluating to the -1 object, instead of the 16r1FFFFFFF object, which was the cause of the initially nresponsive
64-bit
image on the real VM (no problem in the simulator).
I can't test now to verify, but I wonder if this change is fixing the right problem.
I'm pretty sure it is. It only bites in a 64-bit implementation with > 31-bit integers. In the "standard" 64-bit image SmallIntegers are still only 31-bits so the issue never occurs. The issue is that the default
type
of an integer constant in C is int. So if one has to shift any constant such that a non-zero bit will occupy bit 31 (0 relative), it must be cast to a long type to avoid sign extension.
I don't see it. The size of SmallInteger is enforced by the image, not by the object memory. There is nothing in the "standard" 64-bit image format that requires small integers to be restricted to the same limits imposed by a 32-bit object memory.
But they are, right? And earlier in the Spur 64-bit work you were encouraging me to restrict their range to that of the 32-bit system to avoid bugs (of which the one below is one).
The #integerObjectOf: method answers a sqInt, which may be either 32 or 64 bits. This should just work, and I don't see a need for conditional logic other than what might be needed to define the size of sqInt in the first place.
Unfortunately it doesn't. See below.
Now of course I could generate all Integer constants with the L or UL suffix, e.g.
#define MillisecondClockMask 0x1FFFFFFFL
instead of
#define MillisecondClockMask 0x1FFFFFFF
but that's a much more pervasive change than only generating the cast in integerObjectOf when on 64-bits. So I'm happy with the change that I've made. Experience can of course prove me wrong...
I don't see the problem. Interpreter class>>initialize does this: MillisecondClockMask := 16r1FFFFFFF.
This works for all combinations or 32 and 64 bit image and host platform.
There should be no need to generate all of the integer constants with L or UL suffix. It is perfectly acceptable to assign 16r1FFFFFFF to a 64 bit signed or unsigned variable, and no special handling should be required.
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) is also long, and hence an 8 byte value. 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 it make sense now?
_,,,^..^,,,_ best, Eliot
vm-dev@lists.squeakfoundation.org
-
David T. Lewis -
Eliot Miranda