Smalltalk › Squeak › Squeak VM

Re: [squeak-dev] Re: Separate TeaTime (was Re: Re: Teleplace is hiring...)

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

2 messages Options

Eliot Miranda-2

Re: [squeak-dev] Re: Separate TeaTime (was Re: Re: Teleplace is hiring...)

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 <[hidden email]> 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)

Andreas.Raab

Re: [squeak-dev] Re: Separate TeaTime (was Re: Re: Teleplace is hiring...)

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