Fix ChiselStage and Builder handling of logging (#3895)

Previously, object circt.stage.ChiselStage was ignoring the Logger. Also, Chisel was not creating its own logger scope which could lead to clobbering of the Console when running invoking Chisel in the same process multiple times. Fix various places we had to workaround this behavior and fix tests checking --log-level debug.
chipsalliance · Mar 1, 2024 · 88d147d · 88d147d
1 parent e044213
commit 88d147d
Show file tree

Hide file tree

Showing 10 changed files with 182 additions and 80 deletions.
diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala b/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala
@@ -108,7 +108,8 @@ object Definition extends SourceInfoDoc {
         context.warningFilters,
         context.sourceRoots,
         Some(context.globalNamespace),
-        Builder.allDefinitions
+        Builder.allDefinitions,
+        context.loggerOptions
       )
     }
     dynamicContext.inDefinition = true

diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/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.
@@ -453,7 +454,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
@@ -1008,6 +1010,16 @@ private[chisel3] object Builder extends LazyLogging {
   private[chisel3] def build[T <: BaseModule](
     f:              => T,
     dynamicContext: DynamicContext
+  ): (Circuit, T) = {
+    // Logger has its own context separate from Chisel's dynamic context
+    _root_.logger.Logger.makeScope(dynamicContext.loggerOptions) {
+      buildImpl(f, dynamicContext)
+    }
+  }
+
+  private def buildImpl[T <: BaseModule](
+    f:              => T,
+    dynamicContext: DynamicContext
   ): (Circuit, T) = {
     dynamicContextVar.withValue(Some(dynamicContext)) {
       ViewParent: Unit // Must initialize the singleton in a Builder context or weird things can happen

diff --git a/docs/src/cookbooks/cookbook.md b/docs/src/cookbooks/cookbook.md
@@ -813,9 +813,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)

diff --git a/docs/src/explanations/warnings.md b/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)

diff --git a/firrtl/src/main/scala/logger/Logger.scala b/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
   }
 }
 

diff --git a/src/main/scala/chisel3/aop/injecting/InjectingAspect.scala b/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,6 +58,7 @@ 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,
@@ -65,7 +67,8 @@ abstract class InjectorAspect[T <: RawModule, M <: RawModule](
           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

diff --git a/src/main/scala/chisel3/stage/phases/Elaborate.scala b/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(
@@ -37,7 +42,8 @@ class Elaborate extends Phase {
             chiselOptions.warningFilters,
             chiselOptions.sourceRoots,
             None,
-            Nil
+            Nil,
+            loggerOptions
           )
         val (circuit, dut) =
           Builder.build(Module(gen()), context)

diff --git a/src/main/scala/circt/stage/ChiselStage.scala b/src/main/scala/circt/stage/ChiselStage.scala
@@ -3,51 +3,12 @@
 package circt.stage
 
 import chisel3.RawModule
-import chisel3.stage.{
-  ChiselCircuitAnnotation,
-  ChiselGeneratorAnnotation,
-  CircuitSerializationAnnotation,
-  PrintFullStackTraceAnnotation,
-  SourceRootAnnotation,
-  ThrowOnFirstErrorAnnotation,
-  UseLegacyShiftRightWidthBehavior,
-  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,
-    UseLegacyShiftRightWidthBehavior,
-    WarningsAsErrorsAnnotation,
-    WarningConfigurationAnnotation,
-    WarningConfigurationFileAnnotation,
-    SourceRootAnnotation,
-    SplitVerilog,
-    FirtoolBinaryPath,
-    DumpFir
-  ).foreach(_.addOptions(parser))
-}
+import logger.LogLevelAnnotation
 
 /** Entry point for running Chisel with the CIRCT compiler.
   *
@@ -60,13 +21,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],
@@ -75,7 +38,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(
@@ -94,7 +56,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],
@@ -113,7 +74,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)
 
@@ -140,7 +101,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)
@@ -159,7 +120,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)
@@ -178,7 +139,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)
@@ -203,7 +164,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 {