Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug in PUTSP trap implementation? #51

Open
pro465 opened this issue Apr 23, 2022 · 7 comments
Open

bug in PUTSP trap implementation? #51

pro465 opened this issue Apr 23, 2022 · 7 comments

Comments

@pro465
Copy link

pro465 commented Apr 23, 2022

in

lc3-vm/docs/index.html

Lines 929 to 944 in e854c34

<pre class="prettyprint"><code class="">{
/* one char per byte (two bytes per word)
here we need to swap back to
big endian format */
uint16_t* c = memory + reg[R_R0];
while (*c)
{
char char1 = (*c) &amp; 0xFF;
putc(char1, stdout);
char char2 = (*c) &gt;&gt; 8;
if (char2) putc(char2, stdout);
++c;
}
fflush(stdout);
}
</code></pre>

it seems that the trap only stops printing when it encounters a 0x0000. however, the specification says that:

A character string consisting of an odd
number of characters to be written will have x00 in bits [15:8] of the memory
location containing the last character to be written.

in my interpretation, it sounds like the trap should stop when it has encountered a 0x0000, or it should print the first byte, and if the second byte is 0x00, it should stop, otherwise it should print it too. but this does not align with your implementation. i wonder whether it is a bug or my interpretation is wrong?

@rpendleton
Copy link
Collaborator

rpendleton commented Apr 23, 2022

I believe the double null is actually expected. The sentence immediately after the portion of the spec that you quoted mentions the double null:

Writing terminates with the occurrence of x0000 in a memory location.

With that said though, it looks like there's an inconsistency between the opcode appendix and the official open source implementation of the LC3 trap implementations. Here's a comment from the beginning of the PUTSP implementation from lc3os.asm:

TRAP_PUTSP
	; NOTE: This trap will end when it sees any NUL, even in
	; packed form, despite the P&P second edition's requirement
	; of a double NUL.

So although the specification requires a double null, it seems the official open source implementation intentionally ignores that requirement...

I'm not sure what the best answer is here.

@justinmeiners
Copy link
Owner

@rpendleton perhaps we should check the latest addition of the textbook on this subject.

@justinmeiners
Copy link
Owner

Confirmed that double NUL is still the specified behaviour. @pro465 do you have an application this breaks? Or, just bringing it for correctness/consistency sake?

@pro465
Copy link
Author

pro465 commented Apr 24, 2022

@justinmeiners well i was just implementing the LC-3 in Rust and was confused. so i would say i was bringing it up for consistency/correctness.

@justinmeiners
Copy link
Owner

justinmeiners commented Apr 24, 2022

@pro465 I talked with @rpendleton about it. We agree it's weird, but I don't see a great reason to not follow the spec here. Perhaps something else in the lc3 ecosystem will inform us.

Either way, we have been getting several questions about detailed standards topics. I think it would be a good idea to start a markdown document that records these decisions.

@pro465
Copy link
Author

pro465 commented Apr 25, 2022

since, it seems there are widely different implementations and interpretations, i did not implement the traps and stuff, instead i left them to the OS and instead made an OS-agnostic VM:
https://github.com/pro465/lc3

@pro465 pro465 changed the title bug in PUSTP trap implementation? bug in PUTSP trap implementation? Apr 25, 2022
@pro465
Copy link
Author

pro465 commented Apr 25, 2022

Either way, we have been getting several questions about detailed standards topics. I think it would be a good idea to start a markdown document that records these decisions.

yeah i'd agree to that: to get some sort of FAQ section in the README that answers what kind of design decisions the maintainers took.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants