Fix ChiselStage and Builder handling of logging (backport #3895)

core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala

@@ -107,7 +107,8 @@ object Definition extends SourceInfoDoc {
         context.warningFilters,
         context.sourceRoots,
         Some(context.globalNamespace),
-        Builder.allDefinitions
+        Builder.allDefinitions,
+        context.loggerOptions
       )
     }
     dynamicContext.inDefinition = true

core/src/main/scala/chisel3/internal/Builder.scala

@@ -11,21 +11,22 @@ import chisel3.properties.Class
 import chisel3.internal.firrtl.ir._
 import chisel3.internal.firrtl.Converter
 import chisel3.internal.naming._
-import _root_.firrtl.annotations.{CircuitName, ComponentName, IsMember, ModuleName, Named, ReferenceTarget}
+import _root_.firrtl.annotations.{Annotation, CircuitName, ComponentName, IsMember, ModuleName, Named, ReferenceTarget}
 import _root_.firrtl.annotations.AnnotationUtils.validComponentName
 import _root_.firrtl.AnnotationSeq
 import _root_.firrtl.renamemap.MutableRenameMap
 import _root_.firrtl.util.BackendCompilationUtilities._
 import _root_.firrtl.{ir => fir}
 import chisel3.experimental.dataview.{reify, reifySingleData}
 import chisel3.internal.Builder.Prefix
-import logger.LazyLogging
+import logger.{LazyLogging, LoggerOption}
 
 import scala.collection.mutable
 import scala.annotation.tailrec
 import java.io.File
 import scala.util.control.NonFatal
 import chisel3.ChiselException
+import logger.LoggerOptions
 
 private[chisel3] class Namespace(keywords: Set[String], separator: Char = '_') {
   // This HashMap is compressed, not every name in the namespace is present here.
@@ -452,7 +453,8 @@ private[chisel3] class DynamicContext(
   val sourceRoots:       Seq[File],
   val defaultNamespace:  Option[Namespace],
   // Definitions from other scopes in the same elaboration, use allDefinitions below
-  val outerScopeDefinitions: List[Iterable[Definition[_]]]) {
+  val outerScopeDefinitions: List[Iterable[Definition[_]]],
+  val loggerOptions:         LoggerOptions) {
   val importedDefinitionAnnos = annotationSeq.collect { case a: ImportDefinitionAnnotation[_] => a }
 
   // Map from proto module name to ext-module name
@@ -1000,6 +1002,17 @@ private[chisel3] object Builder extends LazyLogging {
     f:              => T,
     dynamicContext: DynamicContext,
     forceModName:   Boolean = true
+  ): (Circuit, T) = {
+    // Logger has its own context separate from Chisel's dynamic context
+    _root_.logger.Logger.makeScope(dynamicContext.loggerOptions) {
+      buildImpl(f, dynamicContext, forceModName)
+    }
+  }
+
+  private def buildImpl[T <: BaseModule](
+    f:              => T,
+    dynamicContext: DynamicContext,
+    forceModName:   Boolean
   ): (Circuit, T) = {
     dynamicContextVar.withValue(Some(dynamicContext)) {
       ViewParent: Unit // Must initialize the singleton in a Builder context or weird things can happen

docs/src/cookbooks/cookbook.md

@@ -832,9 +832,6 @@ If the indexee is a non-power-of-2 size, use the ceiling of the log2 result.
 
 ```scala mdoc:invisible:reset
 import chisel3._
-// Some other test is clobbering the global Logger which breaks the warnings below
-// Setting the output stream to the Console fixes the issue
-logger.Logger.setConsole()
 // Helper to throw away return value so it doesn't show up in mdoc
 def compile(gen: => chisel3.RawModule): Unit = {
   circt.stage.ChiselStage.emitCHIRRTL(gen)

docs/src/explanations/warnings.md

@@ -61,9 +61,6 @@ The supported actions are:
 The following example issues a warning when elaborated normally
 
 ```scala mdoc:invisible:reset
-// Some other test is clobbering the global Logger which breaks the warnings below
-// Setting the output stream to the Console fixes the issue
-logger.Logger.setConsole()
 // Helper to throw away return value so it doesn't show up in mdoc
 def compile(gen: => chisel3.RawModule, args: Array[String] = Array()): Unit = {
   circt.stage.ChiselStage.emitCHIRRTL(gen, args = args)

firrtl/src/main/scala/logger/Logger.scala

@@ -121,7 +121,18 @@ object Logger {
     * @tparam A return type of the code block
     * @return the original return of the code block
     */
-  def makeScope[A](options: AnnotationSeq = Seq.empty)(codeBlock: => A): A = {
+  def makeScope[A](options: AnnotationSeq = Seq.empty)(codeBlock: => A): A =
+    makeScope(() => setOptions(options))(codeBlock)
+
+  /** Set a scope for this logger based on available annotations
+    * @param options LoggerOptions to use
+    * @param codeBlock some Scala code over which to define this scope
+    * @tparam A return type of the code block
+    * @return the original return of the code block
+    */
+  def makeScope[A](options: LoggerOptions)(codeBlock: => A): A = makeScope(() => setOptions(options))(codeBlock)
+
+  private def makeScope[A](doSetOptions: () => Unit)(codeBlock: => A): A = {
     val runState: LoggerState = {
       val newRunState = updatableLoggerState.value.getOrElse(new LoggerState)
       if (newRunState.fromInvoke) {
@@ -133,7 +144,7 @@ object Logger {
       }
     }
     updatableLoggerState.withValue(Some(runState)) {
-      setOptions(options)
+      doSetOptions()
       codeBlock
     }
   }
@@ -326,20 +337,26 @@ object Logger {
       Seq(new AddDefaults, Checks)
         .foldLeft(inputAnnotations)((a, p) => p.transform(a))
 
-    val lopts = view[LoggerOptions](annotations)
-    state.globalLevel = (state.globalLevel, lopts.globalLogLevel) match {
+    setOptions(view[LoggerOptions](annotations))
+  }
+
+  /** Set logger options
+    * @param options options to set
+    */
+  def setOptions(options: LoggerOptions): Unit = {
+    state.globalLevel = (state.globalLevel, options.globalLogLevel) match {
       case (LogLevel.None, LogLevel.None) => LogLevel.None
       case (x, LogLevel.None)             => x
       case (LogLevel.None, x)             => x
       case (_, x)                         => x
     }
-    setClassLogLevels(lopts.classLogLevels)
+    setClassLogLevels(options.classLogLevels)
 
-    if (lopts.logFileName.nonEmpty) {
-      setOutput(lopts.logFileName.get)
+    if (options.logFileName.nonEmpty) {
+      setOutput(options.logFileName.get)
     }
 
-    state.logClassNames = lopts.logClassNames
+    state.logClassNames = options.logClassNames
   }
 }
 

firrtl/src/main/scala/logger/LoggerAnnotations.scala

@@ -3,7 +3,7 @@
 package logger
 
 import firrtl.annotations.{Annotation, NoTargetAnnotation}
-import firrtl.options.{HasShellOptions, ShellOption}
+import firrtl.options.{HasShellOptions, ShellOption, Unserializable}
 
 /** An annotation associated with a Logger command line option */
 sealed trait LoggerOption { this: Annotation => }
@@ -16,6 +16,7 @@ sealed trait LoggerOption { this: Annotation => }
 case class LogLevelAnnotation(globalLogLevel: LogLevel.Value = LogLevel.None)
     extends NoTargetAnnotation
     with LoggerOption
+    with Unserializable
 
 object LogLevelAnnotation extends HasShellOptions {
 
@@ -39,6 +40,7 @@ object LogLevelAnnotation extends HasShellOptions {
 case class ClassLogLevelAnnotation(className: String, level: LogLevel.Value)
     extends NoTargetAnnotation
     with LoggerOption
+    with Unserializable
 
 object ClassLogLevelAnnotation extends HasShellOptions {
 
@@ -63,7 +65,7 @@ object ClassLogLevelAnnotation extends HasShellOptions {
   *  - maps to [[LoggerOptions.logFileName]]
   *  - enabled with `--log-file`
   */
-case class LogFileAnnotation(file: Option[String]) extends NoTargetAnnotation with LoggerOption
+case class LogFileAnnotation(file: Option[String]) extends NoTargetAnnotation with LoggerOption with Unserializable
 
 object LogFileAnnotation extends HasShellOptions {
 
@@ -81,7 +83,11 @@ object LogFileAnnotation extends HasShellOptions {
 /** Enables class names in log output
   *  - enabled with `-lcn/--log-class-names`
   */
-case object LogClassNamesAnnotation extends NoTargetAnnotation with LoggerOption with HasShellOptions {
+case object LogClassNamesAnnotation
+    extends NoTargetAnnotation
+    with LoggerOption
+    with HasShellOptions
+    with Unserializable {
 
   val options = Seq(
     new ShellOption[Unit](

src/main/scala/chisel3/aop/injecting/InjectingAspect.scala

@@ -12,6 +12,7 @@ import firrtl.options.Viewer.view
 import firrtl.{ir, _}
 
 import scala.collection.mutable
+import logger.LoggerOptions
 
 /** Aspect to inject Chisel code into a module of type M
   *
@@ -57,14 +58,16 @@ abstract class InjectorAspect[T <: RawModule, M <: RawModule](
   final def toAnnotation(modules: Iterable[M], circuit: String, moduleNames: Seq[String]): AnnotationSeq = {
     modules.map { module =>
       val chiselOptions = view[ChiselOptions](annotationsInAspect)
+      val loggerOptions = view[LoggerOptions](annotationsInAspect)
       val dynamicContext =
         new DynamicContext(
           annotationsInAspect,
           chiselOptions.throwOnFirstError,
           chiselOptions.warningFilters,
           chiselOptions.sourceRoots,
           None,
-          Nil // FIXME this maybe should somehow grab definitions from earlier elaboration
+          Nil, // FIXME this maybe should somehow grab definitions from earlier elaboration
+          loggerOptions
         )
       // Add existing module names into the namespace. If injection logic instantiates new modules
       //  which would share the same name, they will get uniquified accordingly

src/main/scala/chisel3/stage/phases/Elaborate.scala

@@ -13,21 +13,26 @@ import chisel3.stage.{
   ThrowOnFirstErrorAnnotation
 }
 import firrtl.AnnotationSeq
-import firrtl.options.Phase
+import firrtl.options.{Dependency, Phase}
 import firrtl.options.Viewer.view
+import logger.LoggerOptions
 
 /** Elaborate all [[chisel3.stage.ChiselGeneratorAnnotation]]s into [[chisel3.stage.ChiselCircuitAnnotation]]s.
   */
 class Elaborate extends Phase {
 
-  override def prerequisites = Seq.empty
+  override def prerequisites: Seq[Dependency[Phase]] = Seq(
+    Dependency[chisel3.stage.phases.Checks],
+    Dependency(_root_.logger.phases.Checks)
+  )
   override def optionalPrerequisites = Seq.empty
   override def optionalPrerequisiteOf = Seq.empty
   override def invalidates(a: Phase) = false
 
   def transform(annotations: AnnotationSeq): AnnotationSeq = annotations.flatMap {
     case ChiselGeneratorAnnotation(gen) =>
       val chiselOptions = view[ChiselOptions](annotations)
+      val loggerOptions = view[LoggerOptions](annotations)
       try {
         val context =
           new DynamicContext(
@@ -36,7 +41,8 @@ class Elaborate extends Phase {
             chiselOptions.warningFilters,
             chiselOptions.sourceRoots,
             None,
-            Nil
+            Nil,
+            loggerOptions
           )
         val (circuit, dut) =
           Builder.build(Module(gen()), context)

src/main/scala/circt/stage/ChiselStage.scala

@@ -3,49 +3,13 @@
 package circt.stage
 
 import chisel3.RawModule
-import chisel3.stage.{
-  ChiselCircuitAnnotation,
-  ChiselGeneratorAnnotation,
-  CircuitSerializationAnnotation,
-  PrintFullStackTraceAnnotation,
-  SourceRootAnnotation,
-  ThrowOnFirstErrorAnnotation,
-  WarningConfigurationAnnotation,
-  WarningConfigurationFileAnnotation,
-  WarningsAsErrorsAnnotation
-}
+import chisel3.stage.{ChiselCircuitAnnotation, ChiselGeneratorAnnotation, CircuitSerializationAnnotation}
 import chisel3.stage.CircuitSerializationAnnotation.FirrtlFileFormat
 import firrtl.{AnnotationSeq, EmittedVerilogCircuitAnnotation}
-import firrtl.options.{
-  BareShell,
-  CustomFileEmission,
-  Dependency,
-  Phase,
-  PhaseManager,
-  Shell,
-  Stage,
-  StageMain,
-  Unserializable
-}
+import firrtl.options.{CustomFileEmission, Dependency, Phase, PhaseManager, Stage, StageMain, Unserializable}
 import firrtl.stage.FirrtlCircuitAnnotation
 
-trait CLI { this: BareShell =>
-  parser.note("CIRCT (MLIR FIRRTL Compiler) options")
-  Seq(
-    CIRCTTargetAnnotation,
-    PreserveAggregate,
-    ChiselGeneratorAnnotation,
-    PrintFullStackTraceAnnotation,
-    ThrowOnFirstErrorAnnotation,
-    WarningsAsErrorsAnnotation,
-    WarningConfigurationAnnotation,
-    WarningConfigurationFileAnnotation,
-    SourceRootAnnotation,
-    SplitVerilog,
-    FirtoolBinaryPath,
-    DumpFir
-  ).foreach(_.addOptions(parser))
-}
+import logger.LogLevelAnnotation
 
 /** Entry point for running Chisel with the CIRCT compiler.
   *
@@ -58,13 +22,15 @@ class ChiselStage extends Stage {
   override def optionalPrerequisiteOf = Seq.empty
   override def invalidates(a: Phase) = false
 
-  override val shell = new Shell("circt") with CLI
+  override val shell = new firrtl.options.Shell("circt") with CLI {
+    // These are added by firrtl.options.Shell (which we must extend because we are a Stage)
+    override protected def includeLoggerOptions = false
+  }
 
   override def run(annotations: AnnotationSeq): AnnotationSeq = {
 
     val pm = new PhaseManager(
       targets = Seq(
-        Dependency[chisel3.stage.phases.Checks],
         Dependency[chisel3.stage.phases.AddImplicitOutputFile],
         Dependency[chisel3.stage.phases.AddImplicitOutputAnnotationFile],
         Dependency[chisel3.stage.phases.MaybeAspectPhase],
@@ -73,7 +39,6 @@ class ChiselStage extends Stage {
         Dependency[chisel3.stage.phases.AddDedupGroupAnnotations],
         Dependency[chisel3.stage.phases.MaybeInjectingPhase],
         Dependency[circt.stage.phases.AddImplicitOutputFile],
-        Dependency[circt.stage.phases.Checks],
         Dependency[circt.stage.phases.CIRCT]
       ),
       currentState = Seq(
@@ -92,7 +57,6 @@ object ChiselStage {
   /** A phase shared by all the CIRCT backends */
   private def phase = new PhaseManager(
     Seq(
-      Dependency[chisel3.stage.phases.Checks],
       Dependency[chisel3.aop.injecting.InjectingPhase],
       Dependency[chisel3.stage.phases.Elaborate],
       Dependency[chisel3.stage.phases.Convert],
@@ -111,7 +75,7 @@ object ChiselStage {
     val annos = Seq(
       ChiselGeneratorAnnotation(() => gen),
       CIRCTTargetAnnotation(CIRCTTarget.CHIRRTL)
-    ) ++ (new BareShell("circt") with CLI).parse(args)
+    ) ++ (new Shell("circt")).parse(args)
 
     val resultAnnos = phase.transform(annos)
 
@@ -138,7 +102,7 @@ object ChiselStage {
     val annos = Seq(
       ChiselGeneratorAnnotation(() => gen),
       CIRCTTargetAnnotation(CIRCTTarget.CHIRRTL)
-    ) ++ (new BareShell("circt") with CLI).parse(args)
+    ) ++ (new Shell("circt")).parse(args)
 
     phase
       .transform(annos)
@@ -157,7 +121,7 @@ object ChiselStage {
     val annos = Seq(
       ChiselGeneratorAnnotation(() => gen),
       CIRCTTargetAnnotation(CIRCTTarget.FIRRTL)
-    ) ++ (new BareShell("circt") with CLI).parse(args) ++ firtoolOpts.map(FirtoolOption(_))
+    ) ++ (new Shell("circt")).parse(args) ++ firtoolOpts.map(FirtoolOption(_))
 
     phase
       .transform(annos)
@@ -176,7 +140,7 @@ object ChiselStage {
     val annos = Seq(
       ChiselGeneratorAnnotation(() => gen),
       CIRCTTargetAnnotation(CIRCTTarget.HW)
-    ) ++ (new BareShell("circt") with CLI).parse(args) ++ firtoolOpts.map(FirtoolOption(_))
+    ) ++ (new Shell("circt")).parse(args) ++ firtoolOpts.map(FirtoolOption(_))
 
     phase
       .transform(annos)
@@ -201,7 +165,7 @@ object ChiselStage {
     val annos = Seq(
       ChiselGeneratorAnnotation(() => gen),
       CIRCTTargetAnnotation(CIRCTTarget.SystemVerilog)
-    ) ++ (new BareShell("circt") with CLI).parse(args) ++ firtoolOpts.map(FirtoolOption(_))
+    ) ++ (new Shell("circt")).parse(args) ++ firtoolOpts.map(FirtoolOption(_))
     phase
       .transform(annos)
       .collectFirst {