[Vm-dev] Request for review of VMMaker-dtl.262.mcz

David T. Lewis lewis at mail.msen.com
Sat Jan 7 21:36:10 UTC 2012

On Fri, Jan 06, 2012 at 08:53:21PM +0100, Stefan Marr wrote:
> On 06 Jan 2012, at 17:33, David T. Lewis wrote:
> > 
> > Thanks, this is a good idea to add an explicit check for invalid
> > argument count, and it would cost nothing in performance. I'll
> > wait for any further feedback on this, and include add the check
> > in a future update.
> Why not just change the current test around?
> I see a conceptual problem with that test anyway.
> By testing for 3 arguments, you are pulling an invariant out of
> primitivePerformAt: and check it upfront. That seems to me like a
> 'maintainability' issue.
> While if you would check for 5 arguments and do the mirror thing in the
> success case, and the old stuff in the else branch, you would get the old
> error handling behavior and would only test an invariant local to
> primitivePerformInSuperclass.

I do not think that I understand what you are saying. There are only two
usages of this primitive in the image. The first is
Object>>perform:withArguments:inSuperclass: which is the original usage
that calls the primitive with three arguments on the stack. The second is
ContextPart>>object:perform:withArguments:inClass: which is a recent
addition that calls the primitive with four arguments. The change to the
primitive is intended to support this second usage. Eliot has indicated
that this is important for improving the debugger and that it would be
good to have the support in place in the standard VM as well as in Cog,
so that is all that I am trying to do.

I am attaching Interpreter-primitivePerformInSuperclass.st that implements
your suggestion of failing the primitive on invalid argument count, and
also uses primitiveFailFor: <reason> rather than just primitiveFail. I
won't update VMMaker with this until I've had a chance to hear any feedback
from Eliot, who might very well tell us that this whole approach is an
awful bodge, in which case we are just polishing the turd here ;-)

-------------- next part --------------
'From Squeak3.11alpha of 5 January 2012 [latest update: #11869] on 7 January 2012 at 4:23:34 pm'!

!Interpreter methodsFor: 'control primitives' stamp: 'dtl 1/6/2012 20:22'!
	| lookupClass rcvr currentClass |
	lookupClass := self stackTop.
	rcvr := self stackValue: 3.
	currentClass := self fetchClassOf: rcvr.
	[currentClass ~= lookupClass]
		[currentClass := self superclassOf: currentClass.
		currentClass = nilObj ifTrue: [^self primitiveFailFor: PrimErrBadArgument]].

	argumentCount = 3
		ifTrue: ["normal primitive call with 3 arguments expected on the stack"
			self popStack.
			self primitivePerformAt: lookupClass.
			self successful ifFalse:
				[self push: lookupClass]]
		ifFalse: [argumentCount = 4
			ifTrue: ["mirror primitive call with extra argument specifying object to serve as receiver"
				| s1 s2 s3 s4 s5 |
				"save stack contents"
				s1 := self popStack. "lookupClass"
				s2 := self popStack. "args"
				s3 := self popStack. "selector"
				s4 := self popStack. "mirror receiver"
				s5 := self popStack. "actual receiver"
				"slide stack up one, omitting the actual receiver parameter"
				self push: s4. "mirror receiver"
				self push: s3. "selector"
				self push: s2. "args"
				"perform as if mirror receiver had been the actual receiver"
				self primitivePerformAt: lookupClass.
				self successful ifFalse:
					["restore original stack"
					self pop: 3. "args, selector, mirror receiver"
					self push: s5. "actual receiver"
					self push: s4. "mirror receiver"				
					self push: s3. "selector"
					self push: s2. "args"
					self push: s1. "lookup class" ]]
			ifFalse: ["wrong number of arguments"
				^self primitiveFailFor: PrimErrBadNumArgs]]
! !

More information about the Vm-dev mailing list