# Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken

13 messages
Open this post in threaded view
|

## Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken

 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
Open this post in threaded view
|

## Re: Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken

 On 20/03/2008, Andreas Raab <[hidden email]> 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 > -- Best regards, Igor Stasenko AKA sig.
Open this post in threaded view
|

## Re: Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken

 In reply to this post by Andreas.Raab   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.
Open this post in threaded view
|

## Re: Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken

 In reply to this post by Andreas.Raab   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 >
Open this post in threaded view
|

## 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) Download Attachment
Open this post in threaded view
|

## Re: Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken

 In reply to this post by Andreas.Raab   On Wed, 19 Mar 2008 20:44:00 -0700 Andreas Raab <[hidden email]> wrote: >   > Hi - > > I just noticed hat Interpreter>>signed32BitValueOf: and > signed64BitValueOf: are broken for edge cases. The following example > will illustrate the problem: Have you created a Mantis issue for keeping historical records of bugs etc? Gulik. -- Michael van der Gulik <[hidden email]>
Open this post in threaded view
|

## Re: Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken

 Michael van der Gulik wrote: > On Wed, 19 Mar 2008 20:44:00 -0700 > Andreas Raab <[hidden email]> wrote: > >>   >> Hi - >> >> I just noticed hat Interpreter>>signed32BitValueOf: and >> signed64BitValueOf: are broken for edge cases. The following example >> will illustrate the problem: > > > > 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
Open this post in threaded view
|

## 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. >> >> >> >> SignedIntFixes.2.cs (5K) Download Attachment
Open this post in threaded view
|

## Re: Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken

 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 <[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. >>> >>> >>> >>>
Open this post in threaded view
|

## Re: Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken

 In reply to this post by Andreas.Raab 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 <[hidden email]> Corporate Smalltalk Consulting Ltd.  http://www.smalltalkconsulting.com= = = ========================================================================
Open this post in threaded view
|

## Fwd: Interpreter>>signed32BitValueOf: and signed64BitValueOf: broken

 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 <[hidden email]> > Date: November 23, 2008 11:27:14 PM PST (CA) > To: Squeak Virtual Machine Development Discussion <[hidden email] > > > Cc: Andreas Raab <[hidden email]>, "David T. Lewis" <[hidden email] > > > Subject: Re: [Vm-dev] Interpreter>>signed32BitValueOf: and   > signed64BitValueOf: broken > Reply-To: [hidden email] > > 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 <[hidden email]> > Corporate Smalltalk Consulting Ltd.  http:// > www.smalltalkconsulting.com > = > = > = > = > = > ====================================================================== > > > -- = = = ======================================================================== John M. McIntosh <[hidden email]> Corporate Smalltalk Consulting Ltd.  http://www.smalltalkconsulting.com= = = ========================================================================