Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken
Locked
Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken
 
|
Re: Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken
|
|
Re: Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken
|
|
Re: Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken
|
|
Re: Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken
|
|
In reply to this post by johnmci
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 <[hidden email]> >>> Date: June 7, 2006 10:02:40 PM PDT (CA) >>> To: squeak vm <[hidden email]> >>> 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; [hidden email]; 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. > > > > |
SignedIntFixes.1.cs (4K)
Re: Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken
|
|
Re: Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken
|
|
Re: Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken
|
|
In reply to this post by Andreas.Raab
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 <[hidden email]> >>>> Date: June 7, 2006 10:02:40 PM PDT (CA) >>>> To: squeak vm <[hidden email]> >>>> 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; [hidden email]; 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. >> >> >> >> |
Re: Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken
|
|
Re: Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken
|
|
Fwd: Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken
|
|
Re: Fwd: Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken
|
|
Re: Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken
|
|
| Free forum by Nabble | Edit this page |