[Vm-dev] VM Maker: VMMaker.oscog-eem.2338.mcz

[Eliot Miranda]

Hi Levente,

    I really appreciate you taking a look.  I will say that the rewrite tried the stay as close to the originals while changing 1-relative indexing to 0-relative.  But I think we can also improve on the originals.  Indeed I spotted and fixed a potential bounds violation in primitiveConvert8Bit that only checked the size of one of the two array arguments.

So can I ask you to read the primitives in the previous commit too?  Let's polish these a little :-)

_,,,^..^,,,_ (phone)

> On Feb 22, 2018, at 3:14 PM, Levente Uzonyi <leves at caesar.elte.hu> wrote:
> 
> Just had a look at this snippet, and wrote some quick inline comments.
> 
>> On Thu, 22 Feb 2018, commits at source.squeak.org wrote:
>> 
>> Eliot Miranda uploaded a new version of VMMaker to project VM Maker:
>> http://source.squeak.org/VMMaker/VMMaker.oscog-eem.2338.mcz
>> 
>> ==================== Summary ====================
>> 
>> Name: VMMaker.oscog-eem.2338
>> Author: eem
>> Time: 22 February 2018, 10:25:17.794166 am
>> UUID: 1672c678-41b4-41f2-a061-e201562fc75c
>> Ancestors: VMMaker.oscog-eem.2337
>> 
>> Fix a slip in primitiveDecompressFromByteArray caught by IncludedMethodsTests.  Thank you David!
>> 
>> =============== Diff against VMMaker.oscog-eem.2337 ===============
>> 
>> Item was changed:
>> ----- Method: MiscPrimitivePlugin>>primitiveDecompressFromByteArray (in category 'primitives') -----
>> primitiveDecompressFromByteArray
>>    "Bitmap decompress: bm fromByteArray: ba at: index"
>>    <export: true>
>>    | bm ba index i anInt code data end k n pastEnd |
>>    <var: 'ba' type: #'unsigned char *'>
>>    <var: 'bm' type: #'int *'>
>>    <var: 'anInt' type: #'unsigned int'>
>>    <var: 'code' type: #'unsigned int'>
>>    <var: 'data' type: #'unsigned int'>
>>    <var: 'n' type: #'unsigned int'>
> 
> Shouldn't i be declared as unsigned int too? It's used for indexing.
> 
>>    bm := self cCode: [interpreterProxy arrayValueOf: (interpreterProxy stackValue: 2)]
>>                inSmalltalk: [interpreterProxy
>>                                cCoerce: (interpreterProxy arrayValueOf: (interpreterProxy stackValue: 2))
>>                                to: #'int *'].
>>    (interpreterProxy isOopImmutable: (interpreterProxy stackValue: 2)) ifTrue:
>>        [^interpreterProxy primitiveFailFor: PrimErrNoModification].
>>    (interpreterProxy isBytes: (interpreterProxy stackValue: 1)) ifFalse: [^interpreterProxy primitiveFail].
>>    ba := interpreterProxy firstIndexableField: (interpreterProxy stackValue: 1).
>>    index := interpreterProxy stackIntegerValue: 0.
> 
> Can index be negative or 0? If yes, then the value of i will be negative (unless it's implicit type is unsigned) and that will result in out of bounds indexing into ba a few lines below here.
> 
>>    interpreterProxy failed ifTrue: [^nil].
>>    i := index - 1.
>>    k := 0.
>>    end := interpreterProxy sizeOfSTArrayFromCPrimitive: ba.
>>    pastEnd := interpreterProxy sizeOfSTArrayFromCPrimitive: bm.
>>    [i < end] whileTrue:
>>        [anInt := ba at: i.
>>        i := i + 1.
>>        anInt <= 223 ifFalse:
>>            [anInt <= 254
>>                ifTrue:
>>                    [anInt := anInt - 224 * 256 + (ba at: i).
> 
> I would use bitShift: 8 instead of * 256, just to be consistent with the rest of the method.
> 
>>                    i := i + 1]
>>                ifFalse:
>>                    [anInt := 0.
>>                    1 to: 4 by: 1 do:
>>                        [ :j | anInt := (anInt bitShift: 8) + (ba at: i).
>>                        i := i + 1]]].
>>        n := anInt >> 2.
>> +        k + n > pastEnd ifTrue: [^interpreterProxy primitiveFail].
>> -        k + n >= pastEnd ifTrue: [^interpreterProxy primitiveFail].
>>        code := anInt bitAnd: 3.
>>        "code = 0 ifTrue: [nil]."
>>        code = 1 ifTrue:
>>            [data := ba at: i.
>>            i := i + 1.
>>            data := data bitOr: (data bitShift: 8).
>>            data := data bitOr: (data bitShift: 16).
> 
> I don't know what the point of this part of the code is, but I think the above 4 lines could read:
> 
> [data := 16r10101 * (ba at: i).
> i := i + 1.
> 
> Levente
> 
>>            1 to: n do:
>>                [ :j |
>>                bm at: k put: data.
>>                k := k + 1]].
>>        code = 2 ifTrue:
>>            [data := 0.
>>            1 to: 4 do:
>>                [ :j |
>>                data := (data bitShift: 8) bitOr: (ba at: i).
>>                i := i + 1].
>>            1 to: n do:
>>                [ :j |
>>                bm at: k put: data.
>>                k := k + 1]].
>>        code = 3 ifTrue:
>>            [1 to: n do:
>>                [ :m |
>>                data := 0.
>>                1 to: 4 do:
>>                    [ :j |
>>                    data := (data bitShift: 8) bitOr: (ba at: i).
>>                    i := i + 1].
>>                bm at: k put: data.
>>                k := k + 1]]].
>>    interpreterProxy pop: interpreterProxy methodArgumentCount!