chipsalliance / chisel

Chisel: A Modern Hardware Design Language
https://www.chisel-lang.org/
Apache License 2.0
3.99k stars 597 forks source link

[svsim] Handle Chisel assertions more elegantly #4386

Open jackkoenig opened 2 months ago

jackkoenig commented 2 months ago

This is a bugfix but I'm not sure if it should be backported [yet] because adding new arguments, even with default arguments, to public methods breaks binary compatibility. I don't feel like dealing with that at the moment. If we need to backport it later, we can. We could backport just the bugfix part of it I guess.

The returned exception is now a lot more elegant than svsim.Simulation.UnexpectedEndOfMessages. This also fixes an issue where the Verilator crashes could create core dumps depending on the user's ulimit settings.

I have not yet tested this with VCS, so there may be follow up work (possibly in a follow up PR).

Contributor Checklist

Type of Improvement

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

jackkoenig commented 2 months ago

What's weird is that these failing tests are currently printing failures and maybe even crashing on main, but for some reason SVSim returns success. I think my changes are just exposing some other underlying issue. I had been planning to revisit how we deal with $stop in a follow on PR, but it might just have to be done in this PR as well.

jackkoenig commented 1 month ago

Thanks for the review @dobios! I think I sadly have to bite the bullet and also implement $stop for this to pass CI... will revisit eventually 😭