Hi -
I just noticed hat Interpreter>>signed32BitValueOf: and signed64BitValueOf: are broken for edge cases. The following example will illustrate the problem:
array := IntegerArray new: 1. array at: 1 put: 16rFFFFFFFF. "should fail but doesn't" array at: 1. "answers -1 incorrectly"
array := IntegerArray new: 1. array at: 1 put: -16rFFFFFFFF. "should fail but doesn't" array at: 1. "answers 1 incorrectly"
The problem is that both signed32BitValueOf: as well as signed64BitValueOf: do not test whether the high bit of the magnitude is set (which it mustn't to fit into a signed integer). The fix is trivial in both cases - basically all that's needed at the end of both functions is this:
"Filter out values out of range for the signed interpretation such as 16rFFFFFFFF (positive w/ bit 32 set) and -16rFFFFFFFF (negative w/ bit 32 set). Since the sign is implicit in the class we require that the high bit of the magnitude is not set which is a simple test here" value < 0 ifTrue:[^self primitiveFail]. negative ifTrue:[^0 - value] ifFalse:[^value]
Cheers, - Andreas
On 20/03/2008, Andreas Raab andreas.raab@gmx.de wrote:
Hi -
I just noticed hat Interpreter>>signed32BitValueOf: and signed64BitValueOf: are broken for edge cases. The following example will illustrate the problem:
array := IntegerArray new: 1. array at: 1 put: 16rFFFFFFFF. "should fail but doesn't" array at: 1. "answers -1 incorrectly"
array := IntegerArray new: 1. array at: 1 put: -16rFFFFFFFF. "should fail but doesn't" array at: 1. "answers 1 incorrectly"
The problem is that both signed32BitValueOf: as well as signed64BitValueOf: do not test whether the high bit of the magnitude is set (which it mustn't to fit into a signed integer). The fix is trivial in both cases - basically all that's needed at the end of both functions is this:
"Filter out values out of range for the signed interpretation such as 16rFFFFFFFF (positive w/ bit 32 set) and -16rFFFFFFFF (negative w/ bit 32 set). Since the sign is implicit in the class we require that the high bit of the magnitude is not set which is a simple test here" value < 0 ifTrue:[^self primitiveFail]. negative ifTrue:[^0 - value] ifFalse:[^value]
This C weak types always a place of uncertainty for me. What i fear of, that if you change implementation of #signed32BitValueOf: it can break more things than it was before. Due to same reasons, of course.
Cheers,
- Andreas
I've smacked into this in doing the gstreamer stuff last weekend. Thankfully an sunit test case promptly failed for 64bit work I didn't see an issue with 32bit signed, but likely didn't test.
Attached below is a note I sent to Tim and my workaround, but he's doing some personal business in the UK and I doubt he will address anytime soon. I put together a workaround, but if you can supply the entire smalltalk method that would be helpful.
On Mar 19, 2008, at 8:44 PM, Andreas Raab wrote:
Hi -
I just noticed hat Interpreter>>signed32BitValueOf: and signed64BitValueOf: are broken for edge cases. The following example will illustrate the problem:
array := IntegerArray new: 1. array at: 1 put: 16rFFFFFFFF. "should fail but doesn't" array at: 1. "answers -1 incorrectly"
array := IntegerArray new: 1. array at: 1 put: -16rFFFFFFFF. "should fail but doesn't" array at: 1. "answers 1 incorrectly"
The problem is that both signed32BitValueOf: as well as signed64BitValueOf: do not test whether the high bit of the magnitude is set (which it mustn't to fit into a signed integer). The fix is trivial in both cases - basically all that's needed at the end of both functions is this:
"Filter out values out of range for the signed interpretation such as 16rFFFFFFFF (positive w/ bit 32 set) and -16rFFFFFFFF (negative w/ bit 32 set). Since the sign is implicit in the class we require that the high bit of the magnitude is not set which is a simple test here" value < 0 ifTrue:[^self primitiveFail]. negative ifTrue:[^0 - value] ifFalse:[^value]
Cheers,
- Andreas
Did we fix this? I'm sure I've smacked into this in doing the gstreamer stuff
I have this below if you give it 0x00000000 FFFFFFFF
the check = value >> 32; if (check == 0) { return signed32BitIntegerFor(integerValue); }
returns zero because the 0xFFFFFFFF >> 32 is zero and then we're off to signed32BitIntegerFor however that is wrong.
/* Return a Large Integer object for the given integer value */
sqInt signed64BitIntegerFor(sqLong integerValue) { register struct foo * foo = &fum; sqLong value; sqInt newLargeInteger; sqInt largeClass; sqInt check; sqInt intValue; sqInt i;
if (integerValue < 0) { largeClass = longAt((foo->specialObjectsOop + BaseHeaderSize) + (ClassLargeNegativeInteger << ShiftForWord)); value = 0 - integerValue; } else { largeClass = longAt((foo->specialObjectsOop + BaseHeaderSize) + (ClassLargePositiveInteger << ShiftForWord)); value = integerValue; } if ((sizeof(value)) == 4) { return signed32BitIntegerFor(integerValue); } check = value >> 32; if (check == 0) { return signed32BitIntegerFor(integerValue); } newLargeInteger = instantiateSmallClasssizeInBytes(largeClass, BaseHeaderSize + 8); for (i = 0; i <= 7; i += 1) { intValue = ( value >> (i * 8)) & 255; byteAtput((newLargeInteger + BaseHeaderSize) + i, intValue); } return newLargeInteger; }
Begin forwarded message:
From: tim Rowledge tim@rowledge.org Date: June 7, 2006 10:02:40 PM PDT (CA) To: squeak vm vm-dev@discuss.squeakfoundation.org Subject: Re: InterpreterProxy>>signed64BitIntegerFor: badly broken
On 7-Jun-06, at 6:58 PM, Andreas Raab wrote:
Hi Guys -
I don't know if you ever used the above method but it's horribly, horribly broken. I wrote a little test primitive (see below) that simply used signed64BitIntegerFor(signed64BitValueOf(oop)) and then a loop like here:
0 to: 63 do:[:i| n := 1 bitShift: i. (self test64BitInt: n) = n ifFalse:[self halt: i]. ].
Starting from i = 31 Every. Last. Result. Is Wrong. Can you imagine?
It gets even better, since it's broken in different ways: For i=31 the result is negated, for everything beyound 31 the resulting large integer is non-normalized (and therefore not comparing correctly).
Any ideas?
Well for starters the signed64BitIntegerFor: code assumes an 8 byte large integer no matter what the value being converted so that's going to cause your non-normalized problem. I'm fairly sure you can work out how to fix that bit quickly enough.
I'm not absolutely sure(and I can't be bothered to look it up right now) but wouldn't 1<<31 be a negative value when treated as a 32 bit word? It looks to me as if signed32BitInteger might be the wrong thing to use in signed64itInteger, with positive32BitInteger a bit more plausible.
I have vague memories of when this code was written, mostly of it being related to long file pointers in OSs I wasn't running at that time. Thus I would have relied upon testing by involved parties and taken their word as to the viability of the code. I guess that once again the value of good tests is demonstrated.
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange OpCodes: VMB: Verify, then Make Bad
Workaround I had
signed64BitIntegerForOVERRIDE: integerValue "Answer a new String copied from a null-terminated C string. Caution: This may invoke the garbage collector." | checkUnsignedLong checkUnsignedLongMax |
self var: 'integerValue' type: 'const sqLong'. self var: 'integerValue' type: 'sqLong'. self var: 'checkLong' type: 'sqLong'. self var: 'checkUnsignedLong' type: 'unsigned long long'. self var: 'checkUnsignedLongMax' type: 'unsigned long long'. self var: 'value' type: 'sqLong'. checkUnsignedLong := integerValue. checkUnsignedLongMax := 16rFFFFFFFF. checkUnsignedLong > checkUnsignedLongMax ifFalse: [^interpreterProxy positive32BitIntegerFor: checkUnsignedLong].
^interpreterProxy signed64BitIntegerFor: integerValue.
Try the attached versions. They should be good - I tested with quite a range of inputs and outputs.
Cheers, - Andreas
John M McIntosh wrote:
I've smacked into this in doing the gstreamer stuff last weekend. Thankfully an sunit test case promptly failed for 64bit work I didn't see an issue with 32bit signed, but likely didn't test.
Attached below is a note I sent to Tim and my workaround, but he's doing some personal business in the UK and I doubt he will address anytime soon. I put together a workaround, but if you can supply the entire smalltalk method that would be helpful.
On Mar 19, 2008, at 8:44 PM, Andreas Raab wrote:
Hi -
I just noticed hat Interpreter>>signed32BitValueOf: and signed64BitValueOf: are broken for edge cases. The following example will illustrate the problem:
array := IntegerArray new: 1. array at: 1 put: 16rFFFFFFFF. "should fail but doesn't" array at: 1. "answers -1 incorrectly"
array := IntegerArray new: 1. array at: 1 put: -16rFFFFFFFF. "should fail but doesn't" array at: 1. "answers 1 incorrectly"
The problem is that both signed32BitValueOf: as well as signed64BitValueOf: do not test whether the high bit of the magnitude is set (which it mustn't to fit into a signed integer). The fix is trivial in both cases - basically all that's needed at the end of both functions is this:
"Filter out values out of range for the signed interpretation such as 16rFFFFFFFF (positive w/ bit 32 set) and -16rFFFFFFFF (negative w/ bit 32 set). Since the sign is implicit in the class we require that the high bit of the magnitude is not set which is a simple test here" value < 0 ifTrue:[^self primitiveFail]. negative ifTrue:[^0 - value] ifFalse:[^value]
Cheers,
- Andreas
Did we fix this? I'm sure I've smacked into this in doing the gstreamer stuff
I have this below if you give it 0x00000000 FFFFFFFF
the check = value >> 32; if (check == 0) { return signed32BitIntegerFor(integerValue); }
returns zero because the 0xFFFFFFFF >> 32 is zero and then we're off to signed32BitIntegerFor however that is wrong.
/* Return a Large Integer object for the given integer value */
sqInt signed64BitIntegerFor(sqLong integerValue) { register struct foo * foo = &fum; sqLong value; sqInt newLargeInteger; sqInt largeClass; sqInt check; sqInt intValue; sqInt i;
if (integerValue < 0) { largeClass = longAt((foo->specialObjectsOop + BaseHeaderSize)
- (ClassLargeNegativeInteger << ShiftForWord)); value = 0 - integerValue; } else { largeClass = longAt((foo->specialObjectsOop + BaseHeaderSize)
- (ClassLargePositiveInteger << ShiftForWord)); value = integerValue; } if ((sizeof(value)) == 4) { return signed32BitIntegerFor(integerValue); } check = value >> 32; if (check == 0) { return signed32BitIntegerFor(integerValue); } newLargeInteger = instantiateSmallClasssizeInBytes(largeClass,
BaseHeaderSize + 8); for (i = 0; i <= 7; i += 1) { intValue = ( value >> (i * 8)) & 255; byteAtput((newLargeInteger + BaseHeaderSize) + i, intValue); } return newLargeInteger; }
Begin forwarded message:
From: tim Rowledge tim@rowledge.org Date: June 7, 2006 10:02:40 PM PDT (CA) To: squeak vm vm-dev@discuss.squeakfoundation.org Subject: Re: InterpreterProxy>>signed64BitIntegerFor: badly broken
On 7-Jun-06, at 6:58 PM, Andreas Raab wrote:
Hi Guys -
I don't know if you ever used the above method but it's horribly, horribly broken. I wrote a little test primitive (see below) that simply used signed64BitIntegerFor(signed64BitValueOf(oop)) and then a loop like here:
0 to: 63 do:[:i| n := 1 bitShift: i. (self test64BitInt: n) = n ifFalse:[self halt: i]. ].Starting from i = 31 Every. Last. Result. Is Wrong. Can you imagine?
It gets even better, since it's broken in different ways: For i=31 the result is negated, for everything beyound 31 the resulting large integer is non-normalized (and therefore not comparing correctly).
Any ideas?
Well for starters the signed64BitIntegerFor: code assumes an 8 byte large integer no matter what the value being converted so that's going to cause your non-normalized problem. I'm fairly sure you can work out how to fix that bit quickly enough.
I'm not absolutely sure(and I can't be bothered to look it up right now) but wouldn't 1<<31 be a negative value when treated as a 32 bit word? It looks to me as if signed32BitInteger might be the wrong thing to use in signed64itInteger, with positive32BitInteger a bit more plausible.
I have vague memories of when this code was written, mostly of it being related to long file pointers in OSs I wasn't running at that time. Thus I would have relied upon testing by involved parties and taken their word as to the viability of the code. I guess that once again the value of good tests is demonstrated.
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange OpCodes: VMB: Verify, then Make Bad
Workaround I had
signed64BitIntegerForOVERRIDE: integerValue "Answer a new String copied from a null-terminated C string. Caution: This may invoke the garbage collector." | checkUnsignedLong checkUnsignedLongMax |
self var: 'integerValue' type: 'const sqLong'. self var: 'integerValue' type: 'sqLong'. self var: 'checkLong' type: 'sqLong'. self var: 'checkUnsignedLong' type: 'unsigned long long'. self var: 'checkUnsignedLongMax' type: 'unsigned long long'. self var: 'value' type: 'sqLong'. checkUnsignedLong := integerValue. checkUnsignedLongMax := 16rFFFFFFFF. checkUnsignedLong > checkUnsignedLongMax ifFalse: [^interpreterProxy positive32BitIntegerFor:checkUnsignedLong].
^interpreterProxy signed64BitIntegerFor: integerValue.
And of course, positive64BitIntegerFor: was broken, too (creating non-normalized large integers). Sigh. Here is a full set of the fixed versions.
Cheers, - Andreas
Andreas Raab wrote:
Try the attached versions. They should be good - I tested with quite a range of inputs and outputs.
Cheers,
- Andreas
John M McIntosh wrote:
I've smacked into this in doing the gstreamer stuff last weekend. Thankfully an sunit test case promptly failed for 64bit work I didn't see an issue with 32bit signed, but likely didn't test.
Attached below is a note I sent to Tim and my workaround, but he's doing some personal business in the UK and I doubt he will address anytime soon. I put together a workaround, but if you can supply the entire smalltalk method that would be helpful.
On Mar 19, 2008, at 8:44 PM, Andreas Raab wrote:
Hi -
I just noticed hat Interpreter>>signed32BitValueOf: and signed64BitValueOf: are broken for edge cases. The following example will illustrate the problem:
array := IntegerArray new: 1. array at: 1 put: 16rFFFFFFFF. "should fail but doesn't" array at: 1. "answers -1 incorrectly"
array := IntegerArray new: 1. array at: 1 put: -16rFFFFFFFF. "should fail but doesn't" array at: 1. "answers 1 incorrectly"
The problem is that both signed32BitValueOf: as well as signed64BitValueOf: do not test whether the high bit of the magnitude is set (which it mustn't to fit into a signed integer). The fix is trivial in both cases - basically all that's needed at the end of both functions is this:
"Filter out values out of range for the signed interpretation such as 16rFFFFFFFF (positive w/ bit 32 set) and -16rFFFFFFFF (negative w/ bit 32 set). Since the sign is implicit in the class we require that the high bit of the magnitude is not set which is a simple test here" value < 0 ifTrue:[^self primitiveFail]. negative ifTrue:[^0 - value] ifFalse:[^value]
Cheers,
- Andreas
Did we fix this? I'm sure I've smacked into this in doing the gstreamer stuff
I have this below if you give it 0x00000000 FFFFFFFF
the check = value >> 32; if (check == 0) { return signed32BitIntegerFor(integerValue); }
returns zero because the 0xFFFFFFFF >> 32 is zero and then we're off to signed32BitIntegerFor however that is wrong.
/* Return a Large Integer object for the given integer value */
sqInt signed64BitIntegerFor(sqLong integerValue) { register struct foo * foo = &fum; sqLong value; sqInt newLargeInteger; sqInt largeClass; sqInt check; sqInt intValue; sqInt i;
if (integerValue < 0) { largeClass = longAt((foo->specialObjectsOop + BaseHeaderSize)
- (ClassLargeNegativeInteger << ShiftForWord)); value = 0 - integerValue; } else { largeClass = longAt((foo->specialObjectsOop + BaseHeaderSize)
- (ClassLargePositiveInteger << ShiftForWord)); value = integerValue; } if ((sizeof(value)) == 4) { return signed32BitIntegerFor(integerValue); } check = value >> 32; if (check == 0) { return signed32BitIntegerFor(integerValue); } newLargeInteger = instantiateSmallClasssizeInBytes(largeClass,
BaseHeaderSize + 8); for (i = 0; i <= 7; i += 1) { intValue = ( value >> (i * 8)) & 255; byteAtput((newLargeInteger + BaseHeaderSize) + i, intValue); } return newLargeInteger; }
Begin forwarded message:
From: tim Rowledge tim@rowledge.org Date: June 7, 2006 10:02:40 PM PDT (CA) To: squeak vm vm-dev@discuss.squeakfoundation.org Subject: Re: InterpreterProxy>>signed64BitIntegerFor: badly broken
On 7-Jun-06, at 6:58 PM, Andreas Raab wrote:
Hi Guys -
I don't know if you ever used the above method but it's horribly, horribly broken. I wrote a little test primitive (see below) that simply used signed64BitIntegerFor(signed64BitValueOf(oop)) and then a loop like here:
0 to: 63 do:[:i| n := 1 bitShift: i. (self test64BitInt: n) = n ifFalse:[self halt: i]. ].Starting from i = 31 Every. Last. Result. Is Wrong. Can you imagine?
It gets even better, since it's broken in different ways: For i=31 the result is negated, for everything beyound 31 the resulting large integer is non-normalized (and therefore not comparing correctly).
Any ideas?
Well for starters the signed64BitIntegerFor: code assumes an 8 byte large integer no matter what the value being converted so that's going to cause your non-normalized problem. I'm fairly sure you can work out how to fix that bit quickly enough.
I'm not absolutely sure(and I can't be bothered to look it up right now) but wouldn't 1<<31 be a negative value when treated as a 32 bit word? It looks to me as if signed32BitInteger might be the wrong thing to use in signed64itInteger, with positive32BitInteger a bit more plausible.
I have vague memories of when this code was written, mostly of it being related to long file pointers in OSs I wasn't running at that time. Thus I would have relied upon testing by involved parties and taken their word as to the viability of the code. I guess that once again the value of good tests is demonstrated.
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange OpCodes: VMB: Verify, then Make Bad
Workaround I had
signed64BitIntegerForOVERRIDE: integerValue "Answer a new String copied from a null-terminated C string. Caution: This may invoke the garbage collector." | checkUnsignedLong checkUnsignedLongMax |
self var: 'integerValue' type: 'const sqLong'. self var: 'integerValue' type: 'sqLong'. self var: 'checkLong' type: 'sqLong'. self var: 'checkUnsignedLong' type: 'unsigned long long'. self var: 'checkUnsignedLongMax' type: 'unsigned long long'. self var: 'value' type: 'sqLong'. checkUnsignedLong := integerValue. checkUnsignedLongMax := 16rFFFFFFFF. checkUnsignedLong > checkUnsignedLongMax ifFalse: [^interpreterProxy positive32BitIntegerFor:checkUnsignedLong].
^interpreterProxy signed64BitIntegerFor: integerValue.
Hi -
I've filed a third version with http://bugs.squeak.org/view.php?id=6987 since the last one had a problem in positive64BitIntegerFor: (it created non-normalized integers again when the result was a small integer).
Cheers, - Andreas
Andreas Raab wrote:
And of course, positive64BitIntegerFor: was broken, too (creating non-normalized large integers). Sigh. Here is a full set of the fixed versions.
Cheers,
- Andreas
Andreas Raab wrote:
Try the attached versions. They should be good - I tested with quite a range of inputs and outputs.
Cheers,
- Andreas
John M McIntosh wrote:
I've smacked into this in doing the gstreamer stuff last weekend. Thankfully an sunit test case promptly failed for 64bit work I didn't see an issue with 32bit signed, but likely didn't test.
Attached below is a note I sent to Tim and my workaround, but he's doing some personal business in the UK and I doubt he will address anytime soon. I put together a workaround, but if you can supply the entire smalltalk method that would be helpful.
On Mar 19, 2008, at 8:44 PM, Andreas Raab wrote:
Hi -
I just noticed hat Interpreter>>signed32BitValueOf: and signed64BitValueOf: are broken for edge cases. The following example will illustrate the problem:
array := IntegerArray new: 1. array at: 1 put: 16rFFFFFFFF. "should fail but doesn't" array at: 1. "answers -1 incorrectly"
array := IntegerArray new: 1. array at: 1 put: -16rFFFFFFFF. "should fail but doesn't" array at: 1. "answers 1 incorrectly"
The problem is that both signed32BitValueOf: as well as signed64BitValueOf: do not test whether the high bit of the magnitude is set (which it mustn't to fit into a signed integer). The fix is trivial in both cases - basically all that's needed at the end of both functions is this:
"Filter out values out of range for the signed interpretation such as 16rFFFFFFFF (positive w/ bit 32 set) and -16rFFFFFFFF (negative w/ bit 32 set). Since the sign is implicit in the class we require that the high bit of the magnitude is not set which is a simple test here" value < 0 ifTrue:[^self primitiveFail]. negative ifTrue:[^0 - value] ifFalse:[^value]
Cheers,
- Andreas
Did we fix this? I'm sure I've smacked into this in doing the gstreamer stuff
I have this below if you give it 0x00000000 FFFFFFFF
the check = value >> 32; if (check == 0) { return signed32BitIntegerFor(integerValue); }
returns zero because the 0xFFFFFFFF >> 32 is zero and then we're off to signed32BitIntegerFor however that is wrong.
/* Return a Large Integer object for the given integer value */
sqInt signed64BitIntegerFor(sqLong integerValue) { register struct foo * foo = &fum; sqLong value; sqInt newLargeInteger; sqInt largeClass; sqInt check; sqInt intValue; sqInt i;
if (integerValue < 0) { largeClass = longAt((foo->specialObjectsOop +BaseHeaderSize) + (ClassLargeNegativeInteger << ShiftForWord)); value = 0 - integerValue; } else { largeClass = longAt((foo->specialObjectsOop + BaseHeaderSize) + (ClassLargePositiveInteger << ShiftForWord)); value = integerValue; } if ((sizeof(value)) == 4) { return signed32BitIntegerFor(integerValue); } check = value >> 32; if (check == 0) { return signed32BitIntegerFor(integerValue); } newLargeInteger = instantiateSmallClasssizeInBytes(largeClass, BaseHeaderSize + 8); for (i = 0; i <= 7; i += 1) { intValue = ( value >> (i * 8)) & 255; byteAtput((newLargeInteger + BaseHeaderSize) + i, intValue); } return newLargeInteger; }
Begin forwarded message:
From: tim Rowledge tim@rowledge.org Date: June 7, 2006 10:02:40 PM PDT (CA) To: squeak vm vm-dev@discuss.squeakfoundation.org Subject: Re: InterpreterProxy>>signed64BitIntegerFor: badly broken
On 7-Jun-06, at 6:58 PM, Andreas Raab wrote:
Hi Guys -
I don't know if you ever used the above method but it's horribly, horribly broken. I wrote a little test primitive (see below) that simply used signed64BitIntegerFor(signed64BitValueOf(oop)) and then a loop like here:
0 to: 63 do:[:i| n := 1 bitShift: i. (self test64BitInt: n) = n ifFalse:[self halt: i]. ].Starting from i = 31 Every. Last. Result. Is Wrong. Can you imagine?
It gets even better, since it's broken in different ways: For i=31 the result is negated, for everything beyound 31 the resulting large integer is non-normalized (and therefore not comparing correctly).
Any ideas?
Well for starters the signed64BitIntegerFor: code assumes an 8 byte large integer no matter what the value being converted so that's going to cause your non-normalized problem. I'm fairly sure you can work out how to fix that bit quickly enough.
I'm not absolutely sure(and I can't be bothered to look it up right now) but wouldn't 1<<31 be a negative value when treated as a 32 bit word? It looks to me as if signed32BitInteger might be the wrong thing to use in signed64itInteger, with positive32BitInteger a bit more plausible.
I have vague memories of when this code was written, mostly of it being related to long file pointers in OSs I wasn't running at that time. Thus I would have relied upon testing by involved parties and taken their word as to the viability of the code. I guess that once again the value of good tests is demonstrated.
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange OpCodes: VMB: Verify, then Make Bad
Workaround I had
signed64BitIntegerForOVERRIDE: integerValue "Answer a new String copied from a null-terminated C string. Caution: This may invoke the garbage collector." | checkUnsignedLong checkUnsignedLongMax |
self var: 'integerValue' type: 'const sqLong'. self var: 'integerValue' type: 'sqLong'. self var: 'checkLong' type: 'sqLong'. self var: 'checkUnsignedLong' type: 'unsigned long long'. self var: 'checkUnsignedLongMax' type: 'unsigned long long'. self var: 'value' type: 'sqLong'. checkUnsignedLong := integerValue. checkUnsignedLongMax := 16rFFFFFFFF. checkUnsignedLong > checkUnsignedLongMax ifFalse: [^interpreterProxy positive32BitIntegerFor:checkUnsignedLong].
^interpreterProxy signed64BitIntegerFor: integerValue.
Oh, and for the records signed64BitIntegerFor is broken too and in more than one way: First the short-cut into 32bit is wrong since it tests the magnitude and allows signed integers with 32 bits of magnitude to slip through (reverse problem of the one described below), and of course, it creates non-normalized LargeIntegers which will compare wrongly. If you have a file that exceeds 32 bits you can test this via:
file size = (file size + 0)
which will evaluate to false.
Cheers, - Andreas
Andreas Raab wrote:
Hi -
I just noticed hat Interpreter>>signed32BitValueOf: and signed64BitValueOf: are broken for edge cases. The following example will illustrate the problem:
array := IntegerArray new: 1. array at: 1 put: 16rFFFFFFFF. "should fail but doesn't" array at: 1. "answers -1 incorrectly"
array := IntegerArray new: 1. array at: 1 put: -16rFFFFFFFF. "should fail but doesn't" array at: 1. "answers 1 incorrectly"
The problem is that both signed32BitValueOf: as well as signed64BitValueOf: do not test whether the high bit of the magnitude is set (which it mustn't to fit into a signed integer). The fix is trivial in both cases - basically all that's needed at the end of both functions is this:
"Filter out values out of range for the signed interpretation such as 16rFFFFFFFF (positive w/ bit 32 set) and -16rFFFFFFFF (negative w/ bit 32 set). Since the sign is implicit in the class we require that the high bit of the magnitude is not set which is a simple test here" value < 0 ifTrue:[^self primitiveFail]. negative ifTrue:[^0 - value] ifFalse:[^value]
Cheers,
- Andreas
On Wed, 19 Mar 2008 20:44:00 -0700 Andreas Raab andreas.raab@gmx.de wrote:
Hi -
I just noticed hat Interpreter>>signed32BitValueOf: and signed64BitValueOf: are broken for edge cases. The following example will illustrate the problem:
<snip>
Have you created a Mantis issue for keeping historical records of bugs etc?
Gulik.
Michael van der Gulik wrote:
On Wed, 19 Mar 2008 20:44:00 -0700 Andreas Raab andreas.raab@gmx.de wrote:
Hi -
I just noticed hat Interpreter>>signed32BitValueOf: and signed64BitValueOf: are broken for edge cases. The following example will illustrate the problem:
<snip>
Have you created a Mantis issue for keeping historical records of bugs etc?
Not yet. If you look at the timing it's been late yesterday evening and really, debugging signed32BitValueOf: wasn't what I was trying to do anyway ;-)
Cheers, - Andreas
Er is this still broken?
I was writing some Sunits for the Alien stuff and found
alien signedLongAt: 1 put: -1*16r80000000. " -2147483648" failed. triggered by value < 0 ifTrue:[^self primitiveFail]. as coded below since value is -2147483648
range is Signed: −2,147,483,648 to +2,147,483,647 http://en.wikipedia.org/wiki/Integer_(computer_science)
On 19-Mar-08, at 8:44 PM, Andreas Raab wrote:
Hi -
I just noticed hat Interpreter>>signed32BitValueOf: and signed64BitValueOf: are broken for edge cases. The following example will illustrate the problem:
array := IntegerArray new: 1. array at: 1 put: 16rFFFFFFFF. "should fail but doesn't" array at: 1. "answers -1 incorrectly"
array := IntegerArray new: 1. array at: 1 put: -16rFFFFFFFF. "should fail but doesn't" array at: 1. "answers 1 incorrectly"
The problem is that both signed32BitValueOf: as well as signed64BitValueOf: do not test whether the high bit of the magnitude is set (which it mustn't to fit into a signed integer). The fix is trivial in both cases - basically all that's needed at the end of both functions is this:
"Filter out values out of range for the signed interpretation such as 16rFFFFFFFF (positive w/ bit 32 set) and -16rFFFFFFFF (negative w/ bit 32 set). Since the sign is implicit in the class we require that the high bit of the magnitude is not set which is a simple test here" value < 0 ifTrue:[^self primitiveFail]. negative ifTrue:[^0 - value] ifFalse:[^value]
Cheers,
- Andreas
-- = = = ======================================================================== John M. McIntosh johnmci@smalltalkconsulting.com Corporate Smalltalk Consulting Ltd. http://www.smalltalkconsulting.com = = = ========================================================================
Just to clarify with the current VMMaker source code and the FFI plugin if I do
| foo | foo := ByteArray new: 4. foo signedLongAt:1 put: -2147483648
that fails, should it?
Begin forwarded message:
From: John M McIntosh johnmci@smalltalkconsulting.com Date: November 23, 2008 11:27:14 PM PST (CA) To: Squeak Virtual Machine Development Discussion <vm-dev@lists.squeakfoundation.org
Cc: Andreas Raab andreas.raab@qwaq.com, "David T. Lewis" <lewis@mail.msen.com
Subject: Re: [Vm-dev] Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken Reply-To: johnmci@smalltalkconsulting.com
Er is this still broken?
I was writing some Sunits for the Alien stuff and found
alien signedLongAt: 1 put: -1*16r80000000. " -2147483648" failed. triggered by value < 0 ifTrue:[^self primitiveFail]. as coded below since value is -2147483648
range is Signed: â2,147,483,648 to +2,147,483,647 http://en.wikipedia.org/wiki/Integer_(computer_science)
On 19-Mar-08, at 8:44 PM, Andreas Raab wrote:
Hi -
I just noticed hat Interpreter>>signed32BitValueOf: and signed64BitValueOf: are broken for edge cases. The following example will illustrate the problem:
array := IntegerArray new: 1. array at: 1 put: 16rFFFFFFFF. "should fail but doesn't" array at: 1. "answers -1 incorrectly"
array := IntegerArray new: 1. array at: 1 put: -16rFFFFFFFF. "should fail but doesn't" array at: 1. "answers 1 incorrectly"
The problem is that both signed32BitValueOf: as well as signed64BitValueOf: do not test whether the high bit of the magnitude is set (which it mustn't to fit into a signed integer). The fix is trivial in both cases - basically all that's needed at the end of both functions is this:
"Filter out values out of range for the signed interpretation such as 16rFFFFFFFF (positive w/ bit 32 set) and -16rFFFFFFFF (negative w/ bit 32 set). Since the sign is implicit in the class we require that the high bit of the magnitude is not set which is a simple test here" value < 0 ifTrue:[^self primitiveFail]. negative ifTrue:[^0 - value] ifFalse:[^value]
Cheers,
- Andreas
-- = = = = = ====================================================================== John M. McIntosh johnmci@smalltalkconsulting.com Corporate Smalltalk Consulting Ltd. http:// www.smalltalkconsulting.com = = = = = ======================================================================
-- = = = ======================================================================== John M. McIntosh johnmci@smalltalkconsulting.com Corporate Smalltalk Consulting Ltd. http://www.smalltalkconsulting.com = = = ========================================================================
It shouldn't fail; it's a bug. Unfortunately, I have no time to fix it.
Cheers, - Andreas
John M McIntosh wrote:
Just to clarify with the current VMMaker source code and the FFI plugin if I do
| foo | foo := ByteArray new: 4. foo signedLongAt:1 put: -2147483648
that fails, should it?
Begin forwarded message:
From: John M McIntosh johnmci@smalltalkconsulting.com Date: November 23, 2008 11:27:14 PM PST (CA) To: Squeak Virtual Machine Development Discussion vm-dev@lists.squeakfoundation.org Cc: Andreas Raab andreas.raab@qwaq.com, "David T. Lewis" lewis@mail.msen.com Subject: Re: [Vm-dev] Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken Reply-To: johnmci@smalltalkconsulting.com
Er is this still broken?
I was writing some Sunits for the Alien stuff and found
alien signedLongAt: 1 put: -1*16r80000000. " -2147483648" failed. triggered by value < 0 ifTrue:[^self primitiveFail]. as coded below since value is -2147483648
range is Signed: −2,147,483,648 to +2,147,483,647 http://en.wikipedia.org/wiki/Integer_(computer_science)
On 19-Mar-08, at 8:44 PM, Andreas Raab wrote:
Hi -
I just noticed hat Interpreter>>signed32BitValueOf: and signed64BitValueOf: are broken for edge cases. The following example will illustrate the problem:
array := IntegerArray new: 1. array at: 1 put: 16rFFFFFFFF. "should fail but doesn't" array at: 1. "answers -1 incorrectly"
array := IntegerArray new: 1. array at: 1 put: -16rFFFFFFFF. "should fail but doesn't" array at: 1. "answers 1 incorrectly"
The problem is that both signed32BitValueOf: as well as signed64BitValueOf: do not test whether the high bit of the magnitude is set (which it mustn't to fit into a signed integer). The fix is trivial in both cases - basically all that's needed at the end of both functions is this:
"Filter out values out of range for the signed interpretation such as 16rFFFFFFFF (positive w/ bit 32 set) and -16rFFFFFFFF (negative w/ bit 32 set). Since the sign is implicit in the class we require that the high bit of the magnitude is not set which is a simple test here" value < 0 ifTrue:[^self primitiveFail]. negative ifTrue:[^0 - value] ifFalse:[^value]
Cheers,
- Andreas
--
John M. McIntosh johnmci@smalltalkconsulting.com Corporate Smalltalk Consulting Ltd. http://www.smalltalkconsulting.com ===========================================================================
--
John M. McIntosh johnmci@smalltalkconsulting.com Corporate Smalltalk Consulting Ltd. http://www.smalltalkconsulting.com ===========================================================================
Ok, well I wrote some SUnit first, to check for byte, short, long. signed and unsigned. For byte, and short we can check the entire range of -255 to 255 and -65,535 to 65,535 for valid at:put: results I have a fix for the −2,147,483,648 issue Now tackling 64bit ones, but first I need to alter the alien ffi logic to add get/put longlong since we don't seem to actually have an API in say FFI to access the values.
On 25-Nov-08, at 7:03 PM, Andreas Raab wrote:
It shouldn't fail; it's a bug. Unfortunately, I have no time to fix it.
Cheers,
- Andreas
John M McIntosh wrote:
Just to clarify with the current VMMaker source code and the FFI plugin if I do | foo | foo := ByteArray new: 4. foo signedLongAt:1 put: -2147483648 that fails, should it? Begin forwarded message:
From: John M McIntosh johnmci@smalltalkconsulting.com Date: November 23, 2008 11:27:14 PM PST (CA) To: Squeak Virtual Machine Development Discussion <vm-dev@lists.squeakfoundation.org
Cc: Andreas Raab andreas.raab@qwaq.com, "David T. Lewis" <lewis@mail.msen.com
Subject: Re: [Vm-dev] Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken Reply-To: johnmci@smalltalkconsulting.com
Er is this still broken?
I was writing some Sunits for the Alien stuff and found
alien signedLongAt: 1 put: -1*16r80000000. " -2147483648" failed. triggered by value < 0 ifTrue:[^self primitiveFail]. as coded below since value is -2147483648
range is Signed: −2,147,483,648 to +2,147,483,647 http://en.wikipedia.org/wiki/Integer_(computer_science)
On 19-Mar-08, at 8:44 PM, Andreas Raab wrote:
Hi -
I just noticed hat Interpreter>>signed32BitValueOf: and signed64BitValueOf: are broken for edge cases. The following example will illustrate the problem:
array := IntegerArray new: 1. array at: 1 put: 16rFFFFFFFF. "should fail but doesn't" array at: 1. "answers -1 incorrectly"
array := IntegerArray new: 1. array at: 1 put: -16rFFFFFFFF. "should fail but doesn't" array at: 1. "answers 1 incorrectly"
The problem is that both signed32BitValueOf: as well as signed64BitValueOf: do not test whether the high bit of the magnitude is set (which it mustn't to fit into a signed integer). The fix is trivial in both cases - basically all that's needed at the end of both functions is this:
"Filter out values out of range for the signed interpretation such as 16rFFFFFFFF (positive w/ bit 32 set) and -16rFFFFFFFF (negative w/ bit 32 set). Since the sign is implicit in the class we require that the high bit of the magnitude is not set which is a simple test here" value < 0 ifTrue:[^self primitiveFail]. negative ifTrue:[^0 - value] ifFalse:[^value]
Cheers,
- Andreas
-- = = = = = = = ==================================================================== John M. McIntosh johnmci@smalltalkconsulting.com Corporate Smalltalk Consulting Ltd. http://www.smalltalkconsulting.com = = = = = = = ====================================================================
-- = = = = = = ===================================================================== John M. McIntosh johnmci@smalltalkconsulting.com Corporate Smalltalk Consulting Ltd. http://www.smalltalkconsulting.com = = = = = = =====================================================================
-- = = = ======================================================================== John M. McIntosh johnmci@smalltalkconsulting.com Corporate Smalltalk Consulting Ltd. http://www.smalltalkconsulting.com = = = ========================================================================
vm-dev@lists.squeakfoundation.org
-
Andreas Raab -
Igor Stasenko -
John M McIntosh -
Michael van der Gulik