[Vm-dev] VM Maker: VMMaker.oscog-eem.2338.mcz
Levente Uzonyi
leves at caesar.elte.hu
Thu Feb 22 23:14:07 UTC 2018
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!
More information about the Vm-dev
mailing list