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

difftest-as-dut: allow ref model to advance one more step on mismatch #407

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

shinezyy
Copy link
Contributor

  • When trapping on exception, occasionally, NEMU has executed
    csrrw tp,mscratch,tp, but Spike has not.
    After advancing Spike for one more step,
    Spike's architectural state will match with NEMU again.

@shinezyy shinezyy force-pushed the one-more-step-on-mismatch branch 3 times, most recently from 197089e to 64376bb Compare July 18, 2024 07:49
@shinezyy
Copy link
Contributor Author

Current implementation will corrupt the site of CPU_state during second checking

@shinezyy
Copy link
Contributor Author

Current implementation will corrupt the site of CPU_state during second checking

resolved

Copy link
Member

@cebarobot cebarobot left a comment

Choose a reason for hiding this comment

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

LGTM

@shinezyy
Copy link
Contributor Author

shinezyy commented Jul 22, 2024

@cebarobot @poemonsense

Can we do regression on difftest for bugs detection? Because current CI only ensure `` no difference ''.

If I mistakenly modify NEMU to never report difference in difftest, it will also pass current CI.

@cebarobot
Copy link
Member

I think that is useful, but how to do that?

@poemonsense
Copy link
Member

I know few about NEMU as DUT.

Generally, we need a failure testcase to test the functionality?

@shinezyy
Copy link
Contributor Author

I know few about NEMU as DUT.

Generally, we need a failure testcase to test the functionality?

Yes, for example, use a buggy Spike as ref, and check output.

I am thinking about this because I am afraid that this patch (or similar patches) silently breaks NEMU as DUT (never report difference) but passes CI

@poemonsense
Copy link
Member

I know few about NEMU as DUT.
Generally, we need a failure testcase to test the functionality?

Yes, for example, use a buggy Spike as ref, and check output.

I am thinking about this because I am afraid that this patch (or similar patches) silently breaks NEMU as DUT (never report difference) but passes CI

This feature looks important. The difftest repo also lacks this CI feature.

  1. checking the failure can be done by checking the return code of a command. GitHub CI is internally a bash script. I think it can be done by like https://stackoverflow.com/questions/26675681/how-to-check-the-exit-status-using-an-if-statement I never tried this but it probably works.
false || exit_code=$?
if [[ ${exit_code} -ne 0 ]]; then echo ${exit_code}; fi
  1. To inject a bug into Spike/NEMU, maybe we can add a probablistic return code of nonzero for the isa_difftest_checkregs function?

@poemonsense
Copy link
Member

I know few about NEMU as DUT.
Generally, we need a failure testcase to test the functionality?

Yes, for example, use a buggy Spike as ref, and check output.
I am thinking about this because I am afraid that this patch (or similar patches) silently breaks NEMU as DUT (never report difference) but passes CI

This feature looks important. The difftest repo also lacks this CI feature.

  1. checking the failure can be done by checking the return code of a command. GitHub CI is internally a bash script. I think it can be done by like https://stackoverflow.com/questions/26675681/how-to-check-the-exit-status-using-an-if-statement I never tried this but it probably works.
false || exit_code=$?
if [[ ${exit_code} -ne 0 ]]; then echo ${exit_code}; fi
  1. To inject a bug into Spike/NEMU, maybe we can add a probablistic return code of nonzero for the isa_difftest_checkregs function?

I can have a try for these in the difftest repo as well. We may need to add the similar feature to other repos. It's really important

@shinezyy shinezyy force-pushed the one-more-step-on-mismatch branch 3 times, most recently from ae591cc to 9fbc0df Compare July 23, 2024 09:48
- When trapping on exception, occasionally, NEMU has executed
`csrrw   tp,mscratch,tp`, but Spike has not.
After advancing Spike for one more step,
Spike's architectural state will match with NEMU again.
@shinezyy
Copy link
Contributor Author

Because my CI guard for NEMU as DUT depends on misa.rvb, I will add it in RVB patch

@shinezyy shinezyy merged commit 911f2b3 into master Jul 24, 2024
5 checks passed
@shinezyy shinezyy deleted the one-more-step-on-mismatch branch July 24, 2024 03:27
@NewPaulWalker
Copy link
Contributor

  • When trapping on exception, occasionally, NEMU has executed
    csrrw tp,mscratch,tp, but Spike has not.
    After advancing Spike for one more step,
    Spike's architectural state will match with NEMU again.

This is actually because when certain exceptions occur, NEMU does not let Spike continue executing the instruction that caused the exception. As a result, Spike falls one instruction behind NEMU, leading to a mismatch. After letting Spike execute one more instruction, they match again. I think all exceptions (excluding interrupts) are caused by the instructions themselves, so I decided that whenever an exception occurs, NEMU should let Spike execute one instruction, which is the instruction that cause the exception.
https://github.com/OpenXiangShan/NEMU/blob/master/src/isa/riscv64/system/intr.c#L71

@poemonsense
Copy link
Member

I think all exceptions (excluding interrupts) are caused by the instructions themselves, so I decided that whenever an exception occurs, NEMU should let Spike execute one instruction, which is the instruction that cause the exception.

This is how difftest should work. Upon exception/interrupt, should tick one step.

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.

None yet

4 participants