Merge pull request #1160 from freechipsproject/issue-1159

Fix Stack Trace Trimming in Driver

7 files changed, 262 insertions, 37 deletions

diff --git a/src/main/scala/chisel3/Driver.scala b/src/main/scala/chisel3/Driver.scala
index a78cc92f..158ba65a 100644
--- a/src/main/scala/chisel3/Driver.scala
+++ b/src/main/scala/chisel3/Driver.scala
@@ -6,7 +6,7 @@ import chisel3.internal.ErrorLog
 import chisel3.experimental.RawModule
 import internal.firrtl._
 import firrtl._
-import firrtl.options.{Phase, PhaseManager}
+import firrtl.options.{Phase, PhaseManager, StageError}
 import firrtl.options.phases.DeletedWrapper
 import firrtl.options.Viewer.view
 import firrtl.annotations.JsonProtocol
@@ -226,16 +226,10 @@ object Driver extends BackendCompilationUtilities {
     val annosx = try {
       phases.foldLeft(annos)( (a, p) => p.transform(a) )
     } catch {
-      case ce: ChiselException =>
-        val stackTrace = if (!optionsManager.chiselOptions.printFullStackTrace) {
-          ce.chiselStackTrace
-        } else {
-          val sw = new StringWriter
-          ce.printStackTrace(new PrintWriter(sw))
-          sw.toString
-        }
-        Predef.augmentString(stackTrace).lines.foreach(line => println(s"${ErrorLog.errTag} $line")) // scalastyle:ignore regex line.size.limit
-        annos
+      /* ChiselStage and FirrtlStage can throw StageError. Since Driver is not a StageMain, it cannot catch these. While
+       * Driver is deprecated and removed in 3.2.1+, the Driver catches all errors.
+       */
+      case e: StageError => annos
     }
 
     view[ChiselExecutionResult](annosx)
diff --git a/src/main/scala/chisel3/stage/ChiselCli.scala b/src/main/scala/chisel3/stage/ChiselCli.scala
index 4f7ac19e..78150029 100644
--- a/src/main/scala/chisel3/stage/ChiselCli.scala
+++ b/src/main/scala/chisel3/stage/ChiselCli.scala
@@ -7,6 +7,7 @@ import firrtl.options.Shell
 trait ChiselCli { this: Shell =>
   parser.note("Chisel Front End Options")
   Seq( NoRunFirrtlCompilerAnnotation,
-       PrintFullStackTraceAnnotation )
+       PrintFullStackTraceAnnotation,
+       ChiselGeneratorAnnotation )
     .foreach(_.addOptions(parser))
 }
diff --git a/src/main/scala/chisel3/stage/ChiselStage.scala b/src/main/scala/chisel3/stage/ChiselStage.scala
index 923867d7..aef1abb2 100644
--- a/src/main/scala/chisel3/stage/ChiselStage.scala
+++ b/src/main/scala/chisel3/stage/ChiselStage.scala
@@ -3,9 +3,15 @@
 package chisel3.stage
 
 import firrtl.AnnotationSeq
-import firrtl.options.{Phase, PhaseManager, PreservesAll, Shell, Stage}
+import firrtl.options.{Phase, PhaseManager, PreservesAll, Shell, Stage, StageError, StageMain}
 import firrtl.options.phases.DeletedWrapper
 import firrtl.stage.FirrtlCli
+import firrtl.options.Viewer.view
+
+import chisel3.ChiselException
+import chisel3.internal.ErrorLog
+
+import java.io.{StringWriter, PrintWriter}
 
 class ChiselStage extends Stage with PreservesAll[Phase] {
   val shell: Shell = new Shell("chisel") with ChiselCli with FirrtlCli
@@ -20,11 +26,27 @@ class ChiselStage extends Stage with PreservesAll[Phase] {
          classOf[chisel3.stage.phases.Convert],
          classOf[chisel3.stage.phases.MaybeFirrtlStage] )
 
-  def run(annotations: AnnotationSeq): AnnotationSeq =
-    /* @todo: Should this be wrapped in a try/catch? */
+  def run(annotations: AnnotationSeq): AnnotationSeq = try {
     new PhaseManager(targets) { override val wrappers = Seq( (a: Phase) => DeletedWrapper(a) ) }
       .transformOrder
       .map(firrtl.options.phases.DeletedWrapper(_))
       .foldLeft(annotations)( (a, f) => f.transform(a) )
+  } catch {
+    case ce: ChiselException =>
+      val stackTrace = if (!view[ChiselOptions](annotations).printFullStackTrace) {
+        ce.chiselStackTrace
+      } else {
+        val sw = new StringWriter
+        ce.printStackTrace(new PrintWriter(sw))
+        sw.toString
+      }
+      Predef
+        .augmentString(stackTrace)
+        .lines
+        .foreach(line => println(s"${ErrorLog.errTag} $line")) // scalastyle:ignore regex
+      throw new StageError()
+  }
 
 }
+
+object ChiselMain extends StageMain(new ChiselStage)
diff --git a/src/main/scala/chisel3/stage/phases/Elaborate.scala b/src/main/scala/chisel3/stage/phases/Elaborate.scala
index 150eacbe..0f92c480 100644
--- a/src/main/scala/chisel3/stage/phases/Elaborate.scala
+++ b/src/main/scala/chisel3/stage/phases/Elaborate.scala
@@ -15,28 +15,9 @@ import firrtl.options.{OptionsException, Phase, PreservesAll}
   */
 class Elaborate extends Phase with PreservesAll[Phase] {
 
-  /**
-    * @todo Change this to print to STDERR (`Console.err.println`)
-    */
   def transform(annotations: AnnotationSeq): AnnotationSeq = annotations.flatMap {
-    case a: ChiselGeneratorAnnotation =>
-      try {
-        a.elaborate
-      } catch {
-        case e: OptionsException => throw e
-        case e: ChiselException =>
-          val copts = view[ChiselOptions](annotations)
-          val stackTrace = if (!copts.printFullStackTrace) {
-            e.chiselStackTrace
-          } else {
-            val s = new StringWriter
-            e.printStackTrace(new PrintWriter(s))
-            s.toString
-          }
-          Predef.augmentString(stackTrace).lines.foreach(line => println(s"${ErrorLog.errTag} $line"))
-          Some(a)
-      }
-    case a => Some(a)
+    case a: ChiselGeneratorAnnotation => a.elaborate
+    case a                            => Some(a)
   }
 
 }
diff --git a/src/test/scala/chiselTests/ChiselSpec.scala b/src/test/scala/chiselTests/ChiselSpec.scala
index 75fa68dd..d0a1f5c5 100644
--- a/src/test/scala/chiselTests/ChiselSpec.scala
+++ b/src/test/scala/chiselTests/ChiselSpec.scala
@@ -11,6 +11,8 @@ import chisel3.testers._
 import firrtl.options.OptionsException
 import firrtl.{AnnotationSeq, CommonOptions, ExecutionOptionsManager, FirrtlExecutionFailure, FirrtlExecutionSuccess, HasFirrtlOptions}
 import firrtl.util.BackendCompilationUtilities
+import java.io.ByteArrayOutputStream
+import java.security.Permission
 
 /** Common utility functions for Chisel unit tests. */
 trait ChiselRunners extends Assertions with BackendCompilationUtilities {
@@ -200,3 +202,86 @@ class ChiselPropSpec extends PropSpec with ChiselRunners with PropertyChecks wit
     j <- Gen.choose(0, (1 << w) - 1)
   } yield (w, i, j)
 }
+
+trait Utils {
+
+  /** Run some Scala thunk and return STDOUT and STDERR as strings.
+    * @param thunk some Scala code
+    * @return a tuple containing STDOUT, STDERR, and what the thunk returns
+    */
+  def grabStdOutErr[T](thunk: => T): (String, String, T) = {
+    val stdout, stderr = new ByteArrayOutputStream()
+    val ret = scala.Console.withOut(stdout) { scala.Console.withErr(stderr) { thunk } }
+    (stdout.toString, stderr.toString, ret)
+  }
+
+  /** Encodes a System.exit exit code
+    * @param status the exit code
+    */
+  private case class ExitException(status: Int) extends SecurityException(s"Found a sys.exit with code $status")
+
+  /** A security manager that converts calls to System.exit into [[ExitException]]s by explicitly disabling the ability of
+    * a thread to actually exit. For more information, see:
+    *   - https://docs.oracle.com/javase/tutorial/essential/environment/security.html
+    */
+  private class ExceptOnExit extends SecurityManager {
+    override def checkPermission(perm: Permission): Unit = {}
+    override def checkPermission(perm: Permission, context: Object): Unit = {}
+    override def checkExit(status: Int): Unit = {
+      super.checkExit(status)
+      throw ExitException(status)
+    }
+  }
+
+  /** Encodes a file that some code tries to write to
+    * @param the file name
+    */
+  private case class WriteException(file: String) extends SecurityException(s"Tried to write to file $file")
+
+  /** A security manager that converts writes to any file into [[WriteException]]s.
+    */
+  private class ExceptOnWrite extends SecurityManager {
+    override def checkPermission(perm: Permission): Unit = {}
+    override def checkPermission(perm: Permission, context: Object): Unit = {}
+    override def checkWrite(file: String): Unit = {
+      super.checkWrite(file)
+      throw WriteException(file)
+    }
+  }
+
+  /** Run some Scala code (a thunk) in an environment where all System.exit are caught and returned. This avoids a
+    * situation where a test results in something actually exiting and killing the entire test. This is necessary if you
+    * want to test a command line program, e.g., the `main` method of [[firrtl.options.Stage Stage]].
+    *
+    * NOTE: THIS WILL NOT WORK IN SITUATIONS WHERE THE THUNK IS CATCHING ALL [[Exception]]s OR [[Throwable]]s, E.G.,
+    * SCOPT. IF THIS IS HAPPENING THIS WILL NOT WORK. REPEAT THIS WILL NOT WORK.
+    * @param thunk some Scala code
+    * @return either the output of the thunk (`Right[T]`) or an exit code (`Left[Int]`)
+    */
+  def catchStatus[T](thunk: => T): Either[Int, T] = {
+    try {
+      System.setSecurityManager(new ExceptOnExit())
+      Right(thunk)
+    } catch {
+      case ExitException(a) => Left(a)
+    } finally {
+      System.setSecurityManager(null)
+    }
+  }
+
+  /** Run some Scala code (a thunk) in an environment where file writes are caught and the file that a program tries to
+    * write to is returned. This is useful if you want to test that some thunk either tries to write to a specific file
+    * or doesn't try to write at all.
+    */
+  def catchWrites[T](thunk: => T): Either[String, T] = {
+    try {
+      System.setSecurityManager(new ExceptOnWrite())
+      Right(thunk)
+    } catch {
+      case WriteException(a) => Left(a)
+    } finally {
+      System.setSecurityManager(null)
+    }
+  }
+
+}
diff --git a/src/test/scala/chiselTests/DriverSpec.scala b/src/test/scala/chiselTests/DriverSpec.scala
index 8fc58e21..75ab93ae 100644
--- a/src/test/scala/chiselTests/DriverSpec.scala
+++ b/src/test/scala/chiselTests/DriverSpec.scala
@@ -17,7 +17,13 @@ class DummyModule extends Module {
   io.out := io.in
 }
 
-class DriverSpec extends FreeSpec with Matchers {
+class TypeErrorModule extends chisel3.experimental.MultiIOModule {
+  val in = IO(Input(UInt(1.W)))
+  val out = IO(Output(SInt(1.W)))
+  out := in
+}
+
+class DriverSpec extends FreeSpec with Matchers with chiselTests.Utils {
   "Driver's execute methods are used to run chisel and firrtl" - {
     "options can be picked up from comand line with no args" in {
       // NOTE: Since we don't provide any arguments (notably, "--target-dir"),
@@ -73,5 +79,21 @@ class DriverSpec extends FreeSpec with Matchers {
       dummyOutput.exists() should be(true)
       dummyOutput.delete()
     }
+
+    "user errors show a trimmed stack trace" in {
+      val targetDir = "test_run_dir"
+      val args = Array("--compiler", "low", "--target-dir", targetDir)
+
+      val (stdout, stderr, result) = grabStdOutErr { Driver.execute(args, () => new TypeErrorModule) }
+
+      info("stdout shows a trimmed stack trace")
+      stdout should include ("Stack trace trimmed to user code only")
+
+      info("stdout does not include FIRRTL information")
+      stdout should not include ("firrtl.")
+
+      info("Driver returned a ChiselExecutionFailure")
+      result shouldBe a [ChiselExecutionFailure]
+    }
   }
 }
diff --git a/src/test/scala/chiselTests/stage/ChiselMainSpec.scala b/src/test/scala/chiselTests/stage/ChiselMainSpec.scala
new file mode 100644
index 00000000..27a3093c
--- /dev/null
+++ b/src/test/scala/chiselTests/stage/ChiselMainSpec.scala
@@ -0,0 +1,120 @@
+// See LICENSE for license details.
+
+package chiselTests.stage
+
+import chisel3._
+import chisel3.stage.{ChiselGeneratorAnnotation, ChiselMain}
+import chisel3.experimental.RawModule
+
+import firrtl.AnnotationSeq
+
+import java.io.File
+
+import org.scalatest.{FeatureSpec, GivenWhenThen, Matchers}
+
+object ChiselMainSpec {
+
+  /** A module that connects two different types together resulting in an elaboration error */
+  class DifferentTypesModule extends RawModule {
+    val in = IO(UInt(1.W))
+    val out = IO(SInt(1.W))
+    out := in
+  }
+
+}
+
+class ChiselMainSpec extends FeatureSpec with GivenWhenThen with Matchers with chiselTests.Utils {
+
+  import ChiselMainSpec._
+
+  class ChiselMainFixture {
+    Given("a Chisel stage")
+    val stage = ChiselMain
+  }
+
+  class TargetDirectoryFixture(dirName: String) {
+    val dir = new File(s"test_run_dir/FirrtlStageSpec/$dirName")
+    val buildDir = new File(dir + "/build")
+    dir.mkdirs()
+  }
+
+  case class ChiselMainTest(
+    args: Array[String],
+    generator: Option[Class[_ <: RawModule]] = None,
+    files: Seq[String] = Seq.empty,
+    stdout: Option[String] = None,
+    stderr: Option[String] = None,
+    result: Int = 0) {
+    def testName: String = "args" + args.mkString("_")
+    def argsString: String = args.mkString(" ")
+  }
+
+  def runStageExpectFiles(p: ChiselMainTest): Unit = {
+    scenario(s"""User runs Chisel Stage with '${p.argsString}'""") {
+      val f = new ChiselMainFixture
+      val td = new TargetDirectoryFixture(p.testName)
+
+      p.files.foreach( f => new File(td.buildDir + s"/$f").delete() )
+
+      When(s"""the user tries to compile with '${p.argsString}'""")
+      val (stdout, stderr, result) =
+        grabStdOutErr {
+          catchStatus {
+            val module: Array[String] = Array("foo") ++
+              (if (p.generator.nonEmpty) { Array("--module", p.generator.get.getName) }
+               else                      { Array.empty[String]                        })
+            f.stage.main(Array("-td", td.buildDir.toString) ++ module ++ p.args)
+          }
+        }
+
+      p.stdout match {
+        case Some(a) =>
+          Then(s"""STDOUT should include "$a"""")
+          stdout should include (a)
+        case None =>
+          Then(s"nothing should print to STDOUT")
+          stdout should be (empty)
+      }
+
+      p.stderr match {
+        case Some(a) =>
+          And(s"""STDERR should include "$a"""")
+          stderr should include (a)
+        case None =>
+          And(s"nothing should print to STDERR")
+          stderr should be (empty)
+      }
+
+      p.result match {
+        case 0 =>
+          And(s"the exit code should be 0")
+          result shouldBe a [Right[_,_]]
+        case a =>
+          And(s"the exit code should be $a")
+          result shouldBe (Left(a))
+      }
+
+      p.files.foreach { f =>
+        And(s"file '$f' should be emitted in the target directory")
+        val out = new File(td.buildDir + s"/$f")
+        out should (exist)
+      }
+    }
+  }
+
+  info("As a Chisel user")
+  info("I screw up and compile some bad code")
+  feature("Stack trace trimming") {
+    Seq(
+      ChiselMainTest(args = Array("-X", "low"),
+                     generator = Some(classOf[DifferentTypesModule]),
+                     stdout = Some("Stack trace trimmed to user code only"),
+                     result = 1),
+      ChiselMainTest(args = Array("-X", "high", "--full-stacktrace"),
+                     generator = Some(classOf[DifferentTypesModule]),
+                     stdout = Some("org.scalatest"),
+                     result = 1)
+    ).foreach(runStageExpectFiles)
+  }
+
+}