[svsim] Handle Chisel assertions more elegantly #4386
Conversation
jackkoenig
force-pushed
the
jackkoenig/svsim-handle-fatal
branch
from
438eeb8
to
5992789
Compare
* svsim now overrides Verilators $fatal handling to more elegantly report the error. * svsim can now accept a java.io.OutputStream to consume the stderr from the simulation process. This is especially useful for testing. * svsim will throw Simulation.Message.Error on assertion failures rather than Simulation.UnexpectedEndOfMessages.
jackkoenig
force-pushed
the
jackkoenig/svsim-handle-fatal
branch
from
5992789
to
22e055e
Compare
|
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 |
| // Fire and forget a thread that forwards the process's stderr. | ||
| // This thread will block on the processes error stream until it is closed, then the thread will complete. | ||
| implicit val e: ExecutionContext = ExecutionContext.global | ||
| Future { | ||
| // TODO use transferTo once we require Java 9 or newer. | ||
| val BufSize = 8192 // Taken from java.io.InputStream.DEFAULT_BUFFER_SIZE. | ||
| val err = process.getErrorStream() | ||
| val buffer = new Array[Byte](BufSize) | ||
| var read = 0 | ||
| while ({ | ||
| read = err.read(buffer, 0, BufSize) | ||
| read >= 0 | ||
| }) { | ||
| stderrStream.write(buffer, 0, read) | ||
| } | ||
| } |
There was a problem hiding this comment.
kind of a drive-by comment, but is there a particular reason why you're handling this as a Java Stream rather than using Scala's nice streaming data-types and stream programming constructs? Is there non Scala equivalent to Console.err that you can use?
There was a problem hiding this comment.
I think Scala's streaming constructs are generally for streaming data, whereas for process-level I/O streams I think it's still pretty typical to just use java.io streams (as evidenced by the fact that scala.Console.err is a java.io.OutputStream). I guess I'm not aware of any advantage to wrapping this in a Scala type but if anyone has a suggestion I'm all ears 🙂.
I could use System.err, I'm not really sure when one should use Java's System vs. Scala's Console for these sorts of things, but we often use things like Console.withOut to capture stdout, so I figured I'd go with the Scala API.
|
Thanks for the review @dobios! I think I sadly have to bite the bullet and also implement |
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
docs/src?Type of Improvement
Desired Merge Strategy
Release Notes
Reviewer Checklist (only modified by reviewer)
3.6.x,5.x, or6.xdepending on impact, API modification or big change:7.0)?Enable auto-merge (squash), clean up the commit message, and label withPlease Merge.Create a merge commit.