• Post Reply Bookmark Topic Watch Topic
  • New Topic

Simple Virtual Machine - Stack Based - Unable to find error in code  RSS feed

 
Nick Kilo
Greenhorn
Posts: 22
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Built using jGRASP version 2.0.3.

I've been tasked with building a stack based VM. I'm not quite there an am stuck trying to figure out why my "case 21:" functions inconsistently. Specifically on cases 11-17, it provides no output. But if I put case 21 right after a case 8 in the VM it works without issue.

I have played around with it by printing the stack elements at varying stages, everything appears to function down to adding the output to the stack, since when I call stack.showMeTheMoney() it provides all the stack details reflecting the case 11-17 call.

Any guidance would be greatly appreciated. Please let me know if you have any questions.

VM Class:



Stack Class:



StackIntADT in case you need it:




 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Well, first thing I'd advise is to define a bunch of symbolic names for your op codes. That would make your code so much easier to read, rather than have those comments.

Then, I'd make a convenience method for moving through the instructions and use the post-increment operator instead:

Or you could use common terms in the domain but if you do that, I advise adding a comment:

It's a little cryptic but it creates less visual clutter once you understand what IR and PC stand for.
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
To illustrate how comments are poor substitutes for good names, look at the comment you have on line 70, for the op code 13. That's not right.  Same issue with op code 14.  And fixing the comments instead of creating expressive symbolic names is just being lazy.
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The logic in your pop() method doesn't mesh with the logic in your push() method. 

Also, for zero-based stacks, the management of the top variable is usually done differently from how you are doing it. The usual implementation is for top to represent the current size of the stack, so when you push(), the operation is done simply with stack[top++] = item and when you pop() then you'd do something that would match how top is being managed. I'll let you figure out that part for yourself. The checks for overflow and underflow would also need to be adjusted accordingly. The implementation of peek() might seem complicated at first but when you figure it out, it's really very simple.  The size() method simply returns the value of top, isEmpty() just checks if size() is 0. Once you get a hang of the semantics of these high-level methods, you don't have to do a lot of low-level checking in your other methods. Like when checking for overflow, you just check size() instead of using top directly. Same thing for underflow checks.

Examine this part of your code carefully:

Look at what you're doing on lines 37-38. Does that logically match what you're doing on 58-59?

Also, look at line 32. This is good.

Now, look at lines 43, 53, and 64. Do you see the duplication? Duplication of an idea in code is bad. A calculation should be in only one place. Everything else should just use that calculation via a method call, like what you're doing on line 32.
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Your implementation of op code 22 (call subprogram) is a little confusing even though it's correct. This would have been easier to follow, IMO:
 
Nick Kilo
Greenhorn
Posts: 22
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks for the feedback.  I have made some adjustments based on your observations, but I don't fully understand where I would be using the "word" variable for your recommended convenience method. 

Additionally, none of the changes seemed to have impacted the original problem of WRITE not displaying an integer when it seems it otherwise ought to be when placed after case ADD and beyond in "memory" .

Any additional feedback would be appreciated. Thank you again.

VM (Modification Round 1):



Stack (Modification Round 1):

 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Nick Kilo wrote:I don't fully understand where I would be using the "word" variable for your recommended convenience method.

"word" is just a name that I thought better represented the intent of the value.  You have an instructionRegister and a programCounter and as you step through your "program instructions" each "word" in those instructions are loaded in the instructionRegister.  To call the next integer that represents the part of the instructions that you're processing the "instructionRegister" is not right.  That's like taking the equation "1 + 2 = 3" and referring to  "1" and "+" as "additions".  You want to use the right name that fits the thing it represents properly.  Names should reveal intent/purpose.

Anyway, I'm a little rusty in my assembly language but from what I recall, each thing in an instruction set that will go into the instruction register is a "word"; I may be misremembering. I'll have to google around for a bit first. But that was my intent in using "word" as the variable name.
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
You initialize topOfTheStack to -1

Then, in push(), you do this:
In pop(), you do this:
This looks better now but it's still inelegant.  Lines 59 and 60 are particularly kludgy in the way you manage top. You have twice as much code as you need and you're still starting at -1, which again, is not how 0-based stacks are normally implemented. You usually start with top = 0 and manage top so that it's always the same as your current stack size.

Hint: you can combine line 37 with 38 and line 59 with 60 if you start with top == 0 and use post/pre increment/decrement appropriately. Operations like these are what the pre/post decrement/increment operators are made for.
 
Nick Kilo
Greenhorn
Posts: 22
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I set topOfTheStack to -1 because when I push the first value I have to increment it first with topOfTheStack++ before I assign stack[topOfTheStack] = newInputValue; or it causes an error.  That increment gets me to index 0 of the stack to store the first input.

Though it seems to get around that problem I could use stack[topOfTheStack - 1] = newInputValue; in the push method.

Elegance aside, is the current structure having an adverse effect on the WRITE case not functioning correctly as described previously?

 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I wrote:"word" is just a name that I thought better represented the intent of the value.

Alternatively, you could use "opCode" where you expect the next int coming from the IR to be an op code and "argument" or "value" to clearly express that what you expect to retrieve from the IR is an argument or value that goes with the last op code you read from the IR. So:

Even more succintly:

If you don't mind being more verbose, you can use "operation" instead of "opCode". That will actually make your code more readable, IMO. I'm starting not to like my suggestion of ir(pc++) because it looks very cryptic but the alternative instructionRegister(programCounter++) is very long. ¯\_(ツ)_/¯
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Nick Kilo wrote:
Elegance aside, is the current structure having an adverse effect on the WRITE case not functioning correctly as described previously?

Elegance can help you write correct programs. Kludges hides bugs. You popped the stack but never adjusted PC. No wait, that's not it either.
 
Nick Kilo
Greenhorn
Posts: 22
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Where did I fail to advance the PC?

How does the above error affect WRITE not being able to function after ADD?

Please advise.
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
<facepalm> I see it now. When you execute the ADD, SUBTRACT, and other similar operations, does your PC need to be adjusted? Why? If you look at your "program instructions"—and having the symbolic constants in there makes it VERY readable, BTW, and easy to recognize your mistake—is there anything after the ADD opcode that would necessitate moving the PC?

Basically, the bug in your ADD operation corrupted your PC and therefore made your program do unexpected things. I was looking at the placement of the WRITE case within the switch statement until I realized that you were referring to the WRITE operation that came after ADD in your program instructions. So the first WRITE worked because you had a good PC. But the second WRITE didn't because after that ADD, your PC was corrupted.
 
Nick Kilo
Greenhorn
Posts: 22
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
That was it!  Thank you sir!  I forgot my do loop already does some of the required incrementing and I was over doing it for those cases. Thanks for taking the time to point out my styling errors too, I'm very new to programming and I'm sure I'll stamp those out with time as my knowledge base improves and my exposure to seasoned personnel increases.

Please feel to send me a URL you may have, I can tweet, like, follow, etc. for the help.
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
There are numerous similar bugs like that left in your code. This goes to show how abstracting away some of these things could have helped.  By always doing any of the following:

instead of sprinkling a bunch of disconnected:
all over your code, you could have avoided these bugs. I think you got your signals crossed between the programCounter variable and your topOfTheStack variable. I think that's what happened to me to in reading your code so that's why it took a while to find the logic error.
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Nick Kilo wrote:Please feel to send me a URL you may have, I can tweet, like, follow, etc. for the help.

No problem, glad to be able to help. Tweet to or follow @coderanch if you want to say nice things and spread the word about us.
 
Nick Kilo
Greenhorn
Posts: 22
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Done, just liked and posted on CR Facebook page.
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Nick Kilo wrote:I set topOfTheStack to -1 because...

Ok, watch and learn, kiddo.
 
Nick Kilo
Greenhorn
Posts: 22
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks, that make more sense to me.

I'm monkey see monkey do, and my instructor showed me the method you saw deployed (likely to make it easier for us greenhorns). I use something until it no longer works or I'm presented with superior information. I'll deploy yours moving forward. Thanks again.
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
FWIW, your code is relatively pretty decent. Your indentation style isn't horrible—at least it's pretty consistent—and you seem to be a quick study, picking up right away on the little nuances I showed you. I think you'll do fine going forward. My last piece of advice to you (for now) would be to take whatever your instructor gives you with a grain of salt. If he was the one who showed you that stack implementation, then frankly, I think you should always ask for a second opinion about what he gives you. If his intent was to make it easier for you greenhorns, I don't think he achieved that goal, to be brutally honest. Good luck.
 
Junilu Lacar
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The crypticness of that ir(pc++) expression just keeps bugging the heck out of me. I need to feel better about it. I'd probably end up refactoring to this eventually:


This would allow me to write:

Although it seems like I have a bunch of extra alias methods that I don't really need, I think the context and clarity that the give the code that uses them is worth the few extra lines of code and method calls. An optimizing compiler might even inline these so you might not even have that method call overhead. Just a thought, but I think I feel better about that code now. However, something needs to be done about that pc variable and how the logic of incrementing it or not is spread out all over the code. Ah well, I suppose that's another exercise.
 
Nick Kilo
Greenhorn
Posts: 22
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thank you for taking the time to share that with me; I don't often get to see a more skilled individual's execution of something I'm working, it's helpful.
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!