[Vm-dev] bug when instantiating ExternalData (to return void* from FFI)

[Eliot Miranda]

On Tue, Aug 22, 2017 at 3:47 AM, Esteban Lorenzano <estebanlm at gmail.com>
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
<http://lists.pharo.org/pipermail/pharo-dev_lists.pharo.org/2017-August/130685.html>.
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20170822/e13d7bc6/attachment.html>