2016-03-08 18:17 GMT+01:00 commits@source.squeak.org:
Esteban Lorenzano uploaded a new version of VMMaker to project VM Maker: http://source.squeak.org/VMMaker/VMMaker.oscog-EstebanLorenzano.1714.mcz
==================== Summary ====================
Name: VMMaker.oscog-EstebanLorenzano.1714 Author: EstebanLorenzano Time: 8 March 2016, 6:16:11.06581 pm UUID: e83d2c5f-76cd-47fc-ac29-6c4a25fa5deb Ancestors: VMMaker.oscog-nice.1713
Remove wrong coertion. If coertion is present, this assertion fails:
ref := ByteArray new: 4. ref integerAt: 1 put: -10 size: 4 signed: true. self assert: (ref integerAt: 1 size: 4 signed: true) = -10
=============== Diff against VMMaker.oscog-nice.1713 ===============
Item was changed: ----- Method: ThreadedFFIPlugin>>primitiveFFIIntegerAt (in category 'primitives') ----- primitiveFFIIntegerAt "Answer a (signed or unsigned) n byte integer from the given byte offset in the receiver, using the platform's endianness." | isSigned byteSize byteOffset rcvr addr value mask valueOop | <var: 'value' type: #usqLong> <var: 'mask' type: #usqLong> <export: true> <inline: false> isSigned := interpreterProxy booleanValueOf: (interpreterProxy stackValue: 0). byteSize := interpreterProxy stackIntegerValue: 1. byteOffset := interpreterProxy stackIntegerValue: 2. rcvr := interpreterProxy stackObjectValue: 3. interpreterProxy failed ifTrue:[^0]. (byteOffset > 0 and: [(byteSize between: 1 and: 8) and: [(byteSize bitAnd: byteSize - 1) = 0 "a.k.a. isPowerOfTwo"]]) ifFalse: [^interpreterProxy primitiveFail]. addr := self ffiAddressOf: rcvr startingAt: byteOffset size: byteSize. interpreterProxy failed ifTrue:[^0]. byteSize <= 2 ifTrue: [byteSize = 1 ifTrue: [value := self cCoerceSimple: (interpreterProxy byteAt: addr) to: #'unsigned char'] ifFalse: [value := self cCoerceSimple: (interpreterProxy shortAt: addr) to: #'unsigned short']] ifFalse: [byteSize = 4
ifTrue: [value := interpreterProxylong32At: addr]
ifTrue: [value := self cCoerceSimple:(interpreterProxy long32At: addr) to: #'unsigned int']
This will fix 32bits spur, but what will happen with 64bits? It will happen that you'll break 32bits unsigned access...
0xFFFFFFFF will be interpreted as -1 (long32At:) then promoted to 0xFFFFFFFFFFFFFFFFULL then I don't know what integerObjectOf: will do of it... Probably not the right thing :(
ifFalse: [value := interpreterProxylong64At: addr]]. byteSize < BytesPerWord ifTrue: [isSigned ifTrue: "sign extend value" [mask := 1 << (byteSize * 8 - 1). value := (value bitAnd: mask-1) - (value bitAnd: mask)]. "note: byte/short (&long if BytesPerWord=8) never exceed SmallInteger range" valueOop := interpreterProxy integerObjectOf: value] ifFalse: "general 64 bit integer; note these never fail" [valueOop := isSigned
ifTrue:[interpreterProxy signed64BitIntegerFor: value]
ifFalse:[interpreterProxy positive64BitIntegerFor: value]]. ^interpreterProxy pop: 4 thenPush: valueOop!
2016-03-08 21:09 GMT+01:00 Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com>:
2016-03-08 18:17 GMT+01:00 commits@source.squeak.org:
Esteban Lorenzano uploaded a new version of VMMaker to project VM Maker: http://source.squeak.org/VMMaker/VMMaker.oscog-EstebanLorenzano.1714.mcz
==================== Summary ====================
Name: VMMaker.oscog-EstebanLorenzano.1714 Author: EstebanLorenzano Time: 8 March 2016, 6:16:11.06581 pm UUID: e83d2c5f-76cd-47fc-ac29-6c4a25fa5deb Ancestors: VMMaker.oscog-nice.1713
Remove wrong coertion. If coertion is present, this assertion fails:
ref := ByteArray new: 4. ref integerAt: 1 put: -10 size: 4 signed: true. self assert: (ref integerAt: 1 size: 4 signed: true) = -10
=============== Diff against VMMaker.oscog-nice.1713 ===============
Item was changed: ----- Method: ThreadedFFIPlugin>>primitiveFFIIntegerAt (in category 'primitives') ----- primitiveFFIIntegerAt "Answer a (signed or unsigned) n byte integer from the given byte offset in the receiver, using the platform's endianness." | isSigned byteSize byteOffset rcvr addr value mask valueOop | <var: 'value' type: #usqLong> <var: 'mask' type: #usqLong> <export: true> <inline: false> isSigned := interpreterProxy booleanValueOf: (interpreterProxy stackValue: 0). byteSize := interpreterProxy stackIntegerValue: 1. byteOffset := interpreterProxy stackIntegerValue: 2. rcvr := interpreterProxy stackObjectValue: 3. interpreterProxy failed ifTrue:[^0]. (byteOffset > 0 and: [(byteSize between: 1 and: 8) and: [(byteSize bitAnd: byteSize - 1) = 0 "a.k.a. isPowerOfTwo"]]) ifFalse: [^interpreterProxy primitiveFail]. addr := self ffiAddressOf: rcvr startingAt: byteOffset size: byteSize. interpreterProxy failed ifTrue:[^0]. byteSize <= 2 ifTrue: [byteSize = 1 ifTrue: [value := self cCoerceSimple: (interpreterProxy byteAt: addr) to: #'unsigned char'] ifFalse: [value := self cCoerceSimple: (interpreterProxy shortAt: addr) to: #'unsigned short']] ifFalse: [byteSize = 4
ifTrue: [value := interpreterProxylong32At: addr]
ifTrue: [value := self cCoerceSimple:(interpreterProxy long32At: addr) to: #'unsigned int']
This will fix 32bits spur, but what will happen with 64bits? It will happen that you'll break 32bits unsigned access...
0xFFFFFFFF will be interpreted as -1 (long32At:) then promoted to 0xFFFFFFFFFFFFFFFFULL then I don't know what integerObjectOf: will do of it... Probably not the right thing :(
ifFalse: [value := interpreterProxylong64At: addr]]. byteSize < BytesPerWord ifTrue: [isSigned ifTrue: "sign extend value" [mask := 1 << (byteSize * 8 - 1). value := (value bitAnd: mask-1) - (value bitAnd: mask)]. "note: byte/short (&long if BytesPerWord=8) never exceed SmallInteger range" valueOop := interpreterProxy integerObjectOf: value] ifFalse: "general 64 bit integer; note these never fail" [valueOop := isSigned
ifTrue:[interpreterProxy signed64BitIntegerFor: value]
IMO the right fix would be to force coercion here:
ifTrue:[interpreterProxy signed64BitIntegerFor: (self cCoerceSimple: value to: #sqLong)]
But maybe we can avoid signed64/unsigned64 and use signedMachineInteger and positiveMachineInteger when byteSize = bytePerWord...
ifFalse:[interpreterProxy positive64BitIntegerFor: value]]. ^interpreterProxy pop: 4 thenPush: valueOop!
On 08 Mar 2016, at 21:38, Nicolas Cellier nicolas.cellier.aka.nice@gmail.com wrote:
IMO the right fix would be to force coercion here:
ifTrue:[interpreterProxy signed64BitIntegerFor: (self cCoerceSimple: value to: #sqLong)]
why? forcing a coercion to "unsigned int” while we always use sqTypes didn’t feel correct… also long32At will always answer an sqLong, that’s why I thought simpler approach was to remove a coercion instead adding two…
But maybe we can avoid signed64/unsigned64 and use signedMachineInteger and positiveMachineInteger when byteSize = bytePerWord...
that, I have no idea, maybe :)
Esteban
2016-03-08 22:10 GMT+01:00 Esteban Lorenzano estebanlm@gmail.com:
On 08 Mar 2016, at 21:38, Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com> wrote:
IMO the right fix would be to force coercion here:
ifTrue:[interpreterProxy signed64BitIntegerFor: (self cCoerceSimple: value to: #sqLong)]
why?
I recommend writing simple programs like below, compile and run:
#include <stdio.h> int main() { int x=-1; unsigned long long y; y = (signed) x; printf(" y <- (signed) x :%llx\n", y); y = (unsigned) x; printf(" y <- (unsigned) x :%llx\n", y); }
forcing a coercion to "unsigned int” while we always use sqTypes didn’t feel correct… also long32At will always answer an sqLong, that’s why I thought simpler approach was to remove a coercion instead adding two…
I wouldn't trust it too much. What is long32At doing exactly? I can see two versions of it in platforms/Cross/vm/sqMemoryAccess.h ...
#define long32At intAt # define intAt(oop) intAtPointer(atPointerArg(oop)) # define intAtPointer(ptr) ((sqInt)(*((int *)(ptr))))
In this case, there will be sign promotion and (ref integerAt: 1 size: 4 signed: false) will be broken on 64bits VM...
But with the other #if #else branch:
static inline sqInt intAt(sqInt oop) { return intAtPointer(pointerForOop(oop)); } static inline sqInt intAtPointer(char *ptr) { return (sqInt)(*((unsigned int *)ptr)); }
there won't be any sign promotion, and this time the problem will be with (ref integerAt: 1 size: 4 signed: true)on 64 bits VM
So the 32 high bits of long32 SHOULD be UNTRUSTED and that's why we need to force (int) or (unsigned int) cast in 64 bits VM. It's a mistake to have long32 returning a sqInt, it should return a 32bits int. Currently, int would fit for supported platform, but to be future proof, it should have been int32_t
In my own VM brand I have defined unsignedLong32At to be sure to avoid these nasty horrible things... Most of the time handling unsigned is the right thing to do, they have well defined, least surprising behavior.
But maybe we can avoid signed64/unsigned64 and use signedMachineInteger and positiveMachineInteger when byteSize = bytePerWord...
that, I have no idea, maybe :)
It's just an optimization for avoiding unecessary 64bits ops.
Esteban
On 08 Mar 2016, at 22:50, Nicolas Cellier nicolas.cellier.aka.nice@gmail.com wrote:
2016-03-08 22:10 GMT+01:00 Esteban Lorenzano <estebanlm@gmail.com mailto:estebanlm@gmail.com>:
On 08 Mar 2016, at 21:38, Nicolas Cellier <nicolas.cellier.aka.nice@gmail.com mailto:nicolas.cellier.aka.nice@gmail.com> wrote:
IMO the right fix would be to force coercion here:
ifTrue:[interpreterProxy signed64BitIntegerFor: (self cCoerceSimple: value to: #sqLong)]
mmm… kind of disagree, but ok, you know a lot more than me in this issues :) nevertheless just coercing keeps failing :( what actually worked was to use *machine* methods (btw now I understand them)… I’m committing now.
Esteban
why?
I recommend writing simple programs like below, compile and run:
#include <stdio.h> int main() { int x=-1; unsigned long long y; y = (signed) x; printf(" y <- (signed) x :%llx\n", y); y = (unsigned) x; printf(" y <- (unsigned) x :%llx\n", y); }
forcing a coercion to "unsigned int” while we always use sqTypes didn’t feel correct… also long32At will always answer an sqLong, that’s why I thought simpler approach was to remove a coercion instead adding two…
I wouldn't trust it too much. What is long32At doing exactly? I can see two versions of it in platforms/Cross/vm/sqMemoryAccess.h ...
#define long32At intAt # define intAt(oop) intAtPointer(atPointerArg(oop)) # define intAtPointer(ptr) ((sqInt)(*((int *)(ptr))))
In this case, there will be sign promotion and (ref integerAt: 1 size: 4 signed: false) will be broken on 64bits VM...
But with the other #if #else branch:
static inline sqInt intAt(sqInt oop) { return intAtPointer(pointerForOop(oop)); } static inline sqInt intAtPointer(char *ptr) { return (sqInt)(*((unsigned int *)ptr)); }
there won't be any sign promotion, and this time the problem will be with (ref integerAt: 1 size: 4 signed: true)on 64 bits VM
So the 32 high bits of long32 SHOULD be UNTRUSTED and that's why we need to force (int) or (unsigned int) cast in 64 bits VM. It's a mistake to have long32 returning a sqInt, it should return a 32bits int. Currently, int would fit for supported platform, but to be future proof, it should have been int32_t
In my own VM brand I have defined unsignedLong32At to be sure to avoid these nasty horrible things... Most of the time handling unsigned is the right thing to do, they have well defined, least surprising behavior.
But maybe we can avoid signed64/unsigned64 and use signedMachineInteger and positiveMachineInteger when byteSize = bytePerWord...
that, I have no idea, maybe :)
It's just an optimization for avoiding unecessary 64bits ops.
Esteban
2016-03-09 8:30 GMT+01:00 Esteban Lorenzano estebanlm@gmail.com:
On 08 Mar 2016, at 22:50, Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com> wrote:
2016-03-08 22:10 GMT+01:00 Esteban Lorenzano estebanlm@gmail.com:
On 08 Mar 2016, at 21:38, Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com> wrote:
IMO the right fix would be to force coercion here:
ifTrue:[interpreterProxy signed64BitIntegerFor: (self cCoerceSimple: value to: #sqLong)]
mmm… kind of disagree, but ok, you know a lot more than me in this issues :) nevertheless just coercing keeps failing :( what actually worked was to use *machine* methods (btw now I understand them)… I’m committing now.
Esteban
Doh, the only thing I know is that it's not going to work, until we demonstrate it effectively works! You found the bug, not me, and you see that my proposal doesn't work better than yours. Continue experimenting and I'm pretty sure that the little advantage given by age will soon vanish at this pace :)
Don't forget that the code has to work for extracting 64bits integer on 32bits VM, so a 64bits branch must still exist. The right thing to do is to first write expectations (tests).
Then, IMO, this piece of code is much too clever, and that's why 3 of us (Eliot, you, me) did emit false conjectures... A dumb switch case would be much more robust at the price of a few code duplications...
why?
I recommend writing simple programs like below, compile and run:
#include <stdio.h> int main() { int x=-1; unsigned long long y; y = (signed) x; printf(" y <- (signed) x :%llx\n", y); y = (unsigned) x; printf(" y <- (unsigned) x :%llx\n", y); }
forcing a coercion to "unsigned int” while we always use sqTypes didn’t feel correct… also long32At will always answer an sqLong, that’s why I thought simpler approach was to remove a coercion instead adding two…
I wouldn't trust it too much. What is long32At doing exactly? I can see two versions of it in platforms/Cross/vm/sqMemoryAccess.h ...
#define long32At intAt # define intAt(oop) intAtPointer(atPointerArg(oop)) # define intAtPointer(ptr) ((sqInt)(*((int *)(ptr))))
In this case, there will be sign promotion and (ref integerAt: 1 size: 4 signed: false) will be broken on 64bits VM...
But with the other #if #else branch:
static inline sqInt intAt(sqInt oop) { return intAtPointer(pointerForOop(oop)); } static inline sqInt intAtPointer(char *ptr) { return (sqInt)(*((unsigned int *)ptr)); }
there won't be any sign promotion, and this time the problem will be with (ref integerAt: 1 size: 4 signed: true)on 64 bits VM
So the 32 high bits of long32 SHOULD be UNTRUSTED and that's why we need to force (int) or (unsigned int) cast in 64 bits VM. It's a mistake to have long32 returning a sqInt, it should return a 32bits int. Currently, int would fit for supported platform, but to be future proof, it should have been int32_t
In my own VM brand I have defined unsignedLong32At to be sure to avoid these nasty horrible things... Most of the time handling unsigned is the right thing to do, they have well defined, least surprising behavior.
But maybe we can avoid signed64/unsigned64 and use signedMachineInteger and positiveMachineInteger when byteSize = bytePerWord...
that, I have no idea, maybe :)
It's just an optimization for avoiding unecessary 64bits ops.
Esteban
vm-dev@lists.squeakfoundation.org
-
Esteban Lorenzano -
Nicolas Cellier