Yes, this is related to the discussion of primitivePerform and primitivePerformInSuperclass we had 8th to 12th December.

Just to clarify, the patch here is for the standard interpreter VM, and it is my attempt to implement a functional equivalent to the existing Cog implementation. The current Cog implementation is correct AFIK and this patch is intended to do the same thing for the interpreter VM, albeit in a slightly different way.

With respect to the arguments passed to the primitive on the stack, you can see (I hope) what they should be by looking at the patch below, since I put the stack operations in this one method. In "normal" use, there are three arguments on the stack, and when invoked as a mirror primitive, there is a fourth argument the specifies the object being mirrored.

Dave

On Thu, Jan 05, 2012 at 03:18:58PM +0100, Stefan Marr wrote:

Hi David, Hi Eliot (this might related to my problem from yesterday):

Do you remember the discussion of primitivePerform and primitivePerformInSuperclass we had 8th to 12th December?

One implicit assumption, based on the implementation, I made was that I do not have to give an array of arguments to the primitives. It would just take the arguments from the stack and at least to my understanding, would do the right thing.

Your new implementation does not seem to do that anymore.

I am not sure whether that is the problem I am seeing with Cog right now (reported yesterday). But your implementation looks like it breaks my code :(

What is the correct semantics for primitivePerform and primitivePerformInSuperclass with regard to arguments? Is it really not supported to provide them on the stack?

Thanks Stefan

On 05 Jan 2012, at 14:51, David T. Lewis wrote:

...

...

Reference Mantis 7429: Add Mirror Primitives to the VM

Update primitivePerformInSuperclass to support ContextPart>>object:perform:withArguments:inClass:

Implementation differs from that of oscog in that the original primitivePerformAt: is retained unmodified, and the necessary stack adjustments are done in primitivePerformInSuperclass for the special case of argumentCount 4 (mirror primitive call) rather than 3. The oscog approach may be prefered (not least for its clearer method naming), but making the change in primitivePerformInSuperclass is low risk and more easily implemented by a Sunday Squeaker.

All MirrorPrimitiveTests pass.

=============== Diff against VMMaker-dtl.261 ===============

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

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

• 	"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" ]]
  

• !

• self popStack.
• self primitivePerformAt: lookupClass.
• self successful ifFalse:

• [self push: lookupClass]!
  
  

Item was changed: ----- Method: VMMaker class>>versionString (in category 'version testing') ----- versionString

"VMMaker versionString"

• ^'4.7.19'!

• ^'4.7.18'!

-- Stefan Marr Software Languages Lab Vrije Universiteit Brussel Pleinlaan 2 / B-1050 Brussels / Belgium http://soft.vub.ac.be/~smarr Phone: +32 2 629 2974 Fax: +32 2 629 3525

On Thu, Jan 05, 2012 at 03:56:35PM +0100, Stefan Marr wrote:

Hi Dave:

On 05 Jan 2012, at 15:31, David T. Lewis wrote:

...

With respect to the arguments passed to the primitive on the stack, you can see (I hope) what they should be by looking at the patch below, since I put the stack operations in this one method. In "normal" use, there are three arguments on the stack, and when invoked as a mirror primitive, there is a fourth argument the specifies the object being mirrored.

Well, ok. I see now where my assumption was wrong. I thought #primitivePerformInSuperclass was implemented in terms of #primitivePerform. But apparently it is #primitivePerformAt: (being the common part of #primitivePerformInSuperclass and #primitivePerformWithArgs).

*sigh*

While you are at it, could you improve the naming of #primitivePerformAt: and rename it to #primitivePerformWithArgsAt:?

That is certainly the name it should have had. And since it is only a helper function as far as I can tell it should not cause to much problems.

Thanks Stefan

At this point I prefer not to change anything in this area unless there is a specific reason to do so. My first priority is to not break anything, second priority is to make the mirror primitives work, third priority is to make it consistent with Cog where I can, and last priority would be cosmetic changes or method renaming. Better method comments would not be out of the question though.

So if I sound overly conservative here, but we are dealing with a part of the VM that was designed and implemented by Dan Ingalls, and I am just an amateur VM hacker trying not to break it ;-)

Cheers, Dave

...

Dave

On Thu, Jan 05, 2012 at 03:18:58PM +0100, Stefan Marr wrote:

...

Hi David, Hi Eliot (this might related to my problem from yesterday):

Do you remember the discussion of primitivePerform and primitivePerformInSuperclass we had 8th to 12th December?

One implicit assumption, based on the implementation, I made was that I do not have to give an array of arguments to the primitives. It would just take the arguments from the stack and at least to my understanding, would do the right thing.

Your new implementation does not seem to do that anymore.

I am not sure whether that is the problem I am seeing with Cog right now (reported yesterday). But your implementation looks like it breaks my code :(

What is the correct semantics for primitivePerform and primitivePerformInSuperclass with regard to arguments? Is it really not supported to provide them on the stack?

Thanks Stefan

On 05 Jan 2012, at 14:51, David T. Lewis wrote:

...

...

Reference Mantis 7429: Add Mirror Primitives to the VM

Update primitivePerformInSuperclass to support ContextPart>>object:perform:withArguments:inClass:

Implementation differs from that of oscog in that the original primitivePerformAt: is retained unmodified, and the necessary stack adjustments are done in primitivePerformInSuperclass for the special case of argumentCount 4 (mirror primitive call) rather than 3. The oscog approach may be prefered (not least for its clearer method naming), but making the change in primitivePerformInSuperclass is low risk and more easily implemented by a Sunday Squeaker.

All MirrorPrimitiveTests pass.

=============== Diff against VMMaker-dtl.261 ===============

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

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

• 	"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" ]]
  

• !

• self popStack.
• self primitivePerformAt: lookupClass.
• self successful ifFalse:

• [self push: lookupClass]!
  
  

Item was changed: ----- Method: VMMaker class>>versionString (in category 'version testing') ----- versionString

"VMMaker versionString"

• ^'4.7.19'!

• ^'4.7.18'!

-- Stefan Marr Software Languages Lab Vrije Universiteit Brussel Pleinlaan 2 / B-1050 Brussels / Belgium http://soft.vub.ac.be/~smarr Phone: +32 2 629 2974 Fax: +32 2 629 3525

-- Stefan Marr Software Languages Lab Vrije Universiteit Brussel Pleinlaan 2 / B-1050 Brussels / Belgium http://soft.vub.ac.be/~smarr Phone: +32 2 629 2974 Fax: +32 2 629 3525

On Thu, Jan 05, 2012 at 09:17:44PM +0100, stephane ducasse wrote:

...

So if I sound overly conservative here, but we are dealing with a part of the VM that was designed and implemented by Dan Ingalls, and I am just an amateur VM hacker trying not to break it ;-)

Sure but this is not the graal.

Well, I did not mean to get into a philosophical discussion, I was really only asking for a code review of a small but potentially risky change. Code reviews are a good practice, it's surprising how many things are overlooked when only one set of eyes is at work.

In any case I stand by my admiration for Dan's work. I find it clear, consise, free of unnecessary complications, and a pleasure to read. And like all good writing, it holds up well over time.

Dave

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

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

Dave