[Vm-dev] Integer overflow with BitBlt rule 20 and depth 32

[Juan Vuletich]

Hi Folks,

Looking in detail I found two issues here. One is that the addition can 
silently overflow. The other one is that in the generated C code ALL the 
variables (arguments and temporaries) are int and not unsigned. (BTW, 
this is for sure affecting other rules, not only rgbAdd.) If we fixed 
the int issue, then much simpler (and a bit faster) code would work:

partitionedAdd: word1 to: word2 nBits: nBits nPartitions: nParts
    "Add word1 to word2 as nParts partitions of nBits each.
    This is useful for packed pixels, or packed colors"
    | mask sum result maskedWord1 |
    mask := maskTable at: nBits.  "partition mask starts at the right"
    result := 0.
    1 to: nParts do: [ :i |
        maskedWord1 _ word1 bitAnd: mask.
        sum := maskedWord1 + (word2 bitAnd: mask).
        (sum <= mask  "result must not carry out of partition"
                    and: [ sum >= maskedWord1 ])                      
            "**** This is the only change ****"    
            ifTrue: [result := result bitOr: sum]
            ifFalse: [result := result bitOr: mask].
        mask := mask << nBits  "slide left to next partition"].
    ^ result

I believe we need to add explicit declarations of all variables being 
unsigned. Or perhaps enhance a bit the code generator, by allowing a 
plugin to declare its default numeric type. For BitBlt it could be 
unsigned. For DSP stuff it could be float or double, making the Slang 
code much nicer by not needing all the explicit type declarations. What 
do you think?

Cheers,
Juan Vuletich

Henrik Sperre Johansen wrote:
>  
>
> ------------------------------------------------------------------------
>
> On 20.10.2009 15:56, David T. Lewis wrote:
>>
>> Could this be the cause of issue for which Juan was asking help?
>>
>>    Subject line "[Vm-dev] Bug in BitBlt. Need Help."
>>    
>> http://lists.squeakfoundation.org/pipermail/vm-dev/2009-September/003160.html 
>>
>>    
> Yes, that's exactly it, sorry I had not seen it.
> Attached is a proof-of-concept fix (for partitionedAdd: only), valid 
> as long as nParts > 1 ( I suggest one wouldn't use partitionedAdd 
> anyways if it were :) )
> If anyone has a better idea, it'd be appreciated!
>
> Correctness test:
> | sourceForm destForm blt correctAlphas |
>     correctAlphas := 0.
>     0  to: 255 do: [:sourceAlpha |
>         sourceForm := Form extent: 1 @ 1 depth: 32.
>         sourceForm bits at: 1 put: sourceAlpha << 24 + (33 << 16) + 
> (25 << 8) + 27.
>         0 to: 255 do: [:destAlpha |
>             destForm := Form extent: 1 @ 1 depth: 32.
>             destForm bits at: 1 put: destAlpha << 24 + (255 << 16) + 
> (255 << 8) + 255.
>              blt := BitBlt new.
>              blt sourceForm: sourceForm.
>              blt sourceOrigin: 0 @ 0.
>              blt setDestForm: destForm.
>              blt destOrigin: 0 @ 0.
>              blt combinationRule: 20.
>              blt copyBits.
>              correctAlphas := correctAlphas
>                + (((blt destForm bits at: 1) digitAt: 4) = (destAlpha 
> + sourceAlpha min: 255)
>                         ifTrue: [1]
>                         ifFalse: [0])
>      ]].
>     self assert: 65536 equals: correctAlphas
>
> Performance is not impacted to a significant degree as far as I can 
> tell, the from runtimes of the test:
> [|sourceForm destForm blt|
> destForm := Form extent: 99 at 99 depth: 32.
> 1 to: destForm bits size do: [:ix |
> destForm  bits at: ix put: ((192 << 24) + (255 << 16) + (255 << 8) + 
> 255).].
> sourceForm := Form extent: 99 at 99 depth: 32.
> 1 to: sourceForm bits size do: [:ix |
> sourceForm bits at: ix put: ((192 << 24) + (33 << 16) + (25 << 8) + 
> 27).].
> blt := BitBlt new.
> blt sourceForm: sourceForm.
> blt sourceOrigin: 0 at 0.
> blt setDestForm: destForm.
> blt destOrigin: 0 at 0.
> blt combinationRule: 20.
> 5000 timesRepeat: [
> blt copyBits.]] timeToRun
>
> Cheers,
> Henry
> ------------------------------------------------------------------------
>
>
> No virus found in this incoming message.
> Checked by AVG - www.avg.com 
> Version: 8.5.423 / Virus Database: 270.14.26/2451 - Release Date: 10/22/09 08:51:00
>