An issue with Collections-ul.667

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

An issue with Collections-ul.667

Eliot Miranda-2
Hi Levente,

    I just wanted to note a potential problem with Collections-ul.667

Name: Collections-ul.667
Author: ul
Time: 10 October 2015, 3:40:46.732 pm
UUID: 7cd785ff-1839-4075-ac56-4b71054529d8
Ancestors: Collections-topa.666

Added the missing methods of ByteArray's platform independent access category: #signedByteAt: #signedByteAt:put: #long64At:put:bigEndian: and #long64At:bigEndian:. The 64-bit methods are not optimized.


In this version you provided ByteArray>>#signedByteAt:[put:] which conflicts with FFI-Kernel's methods of the same name.  There's a difference in semantics.  If you look at ExternalStructure's class comment it points out that

"This class provides an abstract base for all structures that can be used by external functions. ExternalStructures have two possible handle types:
- ExternalAddress
If the handle is an external address then the object described does not reside in the Smalltalk object memory.
- ByteArray
If the handle is a byte array then the object described resides in Smalltalk memory.
Useful methods should be implemented by subclasses of ExternalStructure using the common ByteArray/ExternalAddress platform dependent access protocol which will transparently access the correct memory location."

i.e. the primitives used automatically differentiate between an ExternalAddress holding a pointer to external memory, in which case the datum is accessed via that location, or a ByteArray, in which case the datum is accessed in the ByteArray.  Your versions only do the latter and would hence misinterpret data in ExternalAdderss instances.  Here's the FFI code:

ByteArray>>signedByteAt: byteOffset
"Return a 8bit signed integer starting at the given byte offset"
^self integerAt: byteOffset size: 1 signed: true

ByteArray>>integerAt: byteOffset size: nBytes signed: aBoolean
"Primitive. Return an integer of nBytes size from the receiver.
Note: This primitive will access memory in the outer space if
invoked from ExternalAddress."
<primitive: 'primitiveFFIIntegerAt' module:'SqueakFFIPrims'>
^self primitiveFailed

And here's yours:
ByteArray>>signedByteAt: index
"Return an 8-bit signed integer quantity from the given byte index."
| byte |
(byte := self at: index) <= 16r7F ifTrue: [ ^byte ].
^byte - 16r100

Now of course this isn't an issue in builds that load the FFI after Collections, but it does affect images in which FFI-Kernel was loaded and then the image was updated to instal Collections-ul.667.

I'm not sure what the right fix is, but I thought I'd better mention it.  One obvious one is to implement the FFI methods in ExternalAddress>>#signedByteAt:[put:], and if folks agree I can do this.  Let me know.  Pharoers, if I do this, you'll need to make sure that the Pharo base includes ByteArray>>#signedByteAt:[put:].

_,,,^..^,,,_
best, Eliot


cbc
Reply | Threaded
Open this post in threaded view
|

Re: An issue with Collections-ul.667

cbc
That sounds like a decent compromise.  I would strongly request that a ByteArray>>signedByteAt:(put:) exist in the base image. The lack was why it was added - and it is useful for add-on packages such as my Kafka one. 

Thanks,
CNBC

Sent from my iPhone

On Feb 16, 2016, at 1:41 PM, Eliot Miranda <[hidden email]> wrote:

Hi Levente,

    I just wanted to note a potential problem with Collections-ul.667

Name: Collections-ul.667
Author: ul
Time: 10 October 2015, 3:40:46.732 pm
UUID: 7cd785ff-1839-4075-ac56-4b71054529d8
Ancestors: Collections-topa.666

Added the missing methods of ByteArray's platform independent access category: #signedByteAt: #signedByteAt:put: #long64At:put:bigEndian: and #long64At:bigEndian:. The 64-bit methods are not optimized.


In this version you provided ByteArray>>#signedByteAt:[put:] which conflicts with FFI-Kernel's methods of the same name.  There's a difference in semantics.  If you look at ExternalStructure's class comment it points out that

"This class provides an abstract base for all structures that can be used by external functions. ExternalStructures have two possible handle types:
- ExternalAddress
If the handle is an external address then the object described does not reside in the Smalltalk object memory.
- ByteArray
If the handle is a byte array then the object described resides in Smalltalk memory.
Useful methods should be implemented by subclasses of ExternalStructure using the common ByteArray/ExternalAddress platform dependent access protocol which will transparently access the correct memory location."

i.e. the primitives used automatically differentiate between an ExternalAddress holding a pointer to external memory, in which case the datum is accessed via that location, or a ByteArray, in which case the datum is accessed in the ByteArray.  Your versions only do the latter and would hence misinterpret data in ExternalAdderss instances.  Here's the FFI code:

ByteArray>>signedByteAt: byteOffset
"Return a 8bit signed integer starting at the given byte offset"
^self integerAt: byteOffset size: 1 signed: true

ByteArray>>integerAt: byteOffset size: nBytes signed: aBoolean
"Primitive. Return an integer of nBytes size from the receiver.
Note: This primitive will access memory in the outer space if
invoked from ExternalAddress."
<primitive: 'primitiveFFIIntegerAt' module:'SqueakFFIPrims'>
^self primitiveFailed

And here's yours:
ByteArray>>signedByteAt: index
"Return an 8-bit signed integer quantity from the given byte index."
| byte |
(byte := self at: index) <= 16r7F ifTrue: [ ^byte ].
^byte - 16r100

Now of course this isn't an issue in builds that load the FFI after Collections, but it does affect images in which FFI-Kernel was loaded and then the image was updated to instal Collections-ul.667.

I'm not sure what the right fix is, but I thought I'd better mention it.  One obvious one is to implement the FFI methods in ExternalAddress>>#signedByteAt:[put:], and if folks agree I can do this.  Let me know.  Pharoers, if I do this, you'll need to make sure that the Pharo base includes ByteArray>>#signedByteAt:[put:].

_,,,^..^,,,_
best, Eliot



Reply | Threaded
Open this post in threaded view
|

Re: An issue with Collections-ul.667

Levente Uzonyi
In reply to this post by Eliot Miranda-2
Hi Eliot,

If we want to keep the methods in the image, then I see two possibilities:
- One of them is, as you suggested, to add the methods to ExternalAddress.
This is a lot cleaner than relying on inheritance, and would also avoid
the updating issue you mentioned. It's fully backwards-compatible as well
- with these changes FFI can still be used from older images.
- Another option would be to move the primitive from the FFI plugin to the
VM. I didn't check how the primitive works, but I assume that it uses the
external objects array to get known of ExternalAddress, so I think this
is also possible. This way we could use the primitive from the image
side code as well, which should be faster especially for larger numbers.

Btw, there are other semantic differences between the two implementations.
The primitive is always strict about the accepted range of input values,
but the image side code is more lenient about under- and overflows, which
allows some tricks. E.g.:

  (ByteArray new: 1) signedByteAt: 1 put: 255; signedByteAt: 1

returns -1 using the code in Trunk, but it will fail with the primitive.

Levente

On Tue, 16 Feb 2016, Eliot Miranda wrote:

> Hi Levente,
>
>     I just wanted to note a potential problem with Collections-ul.667
>
> Name: Collections-ul.667
> Author: ul
> Time: 10 October 2015, 3:40:46.732 pm
> UUID: 7cd785ff-1839-4075-ac56-4b71054529d8
> Ancestors: Collections-topa.666
>
> Added the missing methods of ByteArray's platform independent access category: #signedByteAt: #signedByteAt:put: #long64At:put:bigEndian: and #long64At:bigEndian:. The 64-bit methods are not
> optimized.
>
>
> In this version you provided ByteArray>>#signedByteAt:[put:] which conflicts with FFI-Kernel's methods of the same name.  There's a difference in semantics.  If you look at
> ExternalStructure's class comment it points out that
>
> "This class provides an abstract base for all structures that can be used by external functions. ExternalStructures have two possible handle types:
> - ExternalAddress
> If the handle is an external address then the object described does not reside in the Smalltalk object memory.
> - ByteArray
> If the handle is a byte array then the object described resides in Smalltalk memory.
> Useful methods should be implemented by subclasses of ExternalStructure using the common ByteArray/ExternalAddress platform dependent access protocol which will transparently access the
> correct memory location."
>
> i.e. the primitives used automatically differentiate between an ExternalAddress holding a pointer to external memory, in which case the datum is accessed via that location, or a ByteArray,
> in which case the datum is accessed in the ByteArray.  Your versions only do the latter and would hence misinterpret data in ExternalAdderss instances.  Here's the FFI code:
>
> ByteArray>>signedByteAt: byteOffset
> "Return a 8bit signed integer starting at the given byte offset"
> ^self integerAt: byteOffset size: 1 signed: true
>
> ByteArray>>integerAt: byteOffset size: nBytes signed: aBoolean
> "Primitive. Return an integer of nBytes size from the receiver.
> Note: This primitive will access memory in the outer space if
> invoked from ExternalAddress."
> <primitive: 'primitiveFFIIntegerAt' module:'SqueakFFIPrims'>
> ^self primitiveFailed
>
> And here's yours:
> ByteArray>>signedByteAt: index
> "Return an 8-bit signed integer quantity from the given byte index."
> | byte |
> (byte := self at: index) <= 16r7F ifTrue: [ ^byte ].
> ^byte - 16r100
>
> Now of course this isn't an issue in builds that load the FFI after Collections, but it does affect images in which FFI-Kernel was loaded and then the image was updated to
> instal Collections-ul.667.
>
> I'm not sure what the right fix is, but I thought I'd better mention it.  One obvious one is to implement the FFI methods in ExternalAddress>>#signedByteAt:[put:], and if folks agree I can
> do this.  Let me know.  Pharoers, if I do this, you'll need to make sure that the Pharo base includes ByteArray>>#signedByteAt:[put:].
>
> _,,,^..^,,,_
> best, Eliot
>
>