Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error reporting #2376

Merged
merged 3 commits into from
Feb 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
5 changes: 3 additions & 2 deletions core/src/main/scala/chisel3/internal/Builder.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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]()
Expand Down Expand Up @@ -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)
Expand Down
72 changes: 43 additions & 29 deletions core/src/main/scala/chisel3/internal/Error.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
}
jackkoenig marked this conversation as resolved.
Show resolved Hide resolved
}

}
Expand All @@ -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.
Expand Down Expand Up @@ -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)

Expand All @@ -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 =
Expand Down Expand Up @@ -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()
Expand Down
6 changes: 4 additions & 2 deletions src/main/scala/chisel3/aop/injecting/InjectingAspect.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 =>
Expand Down
18 changes: 18 additions & 0 deletions src/main/scala/chisel3/stage/ChiselAnnotations.scala
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,24 @@ case object PrintFullStackTraceAnnotation

}

/** On recoverable errors, this will cause Chisel to throw an exception instead of continuing.
*/
case object ThrowOnFirstErrorAnnotation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this expected to be used as an annotation? If not, then this can be made private[chisel3.stage].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be needed by chiseltest which only exposes Annotations and not arguments, but good suggestion in general

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
*/
Expand Down
1 change: 1 addition & 0 deletions src/main/scala/chisel3/stage/ChiselCli.scala
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ trait ChiselCli { this: Shell =>
Seq(
NoRunFirrtlCompilerAnnotation,
PrintFullStackTraceAnnotation,
ThrowOnFirstErrorAnnotation,
ChiselOutputFileAnnotation,
ChiselGeneratorAnnotation
)
Expand Down
3 changes: 3 additions & 0 deletions src/main/scala/chisel3/stage/ChiselOptions.scala
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,22 @@ 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 = {

new ChiselOptions(
runFirrtlCompiler = runFirrtlCompiler,
printFullStackTrace = printFullStackTrace,
throwOnFirstError = throwOnFirstError,
outputFile = outputFile,
chiselCircuit = chiselCircuit
)
Expand Down
5 changes: 3 additions & 2 deletions src/main/scala/chisel3/stage/package.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
Expand Down
14 changes: 11 additions & 3 deletions src/main/scala/chisel3/stage/phases/Elaborate.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand Down
83 changes: 83 additions & 0 deletions src/test/scala/chiselTests/stage/ChiselStageSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: only elaborate should be needed here (though there is no cost to using emitChirrtl.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that class ChiselStage doesn't have elaborate 🤦‍♂️

}

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")
}
}