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

Add an option to produce a textfile trace of instructions executed #39

Merged
merged 3 commits into from
Dec 30, 2023

Conversation

breqdev
Copy link
Owner

@breqdev breqdev commented Dec 27, 2023

Useful for debugging.

Adds infrastructure as well in case we want to make the trace more detailed in the future, make other kinds of trace handlers, etc.

@breqdev breqdev requested a review from ava-silver December 27, 2023 04:29
@breqdev breqdev self-assigned this Dec 27, 2023
Copy link

codecov bot commented Dec 27, 2023

Codecov Report

Attention: 41 lines in your changes are missing coverage. Please review.

Comparison is base (8e222b8) 27.40% compared to head (db62e23) 27.31%.

Files Patch % Lines
src/trace/file.rs 0.00% 14 Missing ⚠️
src/cpu/mos6502/mod.rs 20.00% 12 Missing ⚠️
src/main.rs 0.00% 8 Missing ⚠️
src/systems/mod.rs 0.00% 4 Missing ⚠️
src/trace/mod.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #39      +/-   ##
==========================================
- Coverage   27.40%   27.31%   -0.10%     
==========================================
  Files          42       44       +2     
  Lines        9014     9055      +41     
==========================================
+ Hits         2470     2473       +3     
- Misses       6544     6582      +38     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@breqdev
Copy link
Owner Author

breqdev commented Dec 29, 2023

I wanted to avoid a Box-heavy API here but I think it's unavoidable -- impl doesn't work on System since its a trait :(

fn handle(&mut self, trace: &CpuTrace) {
self
.file
.write_all(format!("{:04X}: {:02X}\n", trace.address, trace.opcode).as_bytes())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be good to make this buffered with BufWriter, i'm happy to implement that

ava-silver
ava-silver previously approved these changes Dec 29, 2023
Copy link
Collaborator

@ava-silver ava-silver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I can implement buffering for the trace but otherwise LGTM!

@ava-silver ava-silver dismissed their stale review December 30, 2023 03:56

waiting until we add support for buffering

@breqdev
Copy link
Owner Author

breqdev commented Dec 30, 2023

Rebased on top of main to include changes from #40

Copy link
Collaborator

@ava-silver ava-silver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

@breqdev
Copy link
Owner Author

breqdev commented Dec 30, 2023

:shipit: :shipit: :shipit:

@breqdev breqdev merged commit a2ba089 into main Dec 30, 2023
5 checks passed
@ava-silver ava-silver deleted the bc/trace branch December 30, 2023 07:02
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

Successfully merging this pull request may close these issues.

2 participants