Fix issues in IPCSimulatorContext (VCS) simulations #570
Conversation
* IPCSimulatorContext: better synchronization on the logs * TestApplicationException: changes for more useful assertion failure messages
| try { | ||
| tester.finish() // needed to dump VCDs + terminate any external process | ||
| } catch { | ||
| case e: TestApplicationException => throw new ChiselAssertionError(s"Simulator exited sooner than expected. See logs for more information about what is assumed to be a Chisel Assertion which failed.") |
There was a problem hiding this comment.
I would prefer if the IPC mechanism could actually communicate if an assertion failed instead of the simulator crashing for some other reason similar to how the Verilator backend does it:
However, this is an improvement over the status quo, so I am happy to get it in for now.
There was a problem hiding this comment.
i wasn't sure how to determine. It would appear that VCS returns 0 even if assert fires. Talking to some folks here i was told its not common to look at VCS error code to determine if an assertion failed. SO this would involve parsing log messages??
There was a problem hiding this comment.
I am not sure what the best approach would be. Verilator allows you to hook into the assertion mechanism. Not sure if there is a similar API for VPI. This would be cool future work, but of course nothing that will hold up this PR.
| @@ -57,7 +57,11 @@ private[chiseltest] class IPCSimulatorContext( | |||
| private def startProcess(cmd: Seq[String], logs: ArrayBuffer[String], cwd: os.Path): Process = { | |||
| val processBuilder = Process(cmd, cwd = cwd.toIO) | |||
| // This makes everything written to stderr get added as lines to logs | |||
| val processLogger = ProcessLogger(println, logs += _) // don't log stdout | |||
| val processLogger = ProcessLogger(println, { str => | |||
There was a problem hiding this comment.
Does the lambda here have to be re-entrant? I did not know that.
There was a problem hiding this comment.
I think it's less about reentrancy and more about thread safety with the reads in dumpLogs below. We sometimes see "Mutation occurred during iteration" exceptions. This is the straightforward fix, an alternative would be to use a thread-safe Queue like java.util.concurrent.ConcurrentLinkedQueue
There was a problem hiding this comment.
I just wondered which other function is accessing the logs.
| inChannel.close() | ||
| outChannel.close() | ||
| cmdChannel.close() | ||
| dumpLogs() |
There was a problem hiding this comment.
would moving the dumpLongs command until after the channels are closed resolve the problem without adding the mutex?
There was a problem hiding this comment.
this change is not sufficient on its own. Is that what you are asking?
|
This does need a |
|
|
|
the CI failure looks like a verilator download issue...? Not sure how to re-launch CI |
You might not have the privileges to. The Mac CI stuff is very brittle and needs a couple of attempts. I will re-launch things until it all passes. Thanks for the contribution! |
ekiwi
added
the
Please Merge
* Fix issues in VCS simulations * IPCSimulatorContext: better synchronization on the logs * TestApplicationException: changes for more useful assertion failure messages * scalafmt Co-authored-by: Jack Koenig <[email protected]> (cherry picked from commit 5956488)
mergify
bot
added
the
Backported
* Fix issues in VCS simulations * IPCSimulatorContext: better synchronization on the logs * TestApplicationException: changes for more useful assertion failure messages * scalafmt Co-authored-by: Jack Koenig <[email protected]> (cherry picked from commit 5956488) Co-authored-by: Megan Wachs <[email protected]>
Fixes #569 and #568