Skip to content

Add an option to dump registers as JSON #128

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

Merged
merged 8 commits into from
Jun 2, 2023

Conversation

long-long-float
Copy link
Contributor

I added an option --dump-registers to dump registers as JSON.
This is useful for automated testing RISC-V binaries.

Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Squash the commits and update README.md.

@jserv jserv requested a review from RinHizakura May 11, 2023 16:50
@@ -1456,3 +1457,13 @@ void ecall_handler(riscv_t *rv)
rv_except_ecall_M(rv, 0);
syscall_handler(rv);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel a little bit weird to dump the result at STDOUT and forcing to silence the output from syscall_handler for this. Since these two features don't depend on each other. Will providing an extra argument for option --dump-registers to specify where to output(it could be a file name or STDOUT, if we really need it) become better?

And if we would like to not output the message from syscall_handler, we may introduce another command line option there.

Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Rebase and resolve the conflicts.

@long-long-float
Copy link
Contributor Author

@jserv Yes, sure.

Also I will add a silence option respecting the following comment.

And if we would like to not output the message from syscall_handler, we may introduce another command line option there.


Squash the commits and update README.md.

I will squash the commits. However how can I update README.md ?

@jserv
Copy link
Contributor

jserv commented Jun 1, 2023

I will squash the commits. However how can I update README.md ?

You can add the description about the proposed option of this emulator.

@jserv jserv requested a review from RinHizakura June 2, 2023 12:27
Copy link
Collaborator

@RinHizakura RinHizakura left a comment

Choose a reason for hiding this comment

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

Thank you! I have no problem now. Just remember to update the README.md as @jserv required.

"dump-registers.\n");
return false;
}
registers_out_file = args[++i];

Check notice

Code scanning / CodeQL

For loop variable changed in body

Loop counters should not be modified in the body of the [loop](1).
@@ -237,6 +264,10 @@
run(rv);
}

/* dump registers as JSON */
if (opt_dump_regs)
dump_registers(rv, registers_out_file);

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression

This argument to a file access function is derived from [user input (a command-line argument)](1) and then passed to dump_registers(out_file_path), which calls fopen(__filename). This argument to a file access function is derived from [user input (a command-line argument)](1) and then passed to dump_registers(out_file_path), which calls fopen(__filename).
if (strncmp(out_file_path, "-", 1) == 0) {
f = stdout;
} else {
f = fopen(out_file_path, "w");

Check failure

Code scanning / CodeQL

File created without restricting permissions

A file may be created here with mode 0666, which would make it world-writable.
@jserv jserv merged commit 62cc6da into sysprog21:master Jun 2, 2023
@jserv
Copy link
Contributor

jserv commented Jun 2, 2023

Thank @long-long-float for contributing!

2011eric pushed a commit to 2011eric/rv32emu that referenced this pull request Jul 22, 2023
vestata pushed a commit to vestata/rv32emu that referenced this pull request Jan 24, 2025
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.

3 participants