Hi All,
Andreas, vm-devs this is a question of y'all. Recently I integrated some of the merge of Alien callback support into ReentrantFFIPlugin. This broke OpenGL example in images not containing Alien. Here's the breakage:
As part of Alien marshalling the FFI plugin needs to know if an argument is an Alien and hence dereference it correctly to pass its address. Here's the code in ReentrantFFIPlugin >>ffiAtomicArgByReference:Class:in:
isAlien := (isString := interpreterProxy includesBehavior: oopClass ThatOf: interpreterProxy classString) ifTrue: [false] ifFalse: [interpreterProxy includesBehavior: oopClass ThatOf: interpreterProxy classAlien].
This is erroneously setting isAlien to true when oopClass is Bitmap (i.e. when uploadFont: tries to glBitmap:with:with:with:with:with:with: a Bitmap). That causes the marshaller to treat the Bitmap as if it were an Alien and pass a field in the Bitmap instead of the Bitmap's base address. Boom.
The problem is that a) if Alien isn't installed in the image then interpreterProxy classAlien == interpreterProxy nilObject, and b) includesBehavior:ThatOf: always answers true if its second argument is nilObject.
I suggest that includesBehavior:ThatOf: is incorrect:
StackInterpreter methods for plugin primitive support includesBehavior: aClass ThatOf: aSuperclass "Return the equivalent of aClass includesBehavior: aSuperclass. Note: written for efficiency and better inlining (only 1 temp)" | theClass | <inline: true> (((theClass := aClass) = aSuperclass) "aClass == aSuperclass" or:[aSuperclass = objectMemory nilObject]) "every class inherits from nil" ifTrue:[^true]. [(theClass := self superclassOf: theClass) = aSuperclass ifTrue:[^true]. theClass ~= objectMemory nilObject] whileTrue. ^false
Classes *don't* inherit from nil. nil is at the end of their superclass chain. That's different from inheriting.
At least in current images UndefinedObject includesBehavior: nil is false, which seems right to me. nil is not a behavior; its use as the sentinel at the end of the superclass chain doesn't imply it is a behavior. So I propose that we change includesBehavior:ThatOf: to something like
StackInterpreter methods for plugin primitive support includesBehavior: aClass ThatOf: aSuperclass "Return the equivalent of aClass includesBehavior: aSuperclass. Note: written for efficiency and better inlining (only 1 temp)" | theClass | <inline: true> aSuperclass = objectMemory nilObject ifTrue: [^false]. theClass := aClass. [theClass = aSuperclass ifTrue: [^true]. theClass ~= objectMemory nilObject] whileTrue: [theClass := self superclassOf: theClass]. ^false
I don't think this will affect anything other than FFI and Alien since those are the only uses I can find, and in my reading of that code the proposed change seems fine; safer in fact.
Agreed?
On Thu, Mar 17, 2011 at 12:30 PM, Matthew Fulmer tapplek@gmail.com wrote:
On Thu, Mar 17, 2011 at 11:23:57AM -0700, Andreas Raab wrote:
On 3/17/2011 10:59, Lawson English wrote:
The biggest holdup for getting these things into the Squeak trunk is simply getting Cobalt itself into the Squeak trunk so it can run on VM 4.2. If anyone wants to help Matt with THAT project, I'm pretty sure he will be happy for the assist.
What do you need help with?
The holdup is the VM's actually:
- Recent Cog VM's crash on startup from OpenGL/FFI
- Windows interpreter VM crashes on startup from JPEGPlugin
- Unix interpreter VM has broken MPEGPlugin
- Mac VM hasn't been extensively tested; John's latest may work,
but Esteban's is missing plugins
Old Cog VM's (2316) almost work, but:
- Windows VM is missing SoundPlugin
- At least on windows, the router sockets timeout after they've
been running for 30 minutes or so
There is very little that needs doing image-side; I've already done that part. Since we have a zero-regression policy for the official Cobalt releases, we can't release until we have working VM's. None of us are VM devs, and we have plenty to work on while we wait, so we're just waiting for the VM guys right now.
-- Matthew Fulmer (a.k.a. Tapple)
On 3/30/2011 4:12, Eliot Miranda wrote:
Classes *don't* inherit from nil. nil is at the end of their superclass chain. That's different from inheriting.
At least in current images UndefinedObject includesBehavior: nil is false, which seems right to me. nil is not a behavior; its use as the sentinel at the end of the superclass chain doesn't imply it is a behavior. So I propose that we change includesBehavior:ThatOf: to something like
StackInterpreter methods for plugin primitive support includesBehavior: aClass ThatOf: aSuperclass "Return the equivalent of aClass includesBehavior: aSuperclass. Note: written for efficiency and better inlining (only 1 temp)" | theClass | <inline: true> aSuperclass = objectMemory nilObject ifTrue: [^false]. theClass := aClass. [theClass = aSuperclass ifTrue: [^true]. theClass ~= objectMemory nilObject] whileTrue: [theClass := self superclassOf: theClass]. ^false
I don't think this will affect anything other than FFI and Alien since those are the only uses I can find, and in my reading of that code the proposed change seems fine; safer in fact.
Agreed?
Sounds reasonable to me. The one thing to check is if there are any places that currently assume that includesBehavior:ThatOf: returns true for a nil argument. If so, we can still add an explicit check that tests for interpreterProxy classAlien to be nil but I agree that the above would be the better solution.
Cheers, - Andreas
On 30 March 2011 10:01, Andreas Raab andreas.raab@gmx.de wrote:
On 3/30/2011 4:12, Eliot Miranda wrote:
Classes *don't* inherit from nil. nil is at the end of their superclass chain. That's different from inheriting. At least in current images UndefinedObject includesBehavior: nil is false, which seems right to me. nil is not a behavior; its use as the sentinel at the end of the superclass chain doesn't imply it is a behavior. So I propose that we change includesBehavior:ThatOf: to something like StackInterpreter methods for plugin primitive support includesBehavior: aClass ThatOf: aSuperclass "Return the equivalent of aClass includesBehavior: aSuperclass. Note: written for efficiency and better inlining (only 1 temp)" | theClass | <inline: true> aSuperclass = objectMemory nilObject ifTrue: [^false]. theClass := aClass. [theClass = aSuperclass ifTrue: [^true]. theClass ~= objectMemory nilObject] whileTrue: [theClass := self superclassOf: theClass]. ^false I don't think this will affect anything other than FFI and Alien since those are the only uses I can find, and in my reading of that code the proposed change seems fine; safer in fact. Agreed?
Sounds reasonable to me. The one thing to check is if there are any places that currently assume that includesBehavior:ThatOf: returns true for a nil argument. If so, we can still add an explicit check that tests for interpreterProxy classAlien to be nil but I agree that the above would be the better solution.
Hmm.. i'd prefer to not see this in marshalling code. But its easier to say than to do.
I did the following trick in NativeBoost:
- added own GC root - an array which serves as a registry for all classes & other tricky stuff, like callbacks.
To test if argument belongs to specific class, i putting the class to that array and then code generator emits the code to load class oop from that array at specific index (which i have obtained during registration), and compares that class oop with class of argument which needs to be marshalled. (For curious, the marshalling implementation is in NBExternalObjectType class).
In Alien, one could do a similar trick. For signature like:
void * foo (MyAlienClass arg1 , MyAlienClass2 arg2)
one could: - put the MyAlienClass and MyAlienClass2 into signature descriptor (or somewhere else), which will tell that during marshalling, a passed argument's class must be exactly same. Then you can avoid slow and cumbersome membership testing, but just test oop's class.
As to me, using inheritance to check argument type is slow and not guarantees correct coercion. For example, all external structures is subclasses of NBExternalStructure, but obviously if you pass an instance of one struct type, say StructA to function which expects StructB. In this case, a membership testing gives nothing.
Cheers, - Andreas
I have built new Unix VM with it, and now running
OpenGL example
works fine (on my virtual box linux, it shows rotating rectangle at 157 fps rate :))
Eliot, i can integrate this fix into VMMaker-oscog branch and upload new VMMaker version.
Should i do that now, or you have more changes coming?
vm-dev@lists.squeakfoundation.org
-
Andreas Raab -
Eliot Miranda -
Igor Stasenko