StaticsTest: Jump requested but A=-2 is an illegal program address.

[fsmith23]

After comparing my .asm files against working ones for hours on end I am dumbstruck. No idea why I'm getting this error. I've poured over every line of my output at least 5 times and everything seems like it should work. I'm guessing there is a single line somewhere that is killing me but I just can't find it.

I've watched the CPU Emulator step through the code but I still can't figure out why my A is being set to -2.

I can send my VMTranslator code (written in Java) to anyone who wants to help, but I doubt you would want to drudge through that. Here is a link to my .asm output file; I can remove it after I figure out what my issue is, sorry if posting it is classed as "posting answers".

If anyone can help me figure out this error I would be extremely grateful.

[jacklack]

I think I just had the same issue.

The problem is coming from calling a function with zero arguments. In this case call Class1.get 0
What happens is that there is no slot for the return value as we didn't push any arguments, what follows is that the return value is stored into the slot where the return address is, instead of beign stored one slot down the stack. Then upon return we jump to this return address which now contain the return value -2 (for 6-8).

The solution is to check if we call a function with zero arguments, then we increase SP and continue the assembly implementation of the VM call function arguments command as normal. (the SP increase acts as a push that will be used for the function return value).

[cadet1620]

jacklack wrote

The problem is coming from calling a function with zero arguments. In this case call Class1.get 0
What happens is that there is no slot for the return value as we didn't push any arguments, what follows is that the return value is stored into the slot where the return address is, instead of beign stored one slot down the stack. Then upon return we jump to this return address which now contain the return value -2 (for 6-8).

Yes, this is what happens when you overwrite the return address with the return value.

The solution is to check if we call a function with zero arguments, then we increase SP and continue the assembly implementation of the VM call function arguments command as normal. (the SP increase acts as a push that will be used for the function return value).

This is an incorrect solution because the called function does not know how many arguments it has so there is no way that the 'return' can deal with the special case of 0 arguments.

The correct thing to do is to save the return IP in a temporary variable like R15, as shown in figure 8.5, before moving the return value into arg[0].

--Mark

[cadet1620]

cadet1620 wrote

This is an incorrect solution....

Well maybe not -- I think I didn't understand when you meant to increment SP.

What you are doing is effectively inserting a "push const 0" ahead of every "call function 0" command and changing the command to "call function 1".

Thus, at entry to function, ARG will point to the inserted argument, and LCL and SP will correctly point immediately following the return IP. Therefore because the called function doesn't know how many arguments is is supposed to have, it will return correctly with the inserted argument stripped from the stack exactly the same way as the real argument is stripped by a function with 1 argument. (Replaced by the return value, actually.)

Since this extra push doesn't need to supply a value, it is as you said, just an SP increment.

This only costs 2 instructions per call, and it should make the return code more than 2 instructions shorter not to need the temp variable, thus resulting in smaller and faster generated code.

Congratulations on an interesting insight; I'll need to try implementing this and seeing how much it saves.

--Mark

[fsmith23]

Thank you both for your help, I guess I'm glad that I helped spark that epiphany for you Cadet.

Actually, this is going to sound incredibly stupid, but the reason I was having this error was because all my generated code back to the beginning of chapter 7 was failing its tests. I suppose at some early hour my brain snapped and I began to assume that if I made it to the end of my generated .asm files, I had "passed" their tests, despite the error I was getting (I suppose I assumed it was a result of my then-unnecessarily-added-infinite-loop). I don't know what I was thinking.

Good news is, I plugged my brain back in and noticed the previously-invisible .cmp files in each directory, and began debugging using the same strategy I had used for the earlier chapters: Run tests, compare .out and .cmp, make changes. After following this strategy all the way through chapter 8 I have managed to actually pass every test. Hooray.

[cadet1620]

cadet1620 wrote

Congratulations on an interesting insight; I'll need to try implementing this and seeing how much it saves.

I finally got a chance to analyze this in detail. Here's what I find.

---

The proposed calling convention change is:

For all calls to functions with no arguments (call function 0 VM commands), increment the stack pointer before pushing the return address.

This, in effect, turns the call into a call function 1 with a random argument. This is benign because the called function will not reference the argument that it does not have. Since the return code does not depend on the number of arguments, this fake argument has no effect on the function's return.

The return code is simplified because the return address does not need to be cached in a temporary variable since there is always a space for the return value to be moved to (ARG[0]) before the return address is needed.

	Original return code		Simplified return code
	FRAME1 = LCL
	RET = *(FRAME-5)
	*ARG = pop()		*ARG = pop()
	SP = ARG+1		SP = ARG+1
			FRAME = LCL
	THAT = *(FRAME-1)		THAT = *(FRAME-1)
	THIS = *(FRAME-2)		THIS = *(FRAME-2)
	ARG = *(FRAME-3)		ARG = *(FRAME-3)
	LCL = *(FRAME-4)		LCL = *(FRAME-4)
	goto RET		goto *(FRAME-5)
	1FRAME is a temporary variable, R15 for example.

Recognizing that the the run of *(FRAME-n) accesses are pops in disguise (using FRAME instead of SP as the stack pointer), can shorten this code significantly. Getting rid of the out-of-sequence FRAME-5 access and the RET store and recall reduces my return code by 6 instructions (13%).

Sounds promising...

But this may be a false optimization because every call to a function with no arguments requires at least one extra instruction to increment the stack pointer for the fake argument. There are 20 such calls in the OS .vm files.

My code writer only writes the return instructions once--the first time it translates a return VM command. All remaining returns require only 2 instructions: @$RETURN 0;JMP ($RETURN is the label at the start of the return code.)

I also only write most of the call code once. The actual calls load target address, number of arguments and return address into registers and jump to the common code. I can add a $CALL0 entry point to $CALL that increments SP and falls into $CALL. This requires 2 instructions.

The net effect will be to save 4 words of ROM, with a small speed improvement. Every call()/return pair will save 4 cycles; every call(...)/return pair will save 6 cycles. Stack usage will increase by a small amount depending on the number of nested no-argument calls.

Additionally, because of the calling convention change, all the test files that contain calls will need to be adjusted.

---

CONCLUSION

This is too much work for too little gain, so I am not going to write a test implementation.

--Mark

[jacklack]

cadet1620 wrote

Additionally, because of the calling convention change, all the test files that contain calls will
need to be adjusted.

I don't understand this. Why would the test files need adjusting? (I assume those are .vm files).

[cadet1620]

jacklack wrote

cadet1620 wrote

Additionally, because of the calling convention change, all the test files that contain calls will
need to be adjusted.

I don't understand this. Why would the test files need adjusting? (I assume those are .vm files).

The test files that will need tweaks are the .TST scripts and their associated .CMP files. They determine whether a test passes of fails based on the return value of a function in the vm files.  Since these functions are above the bootstrap's call to Sys.init() which puts an extra word on the stack, the return value that needs testing will be moved by one in the stack and the final SP value will be off by one. For example, here's how FibonacciElement.cmp will need to change:

% diff FibonacciElement.cmp FibonacciElement-c2.cmp
1,2c1,2
< | RAM[0] |RAM[261]|
< |    262 |      3 |
---
> | RAM[0] |RAM[262]|
> |    263 |      3 |

--Mark

[jacklack]

I didn't need to change the test files at all. I remember passing all the tests of chapter 8 with success.

[cadet1620]

jacklack wrote

I didn't need to change the test files at all. I remember passing all the tests of chapter 8 with success.

Interesting...

How did you write your bootstrap code's call to Sys.init()?  I call my internal writeCall("Sys.init", 0) routine which will include the extra SP increment.

One could eliminate that extra increment in the special case of the Sys.init() call by making the assumption that Sys.init never returns, but that's setting up a time bomb.

--Mark

[jacklack]

I assume I don't return from Sys.init.
Code removed.

[cadet1620]

jacklack wrote

I assume I don't return from Sys.init, here is the bootstrap code:

void Code::AddBootStrapCode()
{
        //Set SP to point to memory address 261 from the initial 256 (to simulate a call to Sys.init)
        m_xAssemblyFile << "@261\n";
        m_xAssemblyFile << "D=A\n";
        m_xAssemblyFile << "@" << RamReservedStrings[SP] << '\n';
        m_xAssemblyFile << "M=D\n";

        //Jump to Sys.init
        m_xAssemblyFile << "@Sys.init\n";
        m_xAssemblyFile << "0;JMP\n";
}

By jumping to Sys.init this way, the stack frame for Sys.init() is corrupt because the LCL pointer is not set.  This does not cause a problem with the supplied Sys.vm because Sys.init() doesn't use any locals and calls it makes to other functions will correctly initialize LCL.

If the Sys.init you write in project 12 uses locals bad things will happen.... At a minimum, you need to set LCL to the same value as SP.  

[Please edit your post to remove the call code -- we want students to develop their own.]

--Mark

[jacklack]

I just started on chapter 9, been a month now that I finished chapter 8. I need to find motivation and time.
Thanks for your help, really appreciated.
I also plan to move on to the Hack 2 architecture you made, one day I hope.

[sgaweda]

For anyone else encountering this problem there's another possible cause that doesn't become apparent right away even if you've followed the book. In my case my logic was flawed. My temporary variable was R15 and in this location I was storing the location in the stack that the return address was stored when I should have been storing the return address itself in this variable. What resulted was that my returns were successful for FibonacciElement which doesn't call any functions without arguments and unsuccessful in StaticsTest which does. I simply corrected my code to store the return address correctly and removed the line of code at the end of return which was used to get the address from the stack and now successfully pass StaticsTest. I hope that my experience helps others who may have made a similar mistake while still trying to implement this problem as the book has suggested it.