Improve error reporting (backport #2376) (#2379)

* Improve error reporting (#2376)

* Do not trim stack traces of exceptions with no stack trace

This prevents us from accidentally giving stack traces to exceptions
that don't have them and giving misleading messages telling users to use
--full-stacktrace when it won't actually do anything.

Also deprecate ChiselException.chiselStackTrace which is no longer being
used anywhere in this codebase.

* Add exception class for multiple-errors reported

New chisel3.internal.Errors replaces old anonymous class that would show
up as chisel3.internal.ErrorLog$$anon$1 in error messages.

* Add new option --throw-on-first-error

This tells Chisel not to aggregate recoverable errors but instead to
throw an exception on the first one. This gives a stack trace for users
who need it for debugging.

(cherry picked from commit ff2e9c92247b3848659fa09fdd53ddde2120036a)

* Waive MiMa false positives

The waived change is to a package private constructor.

Co-authored-by: Jack Koenig <koenig@sifive.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

11 files changed, 176 insertions, 39 deletions

diff --git a/build.sbt b/build.sbt
index f5f178fd..e63496a5 100644
--- a/build.sbt
+++ b/build.sbt
@@ -1,5 +1,7 @@
 // See LICENSE for license details.
 
+import com.typesafe.tools.mima.core._
+
 enablePlugins(SiteScaladocPlugin)
 
 val defaultVersions = Map(
@@ -197,6 +199,10 @@ lazy val chisel = (project in file(".")).
   aggregate(macros, core, plugin).
   settings(
     mimaPreviousArtifacts := Set("edu.berkeley.cs" %% "chisel3" % "3.5.0"),
+    mimaBinaryIssueFilters ++= Seq(
+      // Modified package private methods (https://github.com/lightbend/mima/issues/53)
+      ProblemFilters.exclude[DirectMissingMethodProblem]("chisel3.stage.ChiselOptions.this"),
+    ),
     libraryDependencies += defaultVersions("treadle") % "test",
     Test / scalacOptions += "-P:chiselplugin:genBundleElements",
     scalacOptions in Test ++= Seq("-language:reflectiveCalls"),
diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala b/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala
index b498daf0..59b4c692 100644
--- a/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala
+++ b/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala
@@ -100,7 +100,7 @@ object Definition extends SourceInfoDoc {
     implicit sourceInfo: SourceInfo,
     compileOptions:      CompileOptions
   ): Definition[T] = {
-    val dynamicContext = new DynamicContext(Nil)
+    val dynamicContext = new DynamicContext(Nil, Builder.captureContext().throwOnFirstError)
     Builder.globalNamespace.copyTo(dynamicContext.globalNamespace)
     dynamicContext.inDefinition = true
     val (ir, module) = Builder.build(Module(proto), dynamicContext, false)
diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala
index fb6ebcc7..a00505a8 100644
--- a/core/src/main/scala/chisel3/internal/Builder.scala
+++ b/core/src/main/scala/chisel3/internal/Builder.scala
@@ -361,7 +361,7 @@ private[chisel3] class ChiselContext() {
   val viewNamespace = Namespace.empty
 }
 
-private[chisel3] class DynamicContext(val annotationSeq: AnnotationSeq) {
+private[chisel3] class DynamicContext(val annotationSeq: AnnotationSeq, val throwOnFirstError: Boolean) {
   val globalNamespace = Namespace.empty
   val components = ArrayBuffer[Component]()
   val annotations = ArrayBuffer[ChiselAnnotation]()
@@ -653,7 +653,8 @@ private[chisel3] object Builder extends LazyLogging {
 
   def errors: ErrorLog = dynamicContext.errors
   def error(m: => String): Unit = {
-    if (dynamicContextVar.value.isDefined) {
+    // If --throw-on-first-error is requested, throw an exception instead of aggregating errors
+    if (dynamicContextVar.value.isDefined && !dynamicContextVar.value.get.throwOnFirstError) {
       errors.error(m)
     } else {
       throwException(m)
diff --git a/core/src/main/scala/chisel3/internal/Error.scala b/core/src/main/scala/chisel3/internal/Error.scala
index c42f39ed..62086870 100644
--- a/core/src/main/scala/chisel3/internal/Error.scala
+++ b/core/src/main/scala/chisel3/internal/Error.scala
@@ -4,6 +4,7 @@ package chisel3.internal
 
 import scala.annotation.tailrec
 import scala.collection.mutable.{ArrayBuffer, LinkedHashMap}
+import scala.util.control.NoStackTrace
 import _root_.logger.Logger
 
 object ExceptionHelpers {
@@ -46,31 +47,36 @@ object ExceptionHelpers {
       }
 
       // Step 1: Remove elements from the top in the package trimlist
-      ((a: Array[StackTraceElement]) => a.dropWhile(inTrimlist))
-        // Step 2: Optionally remove elements from the bottom until the anchor
-        .andThen(_.reverse)
-        .andThen(a =>
-          anchor match {
-            case Some(b) => a.dropWhile(ste => !ste.getClassName.startsWith(b))
-            case None    => a
-          }
-        )
-        // Step 3: Remove elements from the bottom in the package trimlist
-        .andThen(_.dropWhile(inTrimlist))
-        // Step 4: Reverse back to the original order
-        .andThen(_.reverse.toArray)
-        // Step 5: Add ellipsis stack trace elements and "--full-stacktrace" info
-        .andThen(a =>
-          ellipsis() +:
-            a :+
-            ellipsis() :+
-            ellipsis(
-              Some("Stack trace trimmed to user code only. Rerun with --full-stacktrace to see the full stack trace")
-            )
-        )
-        // Step 5: Mutate the stack trace in this exception
-        .andThen(throwable.setStackTrace(_))
-        .apply(throwable.getStackTrace)
+      val trimStackTrace =
+        ((a: Array[StackTraceElement]) => a.dropWhile(inTrimlist))
+          // Step 2: Optionally remove elements from the bottom until the anchor
+          .andThen(_.reverse)
+          .andThen(a =>
+            anchor match {
+              case Some(b) => a.dropWhile(ste => !ste.getClassName.startsWith(b))
+              case None    => a
+            }
+          )
+          // Step 3: Remove elements from the bottom in the package trimlist
+          .andThen(_.dropWhile(inTrimlist))
+          // Step 4: Reverse back to the original order
+          .andThen(_.reverse.toArray)
+          // Step 5: Add ellipsis stack trace elements and "--full-stacktrace" info
+          .andThen(a =>
+            ellipsis() +:
+              a :+
+              ellipsis() :+
+              ellipsis(
+                Some("Stack trace trimmed to user code only. Rerun with --full-stacktrace to see the full stack trace")
+              )
+          )
+          // Step 5: Mutate the stack trace in this exception
+          .andThen(throwable.setStackTrace(_))
+
+      val stackTrace = throwable.getStackTrace
+      if (stackTrace.nonEmpty) {
+        trimStackTrace(stackTrace)
+      }
     }
 
   }
@@ -80,11 +86,11 @@ object ExceptionHelpers {
 class ChiselException(message: String, cause: Throwable = null) extends Exception(message, cause, true, true) {
 
   /** Package names whose stack trace elements should be trimmed when generating a trimmed stack trace */
-  @deprecated("Use ExceptionHelpers.packageTrimlist. This will be removed in Chisel 3.6", "3.5")
+  @deprecated("Use ExceptionHelpers.packageTrimlist. This will be removed in Chisel 3.6", "Chisel 3.5")
   val blacklistPackages: Set[String] = Set("chisel3", "scala", "java", "sun", "sbt")
 
   /** The object name of Chisel's internal `Builder`. Everything stack trace element after this will be trimmed. */
-  @deprecated("Use ExceptionHelpers.builderName. This will be removed in Chisel 3.6", "3.5")
+  @deprecated("Use ExceptionHelpers.builderName. This will be removed in Chisel 3.6", "Chisel 3.5")
   val builderName: String = chisel3.internal.Builder.getClass.getName
 
   /** Examine a [[Throwable]], to extract all its causes. Innermost cause is first.
@@ -138,6 +144,11 @@ class ChiselException(message: String, cause: Throwable = null) extends Exceptio
     trimmedReverse.toIndexedSeq.reverse.toArray
   }
 
+  @deprecated(
+    "Use extension method trimStackTraceToUserCode defined in ExceptionHelpers.ThrowableHelpers. " +
+      "This will be removed in Chisel 3.6",
+    "Chisel 3.5.0"
+  )
   def chiselStackTrace: String = {
     val trimmed = trimmedStackTrace(likelyCause)
 
@@ -151,6 +162,7 @@ class ChiselException(message: String, cause: Throwable = null) extends Exceptio
     sw.toString
   }
 }
+private[chisel3] class Errors(message: String) extends ChiselException(message) with NoStackTrace
 
 private[chisel3] object throwException {
   def apply(s: String, t: Throwable = null): Nothing =
@@ -234,8 +246,10 @@ private[chisel3] class ErrorLog {
     }
 
     if (!allErrors.isEmpty) {
-      throw new ChiselException("Fatal errors during hardware elaboration. Look above for error list.")
-        with scala.util.control.NoStackTrace
+      throw new Errors(
+        "Fatal errors during hardware elaboration. Look above for error list. " +
+          "Rerun with --throw-on-first-error if you wish to see a stack trace."
+      )
     } else {
       // No fatal errors, clear accumulated warnings since they've been reported
       errors.clear()
diff --git a/src/main/scala/chisel3/aop/injecting/InjectingAspect.scala b/src/main/scala/chisel3/aop/injecting/InjectingAspect.scala
index abe208cf..ba873c23 100644
--- a/src/main/scala/chisel3/aop/injecting/InjectingAspect.scala
+++ b/src/main/scala/chisel3/aop/injecting/InjectingAspect.scala
@@ -6,9 +6,10 @@ import chisel3.{withClockAndReset, Module, ModuleAspect, RawModule}
 import chisel3.aop._
 import chisel3.internal.{Builder, DynamicContext}
 import chisel3.internal.firrtl.DefModule
-import chisel3.stage.DesignAnnotation
+import chisel3.stage.{ChiselOptions, DesignAnnotation}
 import firrtl.annotations.ModuleTarget
 import firrtl.stage.RunFirrtlTransformAnnotation
+import firrtl.options.Viewer.view
 import firrtl.{ir, _}
 
 import scala.collection.mutable
@@ -56,7 +57,8 @@ abstract class InjectorAspect[T <: RawModule, M <: RawModule](
     */
   final def toAnnotation(modules: Iterable[M], circuit: String, moduleNames: Seq[String]): AnnotationSeq = {
     RunFirrtlTransformAnnotation(new InjectingTransform) +: modules.map { module =>
-      val dynamicContext = new DynamicContext(annotationsInAspect)
+      val chiselOptions = view[ChiselOptions](annotationsInAspect)
+      val dynamicContext = new DynamicContext(annotationsInAspect, chiselOptions.throwOnFirstError)
       // Add existing module names into the namespace. If injection logic instantiates new modules
       //  which would share the same name, they will get uniquified accordingly
       moduleNames.foreach { n =>
diff --git a/src/main/scala/chisel3/stage/ChiselAnnotations.scala b/src/main/scala/chisel3/stage/ChiselAnnotations.scala
index c5811813..e046319d 100644
--- a/src/main/scala/chisel3/stage/ChiselAnnotations.scala
+++ b/src/main/scala/chisel3/stage/ChiselAnnotations.scala
@@ -61,6 +61,24 @@ case object PrintFullStackTraceAnnotation
 
 }
 
+/** On recoverable errors, this will cause Chisel to throw an exception instead of continuing.
+  */
+case object ThrowOnFirstErrorAnnotation
+    extends NoTargetAnnotation
+    with ChiselOption
+    with HasShellOptions
+    with Unserializable {
+
+  val options = Seq(
+    new ShellOption[Unit](
+      longOption = "throw-on-first-error",
+      toAnnotationSeq = _ => Seq(ThrowOnFirstErrorAnnotation),
+      helpText = "Throw an exception on the first error instead of continuing"
+    )
+  )
+
+}
+
 /** An [[firrtl.annotations.Annotation]] storing a function that returns a Chisel module
   * @param gen a generator function
   */
diff --git a/src/main/scala/chisel3/stage/ChiselCli.scala b/src/main/scala/chisel3/stage/ChiselCli.scala
index 26b032bf..f38bf50c 100644
--- a/src/main/scala/chisel3/stage/ChiselCli.scala
+++ b/src/main/scala/chisel3/stage/ChiselCli.scala
@@ -9,6 +9,7 @@ trait ChiselCli { this: Shell =>
   Seq(
     NoRunFirrtlCompilerAnnotation,
     PrintFullStackTraceAnnotation,
+    ThrowOnFirstErrorAnnotation,
     ChiselOutputFileAnnotation,
     ChiselGeneratorAnnotation
   )
diff --git a/src/main/scala/chisel3/stage/ChiselOptions.scala b/src/main/scala/chisel3/stage/ChiselOptions.scala
index ed4d0a2f..7e4305fa 100644
--- a/src/main/scala/chisel3/stage/ChiselOptions.scala
+++ b/src/main/scala/chisel3/stage/ChiselOptions.scala
@@ -7,12 +7,14 @@ import chisel3.internal.firrtl.Circuit
 class ChiselOptions private[stage] (
   val runFirrtlCompiler:   Boolean = true,
   val printFullStackTrace: Boolean = false,
+  val throwOnFirstError:   Boolean = false,
   val outputFile:          Option[String] = None,
   val chiselCircuit:       Option[Circuit] = None) {
 
   private[stage] def copy(
     runFirrtlCompiler:   Boolean = runFirrtlCompiler,
     printFullStackTrace: Boolean = printFullStackTrace,
+    throwOnFirstError:   Boolean = throwOnFirstError,
     outputFile:          Option[String] = outputFile,
     chiselCircuit:       Option[Circuit] = chiselCircuit
   ): ChiselOptions = {
@@ -20,6 +22,7 @@ class ChiselOptions private[stage] (
     new ChiselOptions(
       runFirrtlCompiler = runFirrtlCompiler,
       printFullStackTrace = printFullStackTrace,
+      throwOnFirstError = throwOnFirstError,
       outputFile = outputFile,
       chiselCircuit = chiselCircuit
     )
diff --git a/src/main/scala/chisel3/stage/package.scala b/src/main/scala/chisel3/stage/package.scala
index bf03e2df..b1064c05 100644
--- a/src/main/scala/chisel3/stage/package.scala
+++ b/src/main/scala/chisel3/stage/package.scala
@@ -15,8 +15,9 @@ package object stage {
     def view(options: AnnotationSeq): ChiselOptions = options.collect { case a: ChiselOption => a }
       .foldLeft(new ChiselOptions()) { (c, x) =>
         x match {
-          case _: NoRunFirrtlCompilerAnnotation.type => c.copy(runFirrtlCompiler = false)
-          case _: PrintFullStackTraceAnnotation.type => c.copy(printFullStackTrace = true)
+          case NoRunFirrtlCompilerAnnotation => c.copy(runFirrtlCompiler = false)
+          case PrintFullStackTraceAnnotation => c.copy(printFullStackTrace = true)
+          case ThrowOnFirstErrorAnnotation   => c.copy(throwOnFirstError = true)
           case ChiselOutputFileAnnotation(f) => c.copy(outputFile = Some(f))
           case ChiselCircuitAnnotation(a)    => c.copy(chiselCircuit = Some(a))
         }
diff --git a/src/main/scala/chisel3/stage/phases/Elaborate.scala b/src/main/scala/chisel3/stage/phases/Elaborate.scala
index 2cfb3200..55331cb4 100644
--- a/src/main/scala/chisel3/stage/phases/Elaborate.scala
+++ b/src/main/scala/chisel3/stage/phases/Elaborate.scala
@@ -5,7 +5,13 @@ package chisel3.stage.phases
 import chisel3.Module
 import chisel3.internal.ExceptionHelpers.ThrowableHelpers
 import chisel3.internal.{Builder, DynamicContext}
-import chisel3.stage.{ChiselCircuitAnnotation, ChiselGeneratorAnnotation, ChiselOptions, DesignAnnotation}
+import chisel3.stage.{
+  ChiselCircuitAnnotation,
+  ChiselGeneratorAnnotation,
+  ChiselOptions,
+  DesignAnnotation,
+  ThrowOnFirstErrorAnnotation
+}
 import firrtl.AnnotationSeq
 import firrtl.options.Phase
 import firrtl.options.Viewer.view
@@ -21,14 +27,16 @@ class Elaborate extends Phase {
 
   def transform(annotations: AnnotationSeq): AnnotationSeq = annotations.flatMap {
     case ChiselGeneratorAnnotation(gen) =>
+      val chiselOptions = view[ChiselOptions](annotations)
       try {
-        val (circuit, dut) = Builder.build(Module(gen()), new DynamicContext(annotations))
+        val (circuit, dut) =
+          Builder.build(Module(gen()), new DynamicContext(annotations, chiselOptions.throwOnFirstError))
         Seq(ChiselCircuitAnnotation(circuit), DesignAnnotation(dut))
       } catch {
         /* if any throwable comes back and we're in "stack trace trimming" mode, then print an error and trim the stack trace
          */
         case scala.util.control.NonFatal(a) =>
-          if (!view[ChiselOptions](annotations).printFullStackTrace) {
+          if (!chiselOptions.printFullStackTrace) {
             a.trimStackTraceToUserCode()
           }
           throw (a)
diff --git a/src/test/scala/chiselTests/stage/ChiselStageSpec.scala b/src/test/scala/chiselTests/stage/ChiselStageSpec.scala
index e88a2385..f0f383da 100644
--- a/src/test/scala/chiselTests/stage/ChiselStageSpec.scala
+++ b/src/test/scala/chiselTests/stage/ChiselStageSpec.scala
@@ -34,6 +34,14 @@ object ChiselStageSpec {
     assert(false, "User threw an exception")
   }
 
+  class UserExceptionNoStackTrace extends RawModule {
+    throw new Exception("Something bad happened") with scala.util.control.NoStackTrace
+  }
+
+  class RecoverableError extends RawModule {
+    3.U >> -1
+  }
+
 }
 
 class ChiselStageSpec extends AnyFlatSpec with Matchers with Utils {
@@ -169,6 +177,32 @@ class ChiselStageSpec extends AnyFlatSpec with Matchers with Utils {
     (exception.getStackTrace.mkString("\n") should not).include("java")
   }
 
+  it should "NOT add a stack trace to an exception with no stack trace" in {
+    val exception = intercept[java.lang.Exception] {
+      ChiselStage.emitChirrtl(new UserExceptionNoStackTrace)
+    }
+
+    val message = exception.getMessage
+    info("The exception includes the user's message")
+    message should include("Something bad happened")
+
+    info("The exception should not contain a stack trace")
+    exception.getStackTrace should be(Array())
+  }
+
+  it should "NOT include a stack trace for recoverable errors" in {
+    val exception = intercept[java.lang.Exception] {
+      ChiselStage.emitChirrtl(new RecoverableError)
+    }
+
+    val message = exception.getMessage
+    info("The exception includes the standard error message")
+    message should include("Fatal errors during hardware elaboration. Look above for error list.")
+
+    info("The exception should not contain a stack trace")
+    exception.getStackTrace should be(Array())
+  }
+
   behavior.of("ChiselStage exception handling")
 
   it should "truncate a user exception" in {
@@ -207,4 +241,53 @@ class ChiselStageSpec extends AnyFlatSpec with Matchers with Utils {
     exception.getStackTrace.mkString("\n") should include("java")
   }
 
+  it should "NOT add a stack trace to an exception with no stack trace" in {
+    val exception = intercept[java.lang.Exception] {
+      (new ChiselStage).emitChirrtl(new UserExceptionNoStackTrace)
+    }
+
+    val message = exception.getMessage
+    info("The exception includes the user's message")
+    message should include("Something bad happened")
+
+    info("The exception should not contain a stack trace")
+    exception.getStackTrace should be(Array())
+  }
+
+  it should "NOT include a stack trace for recoverable errors" in {
+    val exception = intercept[java.lang.Exception] {
+      (new ChiselStage).emitChirrtl(new RecoverableError)
+    }
+
+    val message = exception.getMessage
+    info("The exception includes the standard error message")
+    message should include("Fatal errors during hardware elaboration. Look above for error list.")
+
+    info("The exception should not contain a stack trace")
+    exception.getStackTrace should be(Array())
+  }
+
+  it should "include a stack trace for recoverable errors with '--throw-on-first-error'" in {
+    val exception = intercept[java.lang.Exception] {
+      (new ChiselStage).emitChirrtl(new RecoverableError, Array("--throw-on-first-error"))
+    }
+
+    val stackTrace = exception.getStackTrace.mkString("\n")
+    info("The exception should contain a truncated stack trace")
+    stackTrace shouldNot include("java")
+
+    info("The stack trace include information about running --full-stacktrace")
+    stackTrace should include("--full-stacktrace")
+  }
+
+  it should "include an untruncated stack trace for recoverable errors when given both '--throw-on-first-error' and '--full-stacktrace'" in {
+    val exception = intercept[java.lang.Exception] {
+      val args = Array("--throw-on-first-error", "--full-stacktrace")
+      (new ChiselStage).emitChirrtl(new RecoverableError, args)
+    }
+
+    val stackTrace = exception.getStackTrace.mkString("\n")
+    info("The exception should contain a truncated stack trace")
+    stackTrace should include("java")
+  }
 }