Skip to content

Conversation

@en-sc
Copy link
Collaborator

@en-sc en-sc commented Jul 26, 2022

If range's upper bound was equal to 2^64 or the range was wrapping around 0
(which is perfectly legal), writes were not performed due to riscv_addr_t
overflow.

If range's upper bound was equal to 2^64 or the range was wrapping around 0
(which is perfectly legal), writes were not performed due to riscv_addr_t
overflow.
@timsifive
Copy link
Collaborator

This looks right. What testing have you done? Are you able to run the tests in https://github.com/riscv-software-src/riscv-tests/tree/master/debug against spike to check for regressions?

@en-sc
Copy link
Collaborator Author

en-sc commented Jul 26, 2022

I've tested my changes on spike. Though as it is - spike requires some fixes too (I'll link the PR to risc-isa-sim) soon. Once all the changes are done I can provide a regression test for that configuration.

@en-sc
Copy link
Collaborator Author

en-sc commented Jul 26, 2022

I've ran tests in riscv-tests/debug with and without proposed patches (both to spike and to riscv-openocd), running default target (not all-tests). Regardless of configuration I've gotten two exceptions on DisconnectTest and CustomRegisterTest. Is it an issue with my environment (like gcc version), or is it an expected behavior?

@timsifive
Copy link
Collaborator

DisconnectTest and CustomRegisterTest pass for me, so the problem is somehow related to your setup. It could be a gdb version thing, maybe. In either case, if there are no regressions anywhere and the various memory tests pass, then I think that's enough coverage for this change. Please do rerun that with all-tests however, which catches more corner cases.

@en-sc
Copy link
Collaborator Author

en-sc commented Jul 29, 2022

I've changed gdb to 12.1 and ran all-tests. There were no exceptions (make's output is attached, can provide specific logs, if needed)
make-all-tests.log

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.

Great! Thank you for running all the tests.

@en-sc
Copy link
Collaborator Author

en-sc commented Aug 1, 2022

Could you merge the PR or is there something else I need to do first?

@timsifive
Copy link
Collaborator

Yup. Usually I wait a day or weekend in case anyone else wants to comment.

@timsifive timsifive merged commit 5217759 into riscv-collab:riscv Aug 1, 2022
@en-sc en-sc deleted the en-sc/cntr-oflow-in-write_memory_progbuf branch August 14, 2024 18:42
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