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.