[Vm-dev] Interpreter>>signed32BitValueOf: and signed64BitValueOf:
broken
John M McIntosh
johnmci at smalltalkconsulting.com
Thu Mar 20 05:52:17 UTC 2008
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 at rowledge.org>
>> Date: June 7, 2006 10:02:40 PM PDT (CA)
>> To: squeak vm <vm-dev at 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 at 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.
More information about the Vm-dev
mailing list