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

[David T. Lewis]

On Fri, Jan 06, 2012 at 09:27:57AM +0100, Stefan Marr wrote:
> 
> Hi Dave:
> 
> Ok, I tell you that there might be an edge case in your code, and you are renaming the helper? ;)

Hi Stefan,

Thanks for reviewing. No, I am not going to rename the helper, but please
have a look at Eliot's implementation in oscog, which is using a much
better selector name for the method that replaces this. Whenever the
method name changes it should be changed to use Eliot's version.

> 
> On 05 Jan 2012, at 14:51, David T. Lewis wrote:
> 
> >> Item was changed:
> >>  ----- Method: Interpreter>>primitivePerformInSuperclass (in category 'control primitives') -----
> >>  primitivePerformInSuperclass
> >>  	| lookupClass rcvr currentClass |
> >>  	lookupClass := self stackTop.
> >> + 	rcvr := self stackValue: 3.
> >> - 	rcvr := self stackValue: argumentCount.
> >>  	currentClass := self fetchClassOf: rcvr.
> >>  	[currentClass ~= lookupClass]
> >>  		whileTrue:
> >>  		[currentClass := self superclassOf: currentClass.
> >>  		currentClass = nilObj ifTrue: [^ self primitiveFail]].
> >> 
> >> + 	argumentCount = 3
> >> + 		ifTrue: ["normal primitive call with 3 arguments expected on the stack"
> >> + 			self popStack.
> >> + 			self primitivePerformAt: lookupClass.
> >> + 			self successful ifFalse:
> >> + 				[self push: lookupClass]]
> 
> This looks correct, that is just the old code I assume.
 
Yes
 
> >> + 		ifFalse: ["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"
> 
> Could it be that if somebody like me, calls the primitive without understanding it, that there are less than 3 items on the stack? What would happen then?

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.

> You restore the stack on failure, but I guess the at least the stack pointer could be wrong after the restore.
> 

I think it is OK, and the tests pass (including check for stack size), but
this is exactly the sort of problem I am concerned about so let me know
if you see any ways for it to go wrong :)

Dave