[Vm-dev] VM Maker: VMMaker.oscog-EstebanLorenzano.1717.mcz

[Eliot Miranda]

Hi Esteban,

On Tue, Mar 8, 2016 at 11:32 PM, <commits at source.squeak.org> wrote:

>
> Esteban Lorenzano uploaded a new version of VMMaker to project VM Maker:
> http://source.squeak.org/VMMaker/VMMaker.oscog-EstebanLorenzano.1717.mcz
>
> ==================== Summary ====================
>
> Name: VMMaker.oscog-EstebanLorenzano.1717
> Author: EstebanLorenzano
> Time: 9 March 2016, 8:31:33.743422 am
> UUID: 271f0af9-e85c-4afc-bf17-c4aa735ddaaa
> Ancestors: VMMaker.oscog-eem.1716
>
> using *machine methods instead singned64... and positive64...
>
> =============== Diff against VMMaker.oscog-eem.1716 ===============
>
> Item was changed:
>   ----- Method: ThreadedFFIPlugin>>primitiveFFIIntegerAt (in category
> 'primitives') -----
>   primitiveFFIIntegerAt
>         "Answer a (signed or unsigned) n byte integer from the given byte
> offset
>          in the receiver, using the platform's endianness."
>         | isSigned byteSize byteOffset rcvr addr value mask valueOop |
>         <var: 'value' type: #usqLong>
>         <var: 'mask' type: #usqLong>
>         <export: true>
>         <inline: false>
>         isSigned := interpreterProxy booleanValueOf: (interpreterProxy
> stackValue: 0).
>         byteSize := interpreterProxy stackIntegerValue: 1.
>         byteOffset := interpreterProxy stackIntegerValue: 2.
>         rcvr := interpreterProxy stackObjectValue: 3.
>         interpreterProxy failed ifTrue:[^0].
>         (byteOffset > 0
>          and: [(byteSize between: 1 and: 8)
>          and: [(byteSize bitAnd: byteSize - 1) = 0 "a.k.a.
> isPowerOfTwo"]]) ifFalse:
>                 [^interpreterProxy primitiveFail].
>         addr := self ffiAddressOf: rcvr startingAt: byteOffset size:
> byteSize.
>         interpreterProxy failed ifTrue:[^0].
>         byteSize <= 2
>                 ifTrue:
>                         [byteSize = 1
>                                 ifTrue: [value := self cCoerceSimple:
> (interpreterProxy byteAt: addr) to: #'unsigned char']
>                                 ifFalse: [value := self cCoerceSimple:
> (interpreterProxy shortAt: addr) to: #'unsigned short']]
>                 ifFalse:
>                         [byteSize = 4
> +                               ifTrue: [value := self cCoerceSimple:
> (interpreterProxy long32At: addr) to: #'unsigned int']
> -                               ifTrue: [value := interpreterProxy
> long32At: addr]
>                                 ifFalse: [value := interpreterProxy
> long64At: addr]].
>         byteSize < BytesPerWord
>                 ifTrue:
>                         [isSigned ifTrue: "sign extend value"
>                                 [mask := 1 << (byteSize * 8 - 1).
>                                 value := (value bitAnd: mask-1) - (value
> bitAnd: mask)].
>                          "note: byte/short (&long if BytesPerWord=8) never
> exceed SmallInteger range"
>                          valueOop := interpreterProxy integerObjectOf:
> value]
>                 ifFalse: "general 64 bit integer; note these never fail"
>                         [valueOop := isSigned
> +
>  ifTrue:[interpreterProxy signedMachineIntegerFor: value]
> +
>  ifFalse:[interpreterProxy positiveMachineIntegerFor: value]].
> -
>  ifTrue:[interpreterProxy signed64BitIntegerFor: value]
> -
>  ifFalse:[interpreterProxy positive64BitIntegerFor: value]].
>         ^interpreterProxy pop: 4 thenPush: valueOop!
>

This doesn't make sense to me.  The lines

                        [valueOop := isSigned
+
 ifTrue:[interpreterProxy signedMachineIntegerFor: value]
+
 ifFalse:[interpreterProxy positiveMachineIntegerFor: value]].
-
 ifTrue:[interpreterProxy signed64BitIntegerFor: value]
-
 ifFalse:[interpreterProxy positive64BitIntegerFor: value]].

seem wrong.  If we're on a 232-bit machine then signedMachineIntegerFor: &
positiveMachineIntegerFor: will fail for 64-bit results, right?  Whereas on
both 32- and 64-bit machines signed64BitIntegerFor: &
positive64BitIntegerFor: will work.

I think the bug above is that if BytesPerWord is 4 there is no
sign-extension of 32-bits to 64-bits.  So it probably needs to read

                ifFalse: "general 64 bit integer; note these never fail"
                        [(isSigned and: [byteSize = 4]) ifTrue:
"sign-extend 32-bit results to 64-bits..."
                                [mask := 1 << (byteSize * 8 - 1).
                                 value := (value bitAnd: mask-1) - (value
bitAnd: mask)].
                        valueOop := isSigned

ifTrue:[interpreterProxy signed64BitIntegerFor: value]

ifFalse:[interpreterProxy positive64BitIntegerFor: value]].


It would really help if you tested both 64-bit and 32-bit VMs if you're
changing this code.  I know that's inconvenient but asking me to fix
64-bits if you break it is also unfair :-(.

I will test the above, and if it's right, commit.
_,,,^..^,,,_
best, Eliot
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20160308/e7bff721/attachment.htm