PeekPokeAPI: include source location on failed expect() calls. (backp…

…ort #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 45dd82a) # Conflicts: # src/test/scala/chiselTests/simulator/SimulatorSpec.scala * Resolve backport conflicts and binary compatibility issues * Run scalafmt --------- Co-authored-by: Asherah Connor <[email protected]> Co-authored-by: Jack Koenig <[email protected]>
chipsalliance · Jun 6, 2024 · 26ae5a8 · 26ae5a8
1 parent fd3ac01
commit 26ae5a8
Show file tree

Hide file tree

Showing 3 changed files with 119 additions and 32 deletions.
diff --git 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
@@ -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
@@ -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