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

* 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.

(cherry picked from commit 88d147d)

# Conflicts:
#	src/main/scala/circt/stage/ChiselStage.scala

* Resolve backport conflicts

* Make logger annotations unserializable

Change logger annotations to mix-in the Unserializable trait so that they
will not emitted by a stage.  These annotations are not intended to be
seen by CIRCT and these should be stripped from the output FIRRTL text.

Signed-off-by: Schuyler Eldridge <[email protected]>

---------

Signed-off-by: Schuyler Eldridge <[email protected]>
Co-authored-by: Jack Koenig <[email protected]>
Co-authored-by: Schuyler Eldridge <[email protected]>

Author: 3 people

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 {