From 26ae5a85815f3f5bf401886dcc083ba19c319e36 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Thu, 6 Jun 2024 10:01:01 -0700 Subject: [PATCH] PeekPokeAPI: include source location on failed expect() calls. (backport #4144) (#4149) * PeekPokeAPI: include source location on failed expect() calls. (#4144) * simulator: add SourceInfo to expect calls and report. * simulator: add test for failed expects. * simulator: attempt to extract source line. * simulator: make testableData.expect's sourceInfo parameter explicit. * simulator: add factory method for giving failed expect sourceInfo/extraContext. (cherry picked from commit 45dd82a70027c21b1615d79962c03652fe4fa6d1) # Conflicts: # src/test/scala/chiselTests/simulator/SimulatorSpec.scala * Resolve backport conflicts and binary compatibility issues * Run scalafmt --------- Co-authored-by: Asherah Connor Co-authored-by: Jack Koenig --- .../main/scala/chisel3/internal/Error.scala | 47 +++++------ .../scala/chisel3/simulator/PeekPokeAPI.scala | 79 ++++++++++++++++--- .../chiselTests/simulator/SimulatorSpec.scala | 25 ++++++ 3 files changed, 119 insertions(+), 32 deletions(-) diff --git a/core/src/main/scala/chisel3/internal/Error.scala b/core/src/main/scala/chisel3/internal/Error.scala index adca04e2760..f9708ad8652 100644 --- a/core/src/main/scala/chisel3/internal/Error.scala +++ b/core/src/main/scala/chisel3/internal/Error.scala @@ -29,6 +29,28 @@ object ExceptionHelpers { def ellipsis(message: Option[String] = None): StackTraceElement = new StackTraceElement("..", " ", message.getOrElse(""), -1) + private[chisel3] def getErrorLineInFile(sourceRoots: Seq[File], sl: SourceLine): List[String] = { + def tryFileInSourceRoot(sourceRoot: File): Option[List[String]] = { + try { + val file = new File(sourceRoot, sl.filename) + val lines = Source.fromFile(file).getLines() + var i = 0 + while (i < (sl.line - 1) && lines.hasNext) { + lines.next() + i += 1 + } + val line = lines.next() + val caretLine = (" " * (sl.col - 1)) + "^" + Some(line :: caretLine :: Nil) + } catch { + case scala.util.control.NonFatal(_) => None + } + } + val sourceRootsWithDefault = if (sourceRoots.nonEmpty) sourceRoots else Seq(new File(".")) + // View allows us to search the directories one at a time and early out + sourceRootsWithDefault.view.map(tryFileInSourceRoot(_)).collectFirst { case Some(value) => value }.getOrElse(Nil) + } + /** Utility methods that can be added to exceptions. */ implicit class ThrowableHelpers(throwable: Throwable) { @@ -254,28 +276,6 @@ private[chisel3] class ErrorLog( throwOnFirstError: Boolean) { import ErrorLog.withColor - private def getErrorLineInFile(sl: SourceLine): List[String] = { - def tryFileInSourceRoot(sourceRoot: File): Option[List[String]] = { - try { - val file = new File(sourceRoot, sl.filename) - val lines = Source.fromFile(file).getLines() - var i = 0 - while (i < (sl.line - 1) && lines.hasNext) { - lines.next() - i += 1 - } - val line = lines.next() - val caretLine = (" " * (sl.col - 1)) + "^" - Some(line :: caretLine :: Nil) - } catch { - case scala.util.control.NonFatal(_) => None - } - } - val sourceRootsWithDefault = if (sourceRoots.nonEmpty) sourceRoots else Seq(new File(".")) - // View allows us to search the directories one at a time and early out - sourceRootsWithDefault.view.map(tryFileInSourceRoot(_)).collectFirst { case Some(value) => value }.getOrElse(Nil) - } - /** Returns an appropriate location string for the provided source info. * If the source info is of `NoSourceInfo` type, the source location is looked up via stack trace. * If the source info is `None`, an empty string is returned. @@ -292,7 +292,8 @@ private[chisel3] class ErrorLog( // id is optional because it has only been applied to warnings, TODO apply to errors private def logWarningOrError(msg: String, si: Option[SourceInfo], isFatal: Boolean): Unit = { val location = errorLocationString(si) - val sourceLineAndCaret = si.collect { case sl: SourceLine => getErrorLineInFile(sl) }.getOrElse(Nil) + val sourceLineAndCaret = + si.collect { case sl: SourceLine => ExceptionHelpers.getErrorLineInFile(sourceRoots, sl) }.getOrElse(Nil) val fullMessage = if (location.isEmpty) msg else s"$location: $msg" val errorLines = fullMessage :: sourceLineAndCaret val entry = ErrorEntry(errorLines, isFatal) diff --git a/src/main/scala/chisel3/simulator/PeekPokeAPI.scala b/src/main/scala/chisel3/simulator/PeekPokeAPI.scala index 854a64dedac..f1282e7ff4e 100644 --- a/src/main/scala/chisel3/simulator/PeekPokeAPI.scala +++ b/src/main/scala/chisel3/simulator/PeekPokeAPI.scala @@ -3,11 +3,27 @@ package chisel3.simulator import svsim._ import chisel3._ +import chisel3.experimental.{SourceInfo, SourceLine, UnlocatableSourceInfo} +import chisel3.internal.ExceptionHelpers + object PeekPokeAPI extends PeekPokeAPI trait PeekPokeAPI { case class FailedExpectationException[T](observed: T, expected: T, message: String) extends Exception(s"Failed Expectation: Observed value '$observed' != $expected. $message") + object FailedExpectationException { + def apply[T]( + observed: T, + expected: T, + message: String, + sourceInfo: SourceInfo, + extraContext: Seq[String] + ): FailedExpectationException[T] = { + val fullMessage = s"$message ${sourceInfo.makeMessage(x => x)}" + + (if (extraContext.nonEmpty) s"\n${extraContext.mkString("\n")}" else "") + new FailedExpectationException(observed, expected, fullMessage) + } + } implicit class testableClock(clock: Clock) { def step(cycles: Int = 1): Unit = { @@ -56,25 +72,47 @@ trait PeekPokeAPI { } final def peek(): T = encode(data.peekValue()) - final def expect(expected: T): Unit = { + // Added for binary compatibility, not callable directly but can be called if compiled against old Chisel + private[simulator] final def expect(expected: T): Unit = _expect(expected, UnlocatableSourceInfo) + final def expect(expected: T)(implicit sourceInfo: SourceInfo): Unit = _expect(expected, sourceInfo) + // Added to avoid ambiguity errors when using binary compatibility shim + private def _expect(expected: T, sourceInfo: SourceInfo): Unit = { data.expect( expected.litValue, encode(_).litValue, - (observed: BigInt, expected: BigInt) => s"Expectation failed: observed value $observed != $expected" + (observed: BigInt, expected: BigInt) => s"Expectation failed: observed value $observed != $expected", + sourceInfo ) } - final def expect(expected: T, message: String): Unit = { - data.expect(expected.litValue, encode(_).litValue, (_: BigInt, _: BigInt) => message) + // Added for binary compatibility, not callable directly but can be called if compiled against old Chisel + private[simulator] def expect(expected: T, message: String): Unit = + _expect(expected, message, UnlocatableSourceInfo) + final def expect(expected: T, message: String)(implicit sourceInfo: SourceInfo): Unit = + _expect(expected, message, sourceInfo) + // Added to avoid ambiguity errors when using binary compatibility shim + private def _expect(expected: T, message: String, sourceInfo: SourceInfo): Unit = { + data.expect(expected.litValue, encode(_).litValue, (_: BigInt, _: BigInt) => message, sourceInfo) } - final def expect(expected: BigInt): Unit = { + // Added for binary compatibility, not callable directly but can be called if compiled against old Chisel + private[simulator] def expect(expected: BigInt): Unit = _expect(expected, UnlocatableSourceInfo) + final def expect(expected: BigInt)(implicit sourceInfo: SourceInfo): Unit = _expect(expected, sourceInfo) + // Added to avoid ambiguity errors when using binary compatibility shim + private def _expect(expected: BigInt, sourceInfo: SourceInfo): Unit = { data.expect( expected, _.asBigInt, - (observed: BigInt, expected: BigInt) => s"Expectation failed: observed value $observed != $expected" + (observed: BigInt, expected: BigInt) => s"Expectation failed: observed value $observed != $expected", + sourceInfo ) } - final def expect(expected: BigInt, message: String): Unit = { - data.expect(expected, _.asBigInt, (_: BigInt, _: BigInt) => message) + // Added for binary compatibility, not callable directly but can be called if compiled against old Chisel + private[simulator] def expect(expected: BigInt, message: String): Unit = + _expect(expected, message, UnlocatableSourceInfo) + final def expect(expected: BigInt, message: String)(implicit sourceInfo: SourceInfo): Unit = + _expect(expected, message, sourceInfo) + // Added to avoid ambiguity errors when using binary compatibility shim + private def _expect(expected: BigInt, message: String, sourceInfo: SourceInfo): Unit = { + data.expect(expected, _.asBigInt, (_: BigInt, _: BigInt) => message, sourceInfo) } } @@ -122,17 +160,40 @@ trait PeekPokeAPI { val simulationPort = module.port(data) simulationPort.get(isSigned = isSigned) } + @deprecated("Use version that takes a SourceInfo", "Chisel 6.5.0") def expect[T]( expected: T, encode: (Simulation.Value) => T, buildMessage: (T, T) => String + ): Unit = expect(expected, encode, buildMessage) + def expect[T]( + expected: T, + encode: (Simulation.Value) => T, + buildMessage: (T, T) => String, + sourceInfo: SourceInfo ): Unit = { val module = AnySimulatedModule.current module.willPeek() val simulationPort = module.port(data) + simulationPort.check(isSigned = isSigned) { observedValue => val observed = encode(observedValue) - if (observed != expected) throw FailedExpectationException(observed, expected, buildMessage(observed, expected)) + if (observed != expected) { + val extraContext = + sourceInfo match { + case sl: SourceLine => + ExceptionHelpers.getErrorLineInFile(Seq(), sl) + case _ => + Seq() + } + throw FailedExpectationException( + observed, + expected, + buildMessage(observed, expected), + sourceInfo, + extraContext + ) + } } } } diff --git a/src/test/scala/chiselTests/simulator/SimulatorSpec.scala b/src/test/scala/chiselTests/simulator/SimulatorSpec.scala index 8189dd97abb..30e3dafdd70 100644 --- a/src/test/scala/chiselTests/simulator/SimulatorSpec.scala +++ b/src/test/scala/chiselTests/simulator/SimulatorSpec.scala @@ -70,6 +70,31 @@ class SimulatorSpec extends AnyFunSpec with Matchers { assert(result === 12) } + it("reports failed expects correctly") { + val simulator = new VerilatorSimulator("test_run_dir/simulator/GCDSimulator") + val thrown = the[PeekPokeAPI.FailedExpectationException[_]] thrownBy { + simulator + .simulate(new GCD()) { module => + import PeekPokeAPI._ + val gcd = module.wrapped + gcd.io.a.poke(24.U) + gcd.io.b.poke(36.U) + gcd.io.loadValues.poke(1.B) + gcd.clock.step() + gcd.io.loadValues.poke(0.B) + gcd.clock.step(10) + gcd.io.result.expect(5) + } + .result + } + thrown.getMessage must include("Observed value '12' != 5.") + (thrown.getMessage must include).regex( + """ @\[src/test/scala/chiselTests/simulator/SimulatorSpec\.scala \d+:\d+\]""" + ) + thrown.getMessage must include("gcd.io.result.expect(5)") + thrown.getMessage must include(" ^") + } + it("simulate a circuit with zero-width ports") { val width = 0 // Run a simulation with zero width foo