Mantis - Squeak
Viewing Issue Advanced Details
1415 System minor always 07-06-05 23:48 04-06-06 20:47
closed 3.8  
none 3.8  
0001415: [BUG] ByteArray>>unsignedShortAt:
Alain Fischer <>:

OS: Mac OS X 10.3.3
VM: unix - Squeak3.6beta of '4 July 2003' [latest update: #5411]
Image: Squeak3.6 [latest update: 0005424]

(ByteArray with: 255 with: 255) unsignedShortAt: 1
return -1, it seem to me that the correct result is 65535

This is still the behaviour in 3.8-6665 [^] (1,608 bytes) 09-15-05 12:14

07-06-05 23:48   
Ned Konz <>:

On Friday 02 April 2004 2:07 pm, Alain Fischer wrote:
> OS: Mac OS X 10.3.3
> VM: unix - Squeak3.6beta of '4 July 2003' [latest update: #5411]
> Image: Squeak3.6 [latest update: 0005424]
> (ByteArray with: 255 with: 255) unsignedShortAt: 1
> return -1, it seem to me that the correct result is 65535

Well, that would be consistent with:

    (ByteArray with: 255 with: 255) unsignedShortAt: 1 bigEndian: true

I believe the problem is in the FFIPlugin definition of primitiveFFIIntegerAt
where it says:

    byteSize < 4 ifTrue:[
        byteSize = 1
            ifTrue:[value _ interpreterProxy byteAt: addr]
            ifFalse:[ value _ self cCode: '*((short int *) addr)'
                                inSmalltalk: [interpreterProxy halfWordAt: addr]].
        isSigned ifTrue:["sign extend value"
            mask _ 1 << (byteSize * 8 - 1).
            value _ (value bitAnd: mask-1) - (value bitAnd: mask)].
        "note: byte/short never exceed SmallInteger range"
        value _ interpreterProxy integerObjectOf: value.

And of course value is an int, so the sign extension always happens.

I think it should read something like this instead:

    byteSize < 4 ifTrue:[
        byteSize = 1
            ifTrue:[value _ interpreterProxy byteAt: addr]
                isSigned ifTrue: [ "sign extend value"
                    value := self cCode: '*((short int *) addr)'
                    inSmalltalk: [
                        value := interpreterProxy halfWordAt: addr.
                        mask := 1 << (byteSize * 8 - 1).
                        value := (value bitAnd: mask-1) - (value bitAnd: mask) ]]
                ifFalse: [ "unsigned value"
                    value := self cCode: '(int)*((unsigned short int *) addr)'
                    inSmalltalk: [ value := interpreterProxy halfWordAt: addr ]]
        "note: byte/short never exceed SmallInteger range"
        value _ interpreterProxy integerObjectOf: value.

After all, there's no reason to do explicit sign extension in C.

Ned Konz [^]
09-14-05 23:30   
It seems reasonable to me that one would expect 65535 here.

However, why on earth is the integer/double/float-at/put code tied to the blasted ffi plugin? What on earth could possibly be the reason for that?
09-14-05 23:32   
so it's an issue. could we, should we move the prims to the core? I think so personally.
09-15-05 05:09   
Moving the prims to the core is impossible as you'd understand if you were able to answer the first question ("why on earth blablabla"). The prim is for dereferencing pointers (ExternalAddress) and just happens to work for ByteArray since it was convenient.
09-15-05 12:15   
The changeset I proposed just adds "unsigned" where it belongs. I am not in position to test it right now but I think it will do the right thing, as the sign extension takes place afterwards if needed.
09-16-05 03:22   
'Moving the prims to the core is impossible' - what nonsense.

The six prims in question use a single support routing in the ffi plugin that has anything to do with external structures/addresses/whatever. That routine simply compares the receiver class with classExternalAddress, an oop in the special objects array.

It took about 5 minutes to move the integer pair into the core vm, build a new vm, change the image to use the new named prims and test it.

Since the prims are used for ByteArray handling, something in the base image and only peripherally have anything to do with the optional ffi plugin I think they ought to be in the core. Not all platforms have an ffi plugin and so they get prim failures if the relevent methods are invoked. Even platforms that do have an ffi plugin available may have it not installed for security reasons since it allows arbitrary slash and burn.
09-16-05 03:50   
Tim, stop and think for a second. What is the point of moving these prims to the core? They are labeled as belonging to the FFI and the FFI requires the plugin. They contain severe security hazards (dereferencing anything that is an instance of the class stored in the ExternalAddress slot). They shouldn't be used anyway unless the contents of the byte array was created by a platform (ffi) call. There simply is no good reason for moving these prims to the core. There would be a point in writing prims for the platform >indendependent< accessors but not for the platform >dependent< accessors.
09-16-05 04:40   
Fine. So if these particular prims are tightly related to FFI stuff perhaps it would be better if they were implemented in that class. This would remove the temptation to use them for general byte array work and clearly separate them out to purely FFI-land. To have prims that are so dangerous and yet leave them available to ByteArray merely because it was convenient seems pretty silly.
Separating them out (one version purely for ExternalAddress, fail if not of that class, the other for ByteArray) would at least clean that up. And it would leave (ByteArray with:x with:y) unsignedShortAt: z doing something sensible instead of primfail even if ffiplugin was non loaded.

I propose adding 6 new bytearray prims and image changes to
a) specialise the externaladdress handlers to ExternalAddress,
b) add the new prims to ByteArray

Oh and some opinion on fixing the original problem - if it is agreed to be a problem? - would be useful.
nicolas cellier   
03-29-06 02:38   
I agree with tim's last proposition.

ByteArray unsignedShortAt: longAt:put: floatAt: doubleAt:put: etc... are perfectly safe (if guarded by preconditions).

They are usefull outside FFI: enable binary IO / foreign language data interoperability.
However, optional endianness argument would be usefull in this case.

I do not see why they should be FFI specific, apart historical reasons.
03-30-06 03:33   
one change set to make the unsignedShortAt: return something which is more likely expected, which address the original complaint
04-06-06 20:46   
Since there has been no further interest in this I'm including John's ultra minimalist fix for the next release.
04-06-06 20:47   
Fixed so far as I'm concerned. Included in VMMaker -tpr.55.mcz which is likely near a public release