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

target/riscv: Don't assert in riscv013_get_register() #916

Merged
merged 1 commit into from
Sep 14, 2023

Conversation

timsifive
Copy link
Collaborator

When the target isn't halted, simply return an error. This used to be purely internal code so an assert was appropriate. Now after some refactoring and with unavailable harts you could get here when the hart is unavailable. In that case the right thing is simply to return an error message.

Change-Id: I49d26a11fe7565c645fd2480e89a2c35ea9b1688

When the target isn't halted, simply return an error. This used to be
purely internal code so an assert was appropriate. Now after some
refactoring and with unavailable harts you could get here when the hart
is unavailable. In that case the right thing is simply to return an
error message.

Change-Id: I49d26a11fe7565c645fd2480e89a2c35ea9b1688
Signed-off-by: Tim Newsome <tim@sifive.com>
Copy link
Collaborator

@JanMatCodasip JanMatCodasip left a comment

Choose a reason for hiding this comment

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

This looks all right. All calls to riscv_save_register() have their return value properly checked, so the change is safe.

Comment on lines +4949 to +4950
LOG_TARGET_ERROR(target, "Can't save register %s on a hart that is not halted.",
gdb_regno_name(regid));
Copy link
Collaborator

@JanMatCodasip JanMatCodasip Sep 14, 2023

Choose a reason for hiding this comment

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

If you like, the state of the target can be part of the error message. This can help in future with log analysis and troubleshooting.

Suggested change
LOG_TARGET_ERROR(target, "Can't save register %s on a hart that is not halted.",
gdb_regno_name(regid));
LOG_TARGET_ERROR(target, "Can't save register %s on a hart that is not halted (current state: %s).",
gdb_regno_name(regid), target_state_name(target));

@timsifive timsifive merged commit 2427f58 into riscv Sep 14, 2023
5 checks passed
@timsifive timsifive deleted the getreg_assert branch September 14, 2023 18:37
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