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

Fixed halt reason after single-step #618

Closed
wants to merge 1 commit into from

Conversation

JanMatCodasip
Copy link
Collaborator

After single step operation, we should not assume that the halt
reason is single-step. There can be a higher-priority halt cause,
e.g. a breakpoint.

The real halt reason should be obtained from the target (dcsr.cause).

After single step operation, we should not assume that the halt
reason is single-step. There can be a higher-priority halt cause,
e.g. a breakpoint.

The real halt reason should be obtained from the target (dcsr.cause).
Copy link
Collaborator

@timsifive timsifive left a comment

Choose a reason for hiding this comment

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

That looks right. What testing have you done for this? I'm concerned with some weird case where gdb doesn't work right because it expects that state to be single stepped after issuing that.

@JanMatCodasip
Copy link
Collaborator Author

JanMatCodasip commented Jun 8, 2021

What testing have you done for this?

I've tested this internally against the LLDB debugger (which speaks the GDB Remote protocol, same as GDB). Without the patch, the debugger did not report a watchpoint hit during single-stepping to the user.

For GDB, I need to run riscv-tests/debug, after I resolve some locally occurring riscv-test issues mentioned in #563.

@timsifive
Copy link
Collaborator

Assuming e.g. all the spike32-2 tests from riscv-tests/debug pass then I'm fine merging this.

@JanMatCodasip
Copy link
Collaborator Author

Assuming e.g. all the spike32-2 tests from riscv-tests/debug pass then I'm fine merging this.

I've run the riscv-tests on this change here: JanMatCodasip#3
It turns out the impact of this change is large and many tests now fail. I am parking this change (draft) until I manage to find the cause and fix it.

I still believe reporting the real halt cause to the debugger when stepping is correct approach, yet this change is not sufficient as is and something else needs to be updated as well.

@JanMatCodasip
Copy link
Collaborator Author

Closing this outdated merge request.

The main problem appears to be that riscv_openocd_step() reports resume + halt to GDB every time, but "internal" steps (e.g. when resuming from a trigger) should be hidden from the GDB. This can be fixed in a separate merge request.

@JanMatCodasip JanMatCodasip deleted the fix-halt-reason-after-singlestep branch January 13, 2025 06:50
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