<div dir="ltr">Hi Esteban,<div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 8, 2016 at 11:32 PM, <span dir="ltr"><<a href="mailto:commits@source.squeak.org" target="_blank">commits@source.squeak.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br>
Esteban Lorenzano uploaded a new version of VMMaker to project VM Maker:<br>
<a href="http://source.squeak.org/VMMaker/VMMaker.oscog-EstebanLorenzano.1717.mcz" rel="noreferrer" target="_blank">http://source.squeak.org/VMMaker/VMMaker.oscog-EstebanLorenzano.1717.mcz</a><br>
<br>
==================== Summary ====================<br>
<br>
Name: VMMaker.oscog-EstebanLorenzano.1717<br>
Author: EstebanLorenzano<br>
Time: 9 March 2016, 8:31:33.743422 am<br>
UUID: 271f0af9-e85c-4afc-bf17-c4aa735ddaaa<br>
Ancestors: VMMaker.oscog-eem.1716<br>
<br>
using *machine methods instead singned64... and positive64...<br>
<br>
=============== Diff against VMMaker.oscog-eem.1716 ===============<br>
<br>
Item was changed:<br>
----- Method: ThreadedFFIPlugin>>primitiveFFIIntegerAt (in category 'primitives') -----<br>
primitiveFFIIntegerAt<br>
"Answer a (signed or unsigned) n byte integer from the given byte offset<br>
in the receiver, using the platform's endianness."<br>
| isSigned byteSize byteOffset rcvr addr value mask valueOop |<br>
<var: 'value' type: #usqLong><br>
<var: 'mask' type: #usqLong><br>
<export: true><br>
<inline: false><br>
isSigned := interpreterProxy booleanValueOf: (interpreterProxy stackValue: 0).<br>
byteSize := interpreterProxy stackIntegerValue: 1.<br>
byteOffset := interpreterProxy stackIntegerValue: 2.<br>
rcvr := interpreterProxy stackObjectValue: 3.<br>
interpreterProxy failed ifTrue:[^0].<br>
(byteOffset > 0<br>
and: [(byteSize between: 1 and: 8)<br>
and: [(byteSize bitAnd: byteSize - 1) = 0 "a.k.a. isPowerOfTwo"]]) ifFalse:<br>
[^interpreterProxy primitiveFail].<br>
addr := self ffiAddressOf: rcvr startingAt: byteOffset size: byteSize.<br>
interpreterProxy failed ifTrue:[^0].<br>
byteSize <= 2<br>
ifTrue:<br>
[byteSize = 1<br>
ifTrue: [value := self cCoerceSimple: (interpreterProxy byteAt: addr) to: #'unsigned char']<br>
ifFalse: [value := self cCoerceSimple: (interpreterProxy shortAt: addr) to: #'unsigned short']]<br>
ifFalse:<br>
[byteSize = 4<br>
+ ifTrue: [value := self cCoerceSimple: (interpreterProxy long32At: addr) to: #'unsigned int']<br>
- ifTrue: [value := interpreterProxy long32At: addr]<br>
ifFalse: [value := interpreterProxy long64At: addr]].<br>
byteSize < BytesPerWord<br>
ifTrue:<br>
[isSigned ifTrue: "sign extend value"<br>
[mask := 1 << (byteSize * 8 - 1).<br>
value := (value bitAnd: mask-1) - (value bitAnd: mask)].<br>
"note: byte/short (&long if BytesPerWord=8) never exceed SmallInteger range"<br>
valueOop := interpreterProxy integerObjectOf: value]<br>
ifFalse: "general 64 bit integer; note these never fail"<br>
[valueOop := isSigned<br>
+ ifTrue:[interpreterProxy signedMachineIntegerFor: value]<br>
+ ifFalse:[interpreterProxy positiveMachineIntegerFor: value]].<br>
- ifTrue:[interpreterProxy signed64BitIntegerFor: value]<br>
- ifFalse:[interpreterProxy positive64BitIntegerFor: value]].<br>
^interpreterProxy pop: 4 thenPush: valueOop!<br></blockquote><div><br></div><div>This doesn't make sense to me. The lines</div><div><br></div><div> [valueOop := isSigned<br>+ ifTrue:[interpreterProxy signedMachineIntegerFor: value]<br>+ ifFalse:[interpreterProxy positiveMachineIntegerFor: value]].<br>- ifTrue:[interpreterProxy signed64BitIntegerFor: value]<br>- ifFalse:[interpreterProxy positive64BitIntegerFor: value]].<br></div><div><br></div><div>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.</div><div><br></div><div>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</div><div><br></div><div> ifFalse: "general 64 bit integer; note these never fail"<br> [(isSigned and: [byteSize = 4]) ifTrue: "sign-extend 32-bit results to 64-bits..."</div> [mask := 1 << (byteSize * 8 - 1).<br> value := (value bitAnd: mask-1) - (value bitAnd: mask)].<div> valueOop := isSigned<br> ifTrue:[interpreterProxy signed64BitIntegerFor: value]<br> ifFalse:[interpreterProxy positive64BitIntegerFor: value]].<br></div><div><br></div><div> </div></div><div>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 :-(.</div><div><br></div><div>I will test the above, and if it's right, commit.</div><div class="gmail_signature"><div dir="ltr"><div><span style="font-size:small;border-collapse:separate"><div>_,,,^..^,,,_<br></div><div>best, Eliot</div></span></div></div></div>
</div></div>