Smalltalk › Squeak › Squeak VM

bug when instantiating ExternalData (to return void* from FFI)

‹ Previous Topic Next Topic ›
 
classic Classic list List threaded Threaded
♦
♦
 4 messages Options
EstebanLM
Reply | Threaded
Open this post in threaded view
♦
♦
|

bug when instantiating ExternalData (to return void* from FFI)

EstebanLM
 
Hi Eliot, list

I found a bug :)
With latest VM (32 bits, using PharoVM flavour), I crash the VM when executing this:

LGitError giterr_last.

LGitError class>>giterr_last
        ^ self call: #(void *giterr_last()) options: #(  )

this is a very basic FFI call, and should answer an ExternalData instance (pointing to NULL, in this case).

Digging into it, I found at a point (when instantiating the pointer) it answers nil instead an instantiated ExternalData, and this is because in

Spur32BitMemoryManager>>instantiateClass:indexableSize:

otherwise clause is:

                otherwise: "non-indexable"
                        [self cppIf: (PharoVM or: [true]) "Leave the old code but ignore it completely unless someone complains."
                                ifTrue:
                                        [^nil]
                                ifFalse:
                                        ["some Squeak images include funky fixed subclasses of abstract variable
                                          superclasses. e.g. DirectoryEntry as a subclass of ArrayedCollection.
                                          Allow fixed classes to be instantiated here iff nElements = 0."
                                         (nElements ~= 0 or: [instSpec > self lastPointerFormat]) ifTrue:
                                                [^nil].
                                         numSlots := self fixedFieldsOfClassFormat: classFormat.
                                         fillValue := nilObj]].

So it will always answer nil/NULL.
Now, I don’t know why it was commented out, but since there it says  "Leave the old code but ignore it completely unless someone complains.”, I complain :D

Thing is… if I restore old code it works fine.

Question is: should we restore that code? or this shows a deeper problem?

cheers,
Esteban
Eliot Miranda-2
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: bug when instantiating ExternalData (to return void* from FFI)

Eliot Miranda-2
 
Hi Esteban,

On Tue, Aug 22, 2017 at 3:47 AM, Esteban Lorenzano <[hidden email]> wrote:

Hi Eliot, list

I found a bug :)
With latest VM (32 bits, using PharoVM flavour), I crash the VM when executing this:

LGitError giterr_last.

LGitError class>>giterr_last
        ^ self call: #(void *giterr_last()) options: #(  )

this is a very basic FFI call, and should answer an ExternalData instance (pointing to NULL, in this case).

Digging into it, I found at a point (when instantiating the pointer) it answers nil instead an instantiated ExternalData, and this is because in

Spur32BitMemoryManager>>instantiateClass:indexableSize:

otherwise clause is:

                otherwise: "non-indexable"
                        [self cppIf: (PharoVM or: [true]) "Leave the old code but ignore it completely unless someone complains."
                                ifTrue:
                                        [^nil]
                                ifFalse:
                                        ["some Squeak images include funky fixed subclasses of abstract variable
                                          superclasses. e.g. DirectoryEntry as a subclass of ArrayedCollection.
                                          Allow fixed classes to be instantiated here iff nElements = 0."
                                         (nElements ~= 0 or: [instSpec > self lastPointerFormat]) ifTrue:
                                                [^nil].
                                         numSlots := self fixedFieldsOfClassFormat: classFormat.
                                         fillValue := nilObj]].

So it will always answer nil/NULL.
Now, I don’t know why it was commented out, but since there it says  "Leave the old code but ignore it completely unless someone complains.”, I complain :D

Thing is… if I restore old code it works fine.

Question is: should we restore that code? or this shows a deeper problem?

We can replace it with the ifFalse: branch.  I will do so.  I made this change because Nicolai Hess wrote to the Pharo mailing list.  I knew that Squeak relied on the behavior but not Pharo.  So 
cheers,
Esteban



--
_,,,^..^,,,_
best, Eliot
Eliot Miranda-2
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: bug when instantiating ExternalData (to return void* from FFI)

Eliot Miranda-2
In reply to this post by EstebanLM
 


On Tue, Aug 22, 2017 at 3:47 AM, Esteban Lorenzano <[hidden email]> wrote:

Hi Eliot, list

I found a bug :)
With latest VM (32 bits, using PharoVM flavour), I crash the VM when executing this:

LGitError giterr_last.

LGitError class>>giterr_last
        ^ self call: #(void *giterr_last()) options: #(  )

this is a very basic FFI call, and should answer an ExternalData instance (pointing to NULL, in this case).

Digging into it, I found at a point (when instantiating the pointer) it answers nil instead an instantiated ExternalData, and this is because in

Spur32BitMemoryManager>>instantiateClass:indexableSize:

otherwise clause is:

                otherwise: "non-indexable"
                        [self cppIf: (PharoVM or: [true]) "Leave the old code but ignore it completely unless someone complains."
                                ifTrue:
                                        [^nil]
                                ifFalse:
                                        ["some Squeak images include funky fixed subclasses of abstract variable
                                          superclasses. e.g. DirectoryEntry as a subclass of ArrayedCollection.
                                          Allow fixed classes to be instantiated here iff nElements = 0."
                                         (nElements ~= 0 or: [instSpec > self lastPointerFormat]) ifTrue:
                                                [^nil].
                                         numSlots := self fixedFieldsOfClassFormat: classFormat.
                                         fillValue := nilObj]].

So it will always answer nil/NULL.
Now, I don’t know why it was commented out, but since there it says  "Leave the old code but ignore it completely unless someone complains.”, I complain :D

Thing is… if I restore old code it works fine.

Question is: should we restore that code? or this shows a deeper problem?

OK, I didn't mean to hit send.  Ignore the previous posting.  I made this change because Nicolai Hess wrote to the Pharo mailing list.  I knew that Squeak relied on the behavior but not Pharo.  So I decided to keep the behavior for Squeak and make it illegal for Pharo.  Now we know that we know that the FFI depends on it.

Looking at the code the use is here:

ThreadedFFIPlugin>>ffiReturnPointer:ofType:in:
    ...
     self remapOop: oop in:
[retOop := interpreterProxy 
instantiateClass: interpreterProxy classExternalData 
indexableSize: 0].
    ...

Alas InterpreterProxy doesn't provide instantiateClass:, only instantiateClass:indexableSize:.  So the quick fix is to keep the old behavior (that new: 0 works for non-indexable classes) but the right fix is to change the ThreadedFFIPlugin and InterpreterProxy so we can write

    ...
     self remapOop: oop in:
[retOop := interpreterProxy instantiateClass: interpreterProxy classExternalData].
    ...

What do people think?

_,,,^..^,,,_
best, Eliot
EstebanLM
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: bug when instantiating ExternalData (to return void* from FFI)

EstebanLM
 

On 22 Aug 2017, at 22:34, Eliot Miranda <[hidden email]> wrote:



On Tue, Aug 22, 2017 at 3:47 AM, Esteban Lorenzano <[hidden email]> wrote:

Hi Eliot, list

I found a bug :)
With latest VM (32 bits, using PharoVM flavour), I crash the VM when executing this:

LGitError giterr_last.

LGitError class>>giterr_last
        ^ self call: #(void *giterr_last()) options: #(  )

this is a very basic FFI call, and should answer an ExternalData instance (pointing to NULL, in this case).

Digging into it, I found at a point (when instantiating the pointer) it answers nil instead an instantiated ExternalData, and this is because in

Spur32BitMemoryManager>>instantiateClass:indexableSize:

otherwise clause is:

                otherwise: "non-indexable"
                        [self cppIf: (PharoVM or: [true]) "Leave the old code but ignore it completely unless someone complains."
                                ifTrue:
                                        [^nil]
                                ifFalse:
                                        ["some Squeak images include funky fixed subclasses of abstract variable
                                          superclasses. e.g. DirectoryEntry as a subclass of ArrayedCollection.
                                          Allow fixed classes to be instantiated here iff nElements = 0."
                                         (nElements ~= 0 or: [instSpec > self lastPointerFormat]) ifTrue:
                                                [^nil].
                                         numSlots := self fixedFieldsOfClassFormat: classFormat.
                                         fillValue := nilObj]].

So it will always answer nil/NULL.
Now, I don’t know why it was commented out, but since there it says  "Leave the old code but ignore it completely unless someone complains.”, I complain :D

Thing is… if I restore old code it works fine.

Question is: should we restore that code? or this shows a deeper problem?

OK, I didn't mean to hit send.  Ignore the previous posting.  I made this change because Nicolai Hess wrote to the Pharo mailing list.  I knew that Squeak relied on the behavior but not Pharo.  So I decided to keep the behavior for Squeak and make it illegal for Pharo.  Now we know that we know that the FFI depends on it.

Looking at the code the use is here:

ThreadedFFIPlugin>>ffiReturnPointer:ofType:in:
    ...
     self remapOop: oop in:
[retOop := interpreterProxy 
instantiateClass: interpreterProxy classExternalData 
indexableSize: 0].
    ...

Alas InterpreterProxy doesn't provide instantiateClass:, only instantiateClass:indexableSize:.  So the quick fix is to keep the old behavior (that new: 0 works for non-indexable classes) but the right fix is to change the ThreadedFFIPlugin and InterpreterProxy so we can write

    ...
     self remapOop: oop in:
[retOop := interpreterProxy instantiateClass: interpreterProxy classExternalData].
    ...

What do people think?

is ok for me :)

Esteban


_,,,^..^,,,_
best, Eliot
Free forum by Nabble Edit this page