Mantis Bugtracker
  

Viewing Issue Simple Details Jump to Notes ] View Advanced ] Issue History ] Print ]
ID Category Severity Reproducibility Date Submitted Last Update
0001415 [Squeak] System minor always 07-06-05 23:48 04-06-06 20:47
Reporter masm View Status public  
Assigned To tim
Priority normal Resolution fixed  
Status closed   Product Version 3.8
Summary 0001415: [BUG] ByteArray>>unsignedShortAt:
Description Alain Fischer <mailinglist.fischer@bluewin.ch>:

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

Alain
Additional Information This is still the behaviour in 3.8-6665
Attached Files  FFIPlugin-primitiveFFIIntegerAt.st [^] (1,608 bytes) 09-15-05 12:14

- Relationships

- Notes
(0001716 - 1961 - 4641 - 4773 - 4856 - 4856 - 4856)
masm
07-06-05 23:48

Ned Konz <ned@bike-nomad.com>:

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:[
        "short/byte"
        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:[
        "short/byte"
        byteSize = 1
            ifTrue:[value _ interpreterProxy byteAt: addr]
            ifFalse:[
                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
http://bike-nomad.com [^]
GPG key ID: BEEA7EFE
 
(0002650 - 210 - 222 - 222 - 222 - 222 - 222)
tim
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?
 
(0002651 - 88 - 88 - 88 - 88 - 88 - 88)
tim
09-14-05 23:32

so it's an issue. could we, should we move the prims to the core? I think so personally.
 
(0002655 - 257 - 267 - 267 - 267 - 267 - 267)
andreas
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.
 
(0002658 - 201 - 211 - 211 - 211 - 211 - 211)
sam
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.
 
(0002672 - 895 - 931 - 931 - 931 - 931 - 931)
tim
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.
 
(0002674 - 596 - 608 - 608 - 608 - 608 - 608)
andreas
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.
 
(0002675 - 947 - 989 - 989 - 989 - 989 - 989)
tim
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.
 
(0004604 - 396 - 438 - 438 - 438 - 438 - 438)
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.
 
(0004626 - 128 - 128 - 128 - 128 - 128 - 128)
johnmci
03-30-06 03:33

one change set to make the unsignedShortAt: return something which is more likely expected, which address the original complaint
 
(0004691 - 112 - 112 - 112 - 112 - 112 - 112)
tim
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.
 
(0004692 - 100 - 100 - 100 - 100 - 100 - 100)
tim
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
 

- Issue History
Date Modified Username Field Change
07-06-05 23:48 masm New Issue
07-06-05 23:48 masm Note Added: 0001716
07-23-05 02:24 tim Status new => assigned
07-23-05 02:24 tim Assigned To  => tim
09-14-05 23:30 tim Note Added: 0002650
09-14-05 23:32 tim Note Added: 0002651
09-14-05 23:32 tim Status assigned => acknowledged
09-14-05 23:33 tim Assigned To tim =>
09-15-05 05:09 andreas Note Added: 0002655
09-15-05 12:12 sam Note Added: 0002657
09-15-05 12:14 sam Note Deleted: 0002657
09-15-05 12:14 sam File Added: FFIPlugin-primitiveFFIIntegerAt.st
09-15-05 12:14 sam Issue Monitored: sam
09-15-05 12:15 sam Note Added: 0002658
09-16-05 03:22 tim Note Added: 0002672
09-16-05 03:50 andreas Note Added: 0002674
09-16-05 04:40 tim Note Added: 0002675
10-20-05 00:58 tim Status acknowledged => assigned
10-20-05 00:58 tim Assigned To  => tim
03-29-06 02:38 nicolas cellier Note Added: 0004604
03-30-06 03:33 johnmci Note Added: 0004626
04-06-06 20:46 tim Note Added: 0004691
04-06-06 20:47 tim Status assigned => resolved
04-06-06 20:47 tim Fixed in Version  => 3.8
04-06-06 20:47 tim Resolution open => fixed
04-06-06 20:47 tim Note Added: 0004692
04-06-06 20:47 tim Status resolved => closed


Mantis 1.0.8[^]
Copyright © 2000 - 2007 Mantis Group
102 total queries executed.
60 unique queries executed.
Powered by Mantis Bugtracker