MVP of LC3 VM in Rust by tomip01 · Pull Request #6 · tomip01/lc3-vm

tomip01 · 2025-01-02T13:41:50Z

Adds functionality to load programs and execute them
Contains tests for each instruction
Contains integration tests for a happy path and some border cases
Each instruction is documented by explaining how each of them is interpreted as binary

Read memory from a file path

* can process add instruction * modify register access * add methods for getting and setting register values * add tests

Feature: add instructions related to registers

* creates file for trap codes and convertion from u16 * add getc instr * add new trap errors

Feature: memory operations

Feature: Traps

… feature/run_programs merge local repo

Feature: run programs

gabrielbosio · 2025-01-03T21:16:23Z

Can load programs and execute it
Contains tests
Add comments

Please provide a more detailed description of the PR. It's important to provide guidance to reviewers. For example:

Instead of "Can load programs and execute it" it can be "Adds functionality to load programs and execute them"
Instead of "Contains tests" it can be "Contains tests for each instruction" or "Contains integration tests for a happy path and some border cases" or both
Instead of "Add comments" it can be "Each instruction is documented with an example" or "Each instruction is documented by explaining how each of them is interpreted as binary"

gabrielbosio · 2025-01-03T21:19:55Z

+    // collect file to execute
+    let args: Vec<String> = env::args().collect();
+    if args.len() != 2 {
+        println!("make run FILEPATH=<path/to/file>");


As a UX improvement, I would add something to notify the user how the VM is meant to be used. For example, instead of printing:

make run FILEPATH=<path/to/file>

It can print something like:

Usage: make run FILEPATH=<path/to/file>

Done, a8e3acb

gabrielbosio · 2025-01-03T21:26:58Z

+    /// Main execution loop
+    ///
+    /// Read instruction, increments the PC, execute the instruction
+    /// Repeat this while the machine is in a state of running


Please clarify this. I would involve the running field because it's public so the user knows about this field.

Nice!, the members of the VM being public was an error I didn't update.
The comment's clarification and the remove of pub are here, b37e99d

gabrielbosio · 2025-01-03T21:41:31Z

+    /// Get value stored in the register requested
+    ///
+    /// If register is not 0 to 7, an Error is returned


I would include register_index to document how the function argument affects the function logic. For example:

Suggested change

/// Get value stored in the register requested

///

/// If register is not 0 to 7, an Error is returned

/// Get value stored in the register requested in `register_index`

///

/// If `register_index` is outside of the range from 0 to 7, an Error is returned

Done, 2cad29e

gabrielbosio · 2025-01-03T21:41:48Z

+    /// Set a value in the register requested
+    ///
+    /// If register is not 0 to 7, an Error is returned


I would include register_index and value to document how the function arguments affect the function logic.

Done, 2cad29e

gabrielbosio · 2025-01-03T21:42:58Z

+    /// Update flags based on the content of the register requested
+    ///
+    /// If register is not 0 to 7, an Error is returned


I would include register to document how the function argument affects the function logic. Also, if in the other functions the argument is called register_index why it's called different here?

Done, ea89cfd

gabrielbosio · 2025-01-03T21:43:57Z

+        Ok(())
+    }
+
+    /// Based on the opcode from the instruction bits, executes the corresponding procedure


I would include instr to document how the function argument affects the function logic.

Done, 3b71fca

gabrielbosio · 2025-01-03T21:44:44Z

+            Opcode::And => self.and(instr),
+            Opcode::Ldr => self.ldr(instr),
+            Opcode::Str => self.str(instr),
+            Opcode::Rti => Err(VMError::InvalidOpcode),


Add a short comment on why this opcode is invalid

Done, 04491ec

gabrielbosio · 2025-01-03T21:44:49Z

+            Opcode::Ldi => self.ldi(instr),
+            Opcode::Sti => self.sti(instr),
+            Opcode::Jmp => self.jmp(instr),
+            Opcode::Res => Err(VMError::InvalidOpcode),


Add a short comment on why this opcode is invalid

Done, 04491ec

gabrielbosio · 2025-01-03T21:51:47Z

+    /// Stores in DR the addition between SR1 and SR2 or SR1 and imm5 sign extended
+    /// Bit 5 if it is 1, then is read as a imm5


Bit 5, SR2 and imm5 doesn't seem to be related according to this documentation. Please refactor this so that the relationship is explicit. For example:

Suggested change

/// Stores in DR the addition between SR1 and SR2 or SR1 and imm5 sign extended

/// Bit 5 if it is 1, then is read as a imm5

/// If Bit 5 is 0, stores in DR the addition between SR1 and SR2.

/// If Bit 5 is 1, stores in DR the addition between SR1 and imm5.

Done, b5d8659

gabrielbosio · 2025-01-03T21:53:27Z

+    /// Stores in DR the and bitwise between SR1 and SR2 or SR1 and imm5 sign extended
+    /// Bit 5 if it is 1, then is read as a imm5


Bit 5, SR2 and imm5 doesn't seem to be related according to this documentation. Please refactor this so that the relationship is explicit.

Done, b5d8659

gabrielbosio · 2025-01-03T22:05:50Z

+    /// In bit 11 is to jump if is negative
+    /// In bit 10 is to jump if is zero
+    /// In bit 9 is to jump if positive
+    ///
+    /// Stores in PC the addition of the PC and the sign extended PCoffset9 only if the condition flag
+    /// meets any of the bits that are 1


Please rephrase this

Nice!, done, e87d92c

Co-authored-by: Gabriel Bosio <38794644+gabrielbosio@users.noreply.github.com>

* Clarify documentation and the use of the arguments * Rename the argument name for better consistency

tomip01 and others added 30 commits December 26, 2024 09:29

read memory from a file path

0ebb5df

better vm error explanation

6af98b6

convert numbers to opcode instructions

b4cbd92

use std method to convert big endian

53ec0b5

Merge pull request #1 from tomip01/feature/initial_vm

6700edb

Read memory from a file path

include add instruction

b3106a4

* can process add instruction * modify register access * add methods for getting and setting register values * add tests

add instructions and & not

8cddad8

add instruction jmp & branch

a58acf0

add instruction lea & jsr

f37dc9d

change mask from hex to binary

ed2cca0

add instruction ld & ldr

2a38d70

add instructions st & ldi

9c8e85f

add instruction sti & str

498e986

add combined test and refactor of functions

24f3feb

refactor VMError names and uses

35c776b

refactor memory

2b0f9d0

remove reduntant error returns

c5f8222

Merge pull request #2 from tomip01/feature/implementing_instructions

772e04a

Feature: add instructions related to registers

add first trap instr

d399d42

* creates file for trap codes and convertion from u16 * add getc instr * add new trap errors

add documentation for instructions

bbde259

add trap instr out & puts

4f4e154

refactor instructions to separate file

650e5e3

finish all trap instr

e29e6a8

simplify writing in stdout

87893c4

add doc comments

ef7db7f

Merge pull request #3 from tomip01/feature/memory_operations

6eea5cc

Feature: memory operations

remove private call methods on memory

dda3f85

Merge branch 'dev' into feature/traps

1403228

Merge pull request #4 from tomip01/feature/traps

217213e

Feature: Traps

add chech_key() method

f48406d

tomip01 and others added 8 commits December 30, 2024 17:30

add termios config for correct getc function

f7b375c

refactor main

33437f4

add comments and refactors

a290579

Merge branch 'dev' into feature/run_programs

9f91c24

change error prints to the standard error

9ee23b5

Merge branch 'feature/run_programs' of github.com:tomip01/lc3-vm into…

0791b64

… feature/run_programs merge local repo

Merge pull request #5 from tomip01/feature/run_programs

e7996fc

Feature: run programs

add example to README

93bf9c9

gabrielbosio reviewed Jan 3, 2025

View reviewed changes

Comment thread src/lc3/vm.rs Outdated

gabrielbosio reviewed Jan 3, 2025

View reviewed changes

tomip01 and others added 9 commits January 6, 2025 09:42

Improve usage comment when incorrect amount of arguments

a8e3acb

Update main loop execution comments

51cbd35

Co-authored-by: Gabriel Bosio <38794644+gabrielbosio@users.noreply.github.com>

Remove old public members in the VM and clarify comments on

b37e99d

Add comments for the use of args in set and get register

2cad29e

Improve

ea89cfd

* Clarify documentation and the use of the arguments * Rename the argument name for better consistency

Add usage of 'instr' in 'execute' method

3b71fca

Explain why RTI and RES are invalid Opcodes

04491ec

Make documentation explicit on AND and ADD instructions

b5d8659

Make explicit and improve the documentation of 'br'

e87d92c

		/// Stores in DR the addition between SR1 and SR2 or SR1 and imm5 sign extended
		/// Bit 5 if it is 1, then is read as a imm5

		/// Stores in DR the and bitwise between SR1 and SR2 or SR1 and imm5 sign extended
		/// Bit 5 if it is 1, then is read as a imm5

Conversation

tomip01 commented Jan 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabrielbosio commented Jan 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabrielbosio Jan 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabrielbosio Jan 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabrielbosio Jan 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabrielbosio Jan 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tomip01 commented Jan 2, 2025 •

edited

Loading

gabrielbosio Jan 3, 2025 •

edited

Loading

gabrielbosio Jan 3, 2025 •

edited

Loading

gabrielbosio Jan 3, 2025 •

edited

Loading

gabrielbosio Jan 3, 2025 •

edited

Loading