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

[Esteban Lorenzano]

> 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>