[Vm-dev] bug when instantiating ExternalData (to return void* from FFI)
Esteban Lorenzano
estebanlm at gmail.com
Wed Aug 23 09:02:54 UTC 2017
> On 22 Aug 2017, at 22:34, Eliot Miranda <eliot.miranda at gmail.com> wrote:
>
>
>
> On Tue, Aug 22, 2017 at 3:47 AM, Esteban Lorenzano <estebanlm at gmail.com <mailto: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?
is ok for me :)
Esteban
>
> _,,,^..^,,,_
> best, Eliot
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20170823/3998582e/attachment.html>
More information about the Vm-dev
mailing list