[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