From f320d82b62f4645b68711e9814a062822c915c19 Mon Sep 17 00:00:00 2001 From: Asherah Connor Date: Wed, 5 Jun 2024 17:38:57 +0300 Subject: [PATCH 1/6] simulator: add SourceInfo to expect calls and report. --- .../scala/chisel3/simulator/PeekPokeAPI.scala | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/main/scala/chisel3/simulator/PeekPokeAPI.scala b/src/main/scala/chisel3/simulator/PeekPokeAPI.scala index 854a64dedac..70e90d02486 100644 --- a/src/main/scala/chisel3/simulator/PeekPokeAPI.scala +++ b/src/main/scala/chisel3/simulator/PeekPokeAPI.scala @@ -3,11 +3,15 @@ package chisel3.simulator import svsim._ import chisel3._ +import chisel3.experimental.SourceInfo + 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") + case class FailedExpectationException[T](sourceInfo: SourceInfo, observed: T, expected: T, message: String) + extends Exception( + s"Failed Expectation: Observed value '$observed' != $expected. $message ${sourceInfo.makeMessage(x => x)}" + ) implicit class testableClock(clock: Clock) { def step(cycles: Int = 1): Unit = { @@ -56,24 +60,24 @@ trait PeekPokeAPI { } final def peek(): T = encode(data.peekValue()) - final def expect(expected: T): Unit = { + final def expect(expected: T)(implicit sourceInfo: SourceInfo): Unit = { data.expect( expected.litValue, encode(_).litValue, (observed: BigInt, expected: BigInt) => s"Expectation failed: observed value $observed != $expected" ) } - final def expect(expected: T, message: String): Unit = { + final def expect(expected: T, message: String)(implicit sourceInfo: SourceInfo): Unit = { data.expect(expected.litValue, encode(_).litValue, (_: BigInt, _: BigInt) => message) } - final def expect(expected: BigInt): Unit = { + final def expect(expected: BigInt)(implicit sourceInfo: SourceInfo): Unit = { data.expect( expected, _.asBigInt, (observed: BigInt, expected: BigInt) => s"Expectation failed: observed value $observed != $expected" ) } - final def expect(expected: BigInt, message: String): Unit = { + final def expect(expected: BigInt, message: String)(implicit sourceInfo: SourceInfo): Unit = { data.expect(expected, _.asBigInt, (_: BigInt, _: BigInt) => message) } @@ -126,13 +130,16 @@ trait PeekPokeAPI { expected: T, encode: (Simulation.Value) => T, buildMessage: (T, T) => String + )( + implicit 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) + throw FailedExpectationException(sourceInfo, observed, expected, buildMessage(observed, expected)) } } } From 6dc98b727b5677d9683ee2c61e96b04ceccf7845 Mon Sep 17 00:00:00 2001 From: Asherah Connor Date: Wed, 5 Jun 2024 17:39:13 +0300 Subject: [PATCH 2/6] simulator: add test for failed expects. --- .../chiselTests/simulator/SimulatorSpec.scala | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/test/scala/chiselTests/simulator/SimulatorSpec.scala b/src/test/scala/chiselTests/simulator/SimulatorSpec.scala index aca0b254eb7..c4f26aa782b 100644 --- a/src/test/scala/chiselTests/simulator/SimulatorSpec.scala +++ b/src/test/scala/chiselTests/simulator/SimulatorSpec.scala @@ -71,6 +71,25 @@ class SimulatorSpec extends AnyFunSpec with Matchers { assert(result === 12) } + it("reports failed expects correctly") { + val simulator = new VerilatorSimulator("test_run_dir/simulator/GCDSimulator") + a[PeekPokeAPI.FailedExpectationException[_]] must be 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 + } + } + it("runs a design that includes an external module") { class Bar extends ExtModule with HasExtModuleInline { val a = IO(Output(Bool())) From f4fb9af1a8b4bb3f8ecdb9d96b09b0fef2e8a86e Mon Sep 17 00:00:00 2001 From: Asherah Connor Date: Wed, 5 Jun 2024 18:38:03 +0300 Subject: [PATCH 3/6] simulator: attempt to extract source line. --- .../main/scala/chisel3/internal/Error.scala | 47 ++++++++++--------- .../scala/chisel3/simulator/PeekPokeAPI.scala | 33 +++++++++++-- .../chiselTests/simulator/SimulatorSpec.scala | 9 +++- 3 files changed, 60 insertions(+), 29 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 70e90d02486..cf6c1ff38ad 100644 --- a/src/main/scala/chisel3/simulator/PeekPokeAPI.scala +++ b/src/main/scala/chisel3/simulator/PeekPokeAPI.scala @@ -3,14 +3,22 @@ package chisel3.simulator import svsim._ import chisel3._ -import chisel3.experimental.SourceInfo +import chisel3.experimental.{SourceInfo, SourceLine} +import chisel3.internal.ExceptionHelpers object PeekPokeAPI extends PeekPokeAPI trait PeekPokeAPI { - case class FailedExpectationException[T](sourceInfo: SourceInfo, observed: T, expected: T, message: String) + case class FailedExpectationException[T]( + sourceInfo: SourceInfo, + extraContext: Seq[String], + observed: T, + expected: T, + message: String) extends Exception( - s"Failed Expectation: Observed value '$observed' != $expected. $message ${sourceInfo.makeMessage(x => x)}" + s"Failed Expectation: Observed value '$observed' != $expected. " + + s"$message ${sourceInfo.makeMessage(x => x)}" + + (if (extraContext.nonEmpty) s"\n${extraContext.mkString("\n")}" else "") ) implicit class testableClock(clock: Clock) { @@ -136,10 +144,25 @@ trait PeekPokeAPI { 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(sourceInfo, observed, expected, buildMessage(observed, expected)) + if (observed != expected) { + val extraContext = + sourceInfo match { + case sl: SourceLine => + ExceptionHelpers.getErrorLineInFile(Seq(), sl) + case _ => + Seq() + } + throw FailedExpectationException( + sourceInfo, + extraContext, + observed, + expected, + buildMessage(observed, expected) + ) + } } } } diff --git a/src/test/scala/chiselTests/simulator/SimulatorSpec.scala b/src/test/scala/chiselTests/simulator/SimulatorSpec.scala index c4f26aa782b..758cbb86023 100644 --- a/src/test/scala/chiselTests/simulator/SimulatorSpec.scala +++ b/src/test/scala/chiselTests/simulator/SimulatorSpec.scala @@ -73,7 +73,7 @@ class SimulatorSpec extends AnyFunSpec with Matchers { it("reports failed expects correctly") { val simulator = new VerilatorSimulator("test_run_dir/simulator/GCDSimulator") - a[PeekPokeAPI.FailedExpectationException[_]] must be thrownBy { + val thrown = the[PeekPokeAPI.FailedExpectationException[_]] thrownBy { simulator .simulate(new GCD()) { module => import PeekPokeAPI._ @@ -88,6 +88,13 @@ class SimulatorSpec extends AnyFunSpec with Matchers { } .result } + (thrown.getMessage must include).regex("Observed value '12' != 5\\.") + (thrown.getMessage must include).regex( + " @\\[src/test/scala/chiselTests/simulator/SimulatorSpec\\.scala \\d+:\\d+\\]" + ) + (thrown.getMessage must include).regex("gcd\\.io\\.result\\.expect\\(5\\)") + // Not actually anchoring this so it's not extremely brittle, only somewhat. + (thrown.getMessage must include).regex(" \\^") } it("runs a design that includes an external module") { From 891ed06462f0efc3a04dbcd051a7b43a37f86909 Mon Sep 17 00:00:00 2001 From: Asherah Connor Date: Thu, 6 Jun 2024 11:45:10 +0300 Subject: [PATCH 4/6] simulator: prefer strings to regexes. Co-authored-by: Jack Koenig --- src/test/scala/chiselTests/simulator/SimulatorSpec.scala | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/test/scala/chiselTests/simulator/SimulatorSpec.scala b/src/test/scala/chiselTests/simulator/SimulatorSpec.scala index 758cbb86023..93e4783874c 100644 --- a/src/test/scala/chiselTests/simulator/SimulatorSpec.scala +++ b/src/test/scala/chiselTests/simulator/SimulatorSpec.scala @@ -88,13 +88,12 @@ class SimulatorSpec extends AnyFunSpec with Matchers { } .result } - (thrown.getMessage must include).regex("Observed value '12' != 5\\.") + thrown.getMessage must include("Observed value '12' != 5.") (thrown.getMessage must include).regex( - " @\\[src/test/scala/chiselTests/simulator/SimulatorSpec\\.scala \\d+:\\d+\\]" + """ @\[src/test/scala/chiselTests/simulator/SimulatorSpec\.scala \d+:\d+\]""" ) - (thrown.getMessage must include).regex("gcd\\.io\\.result\\.expect\\(5\\)") - // Not actually anchoring this so it's not extremely brittle, only somewhat. - (thrown.getMessage must include).regex(" \\^") + thrown.getMessage must include("gcd.io.result.expect(5)") + thrown.getMessage must include(" ^") } it("runs a design that includes an external module") { From 3570d52238b3534df5c31295f413714d91c60fce Mon Sep 17 00:00:00 2001 From: Asherah Connor Date: Thu, 6 Jun 2024 11:49:15 +0300 Subject: [PATCH 5/6] simulator: make testableData.expect's sourceInfo parameter explicit. --- .../scala/chisel3/simulator/PeekPokeAPI.scala | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/main/scala/chisel3/simulator/PeekPokeAPI.scala b/src/main/scala/chisel3/simulator/PeekPokeAPI.scala index cf6c1ff38ad..67542ef400a 100644 --- a/src/main/scala/chisel3/simulator/PeekPokeAPI.scala +++ b/src/main/scala/chisel3/simulator/PeekPokeAPI.scala @@ -72,21 +72,23 @@ trait PeekPokeAPI { 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)(implicit sourceInfo: SourceInfo): Unit = { - data.expect(expected.litValue, encode(_).litValue, (_: BigInt, _: BigInt) => message) + data.expect(expected.litValue, encode(_).litValue, (_: BigInt, _: BigInt) => message, sourceInfo) } final def expect(expected: BigInt)(implicit 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)(implicit sourceInfo: SourceInfo): Unit = { - data.expect(expected, _.asBigInt, (_: BigInt, _: BigInt) => message) + data.expect(expected, _.asBigInt, (_: BigInt, _: BigInt) => message, sourceInfo) } } @@ -137,9 +139,8 @@ trait PeekPokeAPI { def expect[T]( expected: T, encode: (Simulation.Value) => T, - buildMessage: (T, T) => String - )( - implicit sourceInfo: SourceInfo + buildMessage: (T, T) => String, + sourceInfo: SourceInfo ): Unit = { val module = AnySimulatedModule.current module.willPeek() From 77ebff88bd9a24184ef0aa6ed3bc88a72cee4ce2 Mon Sep 17 00:00:00 2001 From: Asherah Connor Date: Thu, 6 Jun 2024 11:53:16 +0300 Subject: [PATCH 6/6] simulator: add factory method for giving failed expect sourceInfo/extraContext. Co-authored-by: Jack Koenig --- .../scala/chisel3/simulator/PeekPokeAPI.scala | 32 +++++++++++-------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/main/scala/chisel3/simulator/PeekPokeAPI.scala b/src/main/scala/chisel3/simulator/PeekPokeAPI.scala index 67542ef400a..6a5edc1166b 100644 --- a/src/main/scala/chisel3/simulator/PeekPokeAPI.scala +++ b/src/main/scala/chisel3/simulator/PeekPokeAPI.scala @@ -9,17 +9,21 @@ import chisel3.internal.ExceptionHelpers object PeekPokeAPI extends PeekPokeAPI trait PeekPokeAPI { - case class FailedExpectationException[T]( - sourceInfo: SourceInfo, - extraContext: Seq[String], - observed: T, - expected: T, - message: String) - extends Exception( - s"Failed Expectation: Observed value '$observed' != $expected. " + - s"$message ${sourceInfo.makeMessage(x => x)}" + - (if (extraContext.nonEmpty) s"\n${extraContext.mkString("\n")}" else "") - ) + 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 = { @@ -157,11 +161,11 @@ trait PeekPokeAPI { Seq() } throw FailedExpectationException( - sourceInfo, - extraContext, observed, expected, - buildMessage(observed, expected) + buildMessage(observed, expected), + sourceInfo, + extraContext ) } }