Patch support for --timing flag in verilator

[mczyz-antmicro]

This PR adds a fix to error "Simulator shut down prematurely" when cocotb is used with Verilator and --timing parameter. The proposed fix is related to issue #3254

If the HDL code contains timing statements, e.g. #10 clk = ~clk;, then the Verilator should be called with a --timing flag. In current implementation Verilator shuts down prematurely, beacuse the call to VerilatedVpi::cbNextDeadline() returns uint64 max, meaning no more scheduled events (cbNextDeadline() is the deadline of the next registered VPI callback). If we want to support the Verilator's --timing flag, then a check must be performed to see if there are any events pending top->eventsPending() and then find the next time slot top->nextTimeSlot(). The next time step is then the smaller of time steps returned by VerilatedVpi::cbNextDeadline() and top->nextTimeSlot()

[kgugala]

hmm, CI failures seem to be unrelated to this PR

[ktbarrett]

Yeah, they seem unrelated, which is odd since #3321 is passing.

[codecov]

Codecov Report

Merging #3316 (7f8246f) into master (57b2238) will increase coverage by 4.95%.
The diff coverage is n/a.

❗ Current head 7f8246f differs from pull request most recent head a76b296. Consider uploading reports for the commit a76b296 to get more accurate results

@@            Coverage Diff             @@
##           master    #3316      +/-   ##
==========================================
+ Coverage   43.89%   48.85%   +4.95%     
==========================================
  Files          49       49              
  Lines        8816     8816              
  Branches     2446     2446              
==========================================
+ Hits         3870     4307     +437     
+ Misses       4388     3932     -456     
- Partials      558      577      +19     

see 17 files with indirect coverage changes

[ktbarrett]

So when not in --timing mode, what does top->eventsPending() and top->nextTimeSlot() return? What happens if cocotb "deadlocks" by waiting on an event that will never happen? We expect the simulator to detect that and exit instead of hang indefinitely. Does this solution avoid that?

[ktbarrett]

I don't know enough about Verilator to say this doesn't change existing behavior. Waiting on feedback.

[imphil]

LGTM, thanks! Can you do the small nitpick update, @mczyz-antmicro?

So when not in --timing mode, what does top->eventsPending() and top->nextTimeSlot() return?

• eventsPending() returns false without complaining,
• nextTimeSlot() returns 0 with a fatal error

https://github.com/verilator/verilator/blob/c2483189580e0c0b9ba87a9087d1a1032ba5c011/src/V3EmitCModel.cpp#L451-L455

[imphil]

Rerunning CI against a master merge.

[mczyz-antmicro]

Hey,

• the return values are as @imphil claimed (observed them in my local runs)
• I've created a test case to confirm that deadlocks are stopped gracefully, I will push the testcase in a few minutes to a branch on fork, because I don't think that it is 100% compliant with contributing rules. Would be great if you could check it out and decide if you would like to merge it
• @imphil I will push the cosmetic update to (vluint64_t)~0ULL in a minute

My thoughts from the testcase (I am also attaching 2 log files from a test run with master branch and with patch branch) -
Claims:

• If there ARE timing structures in HDL code AND --timing IS NOT given, we expect:
  • fail on Verilation.
    • This is true based on make clean all runs
• If there ARE NO timing structure in HDL code AND --timing IS NOT given, we expect:
  • Clean pass if a clock driver is in the cocotb routine
  • SimFailure if there is no clock driver in the cocotb routine
    • This is true based on EXTRA_ARGS+=+define+TEST_CLK_EXTERNAL=1 make clean all runs
• If there ARE timing structres in HDL AND --timing IS given, we expect:
  • Master to fail with message %Error: /opt/verilator/share/verilator/include/verilated_timing.cpp:60: %Error: Encountered process that should've been resumed at an earlier simulation time. Missed a time slot? , which is the original issue
  • With patch the simulation doesnt fail or hang-up. The false negative in test_sim_fail is explained easily: there is now a clock driver in HDL code, so the await dut.clk command in cocotb coroutine yields no error, thus the simulation ends properly (test expected a fail).
    test_3316.clk_in_coroutine PASS
    test_3316.clk_in_hdl PASS
    test_3316.test_sim_failure_a FAIL
• If there ARE NO timing structres in HDL AND --timing IS given, we expect:
  • no impact to cocotb
    • confirmed by runs with EXTRA_ARGS+=+define+TEST_CLK_EXTERNAL=1 EXTRA_ARGS+=--timing make clean all

Conclusions:

• Simulator does not hang in any of the cases
• The patch only affects behaviour when "there ARE timing structres in HDL AND --timing IS given"
• Other pass/fails are all as expected

run_patch.log
run_master.log

I've also observed "a tangent" issue, which is that Verilation command is built in the cocotb.runner. I haven't found a way to pass "--timing" to it in a clean way. It would be nice to have another argument similar to build_cmds, but for other compilation args. This is why I used a makefile to run my tests and not build/test cocotb functions

[mczyz-antmicro]

• @imphil Added your suggestion in this commit
• Testcase is on this branch

I rebased both branches to latest master, too

[ktbarrett]

@mczyz-antmicro The test looks good but we don't really need both the Python runner and the Makefile for the test. The only issue I see is that it would be nice to timeout and kill the hung process so the rest of the regression can complete.

If you wanted to open a new PR with that test, it would be appreciated.

[imphil]

Thanks @mczyz-antmicro for your work on this PR! Thanks especially for the in-depth thoughts on testing your change in #3316 (comment), that's exceptional and very much appreciated.

[mczyz-antmicro]

I will follow up with testcase in #3333