Chisel Linting Framework (#2435)

* Added Linting Library, with lint rules for anonymous registers and auto-truncation of widths during connection

* Updated makefiles to enable running linting via CHISEL_OPTIONS

emulator/Makefrag-verilator

@@ -10,7 +10,7 @@ verilog = \
 
 $(generated_dir)/%.fir $(generated_dir)/%.d: $(FIRRTL_JAR) $(chisel_srcs) $(bootrom_img)
 	mkdir -p $(dir $@)
-	cd $(base_dir) && $(SBT) "runMain $(PROJECT).Generator -td $(generated_dir) -T $(PROJECT).$(MODEL) -C $(CONFIG)"
+	cd $(base_dir) && $(SBT) "runMain $(PROJECT).Generator -td $(generated_dir) -T $(PROJECT).$(MODEL) -C $(CONFIG) $(CHISEL_OPTIONS)"
 
 %.v %.conf: %.fir $(FIRRTL_JAR)
 	mkdir -p $(dir $@)
@@ -22,6 +22,7 @@ $(generated_dir)/%.fir $(generated_dir)/%.d: $(FIRRTL_JAR) $(chisel_srcs) $(boot
     -faf $*.anno.json \
     -td $(generated_dir)/$(long_name)/ \
     -fct $(subst $(SPACE),$(COMMA),$(FIRRTL_TRANSFORMS)) \
+    $(FIRRTL_OPTIONS) \
 
 $(generated_dir)/$(long_name).behav_srams.v : $(generated_dir)/$(long_name).conf $(VLSI_MEM_GEN)
 	cd $(generated_dir) && \

src/main/resources/META-INF/services/firrtl.options.RegisteredLibrary

@@ -0,0 +1,3 @@
+freechips.rocketchip.linting.LintReporter
+freechips.rocketchip.linting.rule.LintAnonymousRegisters
+freechips.rocketchip.linting.rule.LintTruncatingWidths

src/main/scala/linting/LintAnnotation.scala

@@ -0,0 +1,36 @@
+// See LICENSE.SiFive for license details.
+
+package freechips.rocketchip.linting
+
+import firrtl.ir.{Info, FileInfo}
+import firrtl.annotations.NoTargetAnnotation
+import chisel3.experimental.ChiselAnnotation
+
+/** Parent trait for all linting annotations */
+trait LintAnnotation extends NoTargetAnnotation with ChiselAnnotation {
+  override def toFirrtl = this
+}
+
+/** Represents a linting violation under a given linter rule */
+case class Violation(linter: rule.LintRule, info: Info, message: String, modules: Set[String]) extends LintAnnotation {
+  def getScalaFiles: Seq[String] = {
+    val scala = "(.*\\.scala).*".r
+    rule.flatten(info).flatMap {
+      case f: FileInfo => f.info.serialize match {
+        case scala(file) => Some(file)
+        case other => None
+      }
+      case other => None
+    }
+  }
+}
+
+/** A list of files to ignore lint violations on, for a given lint rule */
+case class Whitelist(lintName: String, whiteList: Set[String]) extends LintAnnotation
+
+/** A container of lint rule violation display options */
+case class DisplayOptions(
+    level: String  = "strict",
+    totalLimit: Option[Int] = None,
+    perErrorLimit: Map[String, Int] = Map.empty
+) extends LintAnnotation

src/main/scala/linting/LintException.scala

@@ -0,0 +1,58 @@
+// See LICENSE.SiFive for license details.
+
+package freechips.rocketchip.linting
+
+import firrtl.FirrtlUserException
+import firrtl.ir.FileInfo
+
+/** Thrown to report all linting rule violations, according to the display options */
+case class LintException(seq: Seq[Violation], lintDisplayOptions: DisplayOptions) extends FirrtlUserException(
+  LintException.buildMessage(seq, lintDisplayOptions)
+)
+
+object LintException {
+  private def makeNumber(max: Int, n: Int, prefix: String): String = {
+    val nDigits = max.toString.size
+    val nLeading = nDigits - n.toString.size
+    prefix * nLeading + n.toString
+  }
+
+  private[linting] def buildMessage(seq: Seq[Violation], lintDisplayOptions: DisplayOptions): String = {
+    val groupedErrors = seq.groupBy {
+      case l: Violation => l.linter.lintName
+    }
+    val maxErrorNumber = groupedErrors.keys.max
+
+    val (_, reports) = groupedErrors.toSeq.sortBy(_._1).reverse.foldRight((0, Seq.empty[String])) {
+      case ((lintName: String, lintErrors: Seq[Violation]), (totalErrors: Int, reportsSoFar: Seq[String])) =>
+        val le                  = lintErrors.head.linter
+        val perErrorLimit       = lintDisplayOptions.perErrorLimit.getOrElse(lintName, lintErrors.size)
+        val totalErrorLimit     = lintDisplayOptions.totalLimit.map(t => t - totalErrors).getOrElse(perErrorLimit)
+        val remainingErrorLimit = totalErrorLimit.min(perErrorLimit)
+        val scalaFiles          = lintErrors.flatMap(_.getScalaFiles).distinct
+        val lintString          = lintName
+        val header =
+          s"""
+             |Lint rule ${le.lintName}: ${lintErrors.size} exceptions!
+             | - Recommended fix:
+             |     ${le.recommendedFix}
+             | - Whitelist file via Chisel cmdline arg:
+             |     ${le.whitelistAPI(scalaFiles)}
+             | - Whitelist file via Chisel scala API:
+             |     ${le.scalaAPI(scalaFiles)}
+             | - Disable this linting check:
+             |     ${le.disableCLI}
+             | - Modify display settings with:
+             |     --lint-options ...,display:${lintName}=<number>,...
+             |""".stripMargin
+
+        val errors = lintErrors.zip(1 to remainingErrorLimit).map {
+          case (lint: Violation, idx: Int) =>
+            s"$lintString.${makeNumber(remainingErrorLimit.min(lintErrors.size),idx,"0")}:${lint.info} ${lint.message} in ${lint.modules}"
+          }.mkString("\n")
+
+        (totalErrors + remainingErrorLimit, (header + errors) +: reportsSoFar)
+    }
+    reports.reverse.mkString("\n")
+  }
+}

src/main/scala/linting/LintReporter.scala

@@ -0,0 +1,82 @@
+// See LICENSE.SiFive for license details.
+
+package freechips.rocketchip.linting
+
+import firrtl._
+import firrtl.ir._
+import firrtl.traversals.Foreachers._
+import firrtl.annotations.NoTargetAnnotation
+import firrtl.options.{OptionsException, RegisteredLibrary, ShellOption, PreservesAll, Dependency}
+import firrtl.stage.RunFirrtlTransformAnnotation
+import chisel3.experimental.ChiselAnnotation
+import scala.collection.mutable
+
+/** The final transform for all linting
+  * Collects all computer lint violations and displays them
+  * Optionally kills the compilation, or proceeds with a warning
+  */
+final class LintReporter extends Transform with RegisteredLibrary with DependencyAPIMigration with PreservesAll[Transform] {
+  val displayTotal = "displayTotal=(\\d+)".r
+  val perTotal = "display:([_a-zA-Z0-9\\-]+)=(\\d+)".r
+  val perAllTotal = "display:\\*=(\\d+)".r
+
+  lazy val options = Seq(
+    new ShellOption[String](
+      longOption = s"lint",
+      toAnnotationSeq = {
+        case "*" => RunFirrtlTransformAnnotation(this) +: (Linter.lintMap.values.map(RunFirrtlTransformAnnotation(_)).toSeq)
+        case other => RunFirrtlTransformAnnotation(this) +: (other.split(',').toSeq.map { s =>
+          Linter.lintMap.get(s) match {
+            case Some(l) => RunFirrtlTransformAnnotation(l)
+            case None => sys.error(s"Unknown linter argument: $s")
+          }
+        })
+      },
+      helpText = s"Enable linting for specified rules, where * is all rules. Available rules: ${Linter.linters.map(l => l.lintName).mkString(",")}.",
+      helpValueName = Some("[*]|[<lintRule>,<lintRule>,...]")
+    ),
+    new ShellOption[String](
+      longOption = "lint-options",
+      toAnnotationSeq = { arg: String =>
+        val displayOptions = arg.split(',').toSeq.foldLeft(DisplayOptions()) { (opt, str) =>
+          str match {
+            case "strict" => opt.copy(level = "strict")
+            case "warn" => opt.copy(level = "warn")
+            case displayTotal(n) => opt.copy(totalLimit = Some(n.toInt))
+            case perTotal(lint, n) => opt.copy(perErrorLimit = opt.perErrorLimit + (Linter.lintMap(lint).lintName -> n.toInt))
+            case perAllTotal(n) => opt.copy(perErrorLimit = Linter.linters.map(l => l.lintName -> n.toInt).toMap)
+            case other => throw sys.error(s"Unrecognized option passed to --lint: $other")
+          }
+        }
+        Seq(displayOptions)
+      },
+      helpText = "Customize linting options, including strict/warn or number of violations displayed.",
+      helpValueName = Some("(strict|warn)[,displayTotal=<numError>][,display:<lintName>=<numError>]")
+    )
+  )
+
+  // Run before ExpandWhens
+  override def optionalPrerequisiteOf = Seq(Dependency[firrtl.passes.ExpandWhensAndCheck])
+
+  override def execute(state: CircuitState): CircuitState = {
+    val grouped = state.annotations.groupBy {
+      case e: Violation => "v"
+      case o: DisplayOptions => "o"
+      case w: Whitelist => "w"
+      case other => "a"
+    }
+
+    val violations = grouped.getOrElse("v", Nil).asInstanceOf[Seq[Violation]]
+    val options = grouped.getOrElse("o", Nil).headOption.getOrElse(DisplayOptions()).asInstanceOf[DisplayOptions]
+    val remainingAnnotations = grouped.getOrElse("a", Nil)
+
+    if(violations.nonEmpty) {
+      options.level match {
+        case "strict" => throw LintException(violations.toSeq, options)
+        case "warn" => println(LintException.buildMessage(violations.toSeq, options))
+      }
+    }
+
+    state.copy(annotations = remainingAnnotations)
+  }
+}

src/main/scala/linting/Linter.scala

@@ -0,0 +1,33 @@
+// See LICENSE.SiFive for license details.
+
+package freechips.rocketchip.linting
+
+import firrtl._
+import firrtl.ir._
+import chisel3.experimental.annotate
+
+/** Chisel users: Use to whitelist files
+  * Lint rule writers: update linters list whenever a new lint rule is created
+  */
+object Linter {
+
+  /** Use to whitelist specific files from specific linting rules
+    *
+    * @param lintRuleName the name of the lint rule
+    * @param filenames scala files to except from this linting rule
+    */
+  def whitelist(lintRuleName: String, filenames: String*) = {
+    require(lintMap.contains(lintRuleName), s"Unknown lint name: $lintRuleName")
+    annotate(Whitelist(lintRuleName, filenames.toSet))
+  }
+
+  // Update list for any new lint rule
+  private[linting] lazy val linters = Seq(
+    new rule.LintAnonymousRegisters,
+    new rule.LintTruncatingWidths
+  )
+
+  private [linting] lazy val lintMap = linters.flatMap {
+    l => Seq(l.lintName -> l)
+  }.toMap
+}

src/main/scala/linting/rule/LintAnonymousRegisters.scala

@@ -0,0 +1,34 @@
+// See LICENSE.SiFive for license details.
+
+package freechips.rocketchip.linting
+package rule
+
+import firrtl.ir._
+import firrtl.options.Dependency
+
+
+/** Reports all anonymous registers in design
+  * An anonymous register is one which is prefixed with an "_"
+  */
+final class LintAnonymousRegisters extends LintRule {
+
+  val recommendedFix: String = "Use named intermediate val, or if that fails use @chiselName or *.suggestName(...)"
+
+  val lintName: String = "anon-regs"
+
+  // Should run before LowerTypes so anonymous aggregate registers are reported as one register
+  override def optionalPrerequisiteOf = super.optionalPrerequisiteOf :+ Dependency(firrtl.passes.LowerTypes)
+
+  override protected def lintStatement(violations: Violations, mname: String)(s: Statement): Unit = {
+    s match {
+      case r: DefRegister  if isTemporary(r.name) =>
+        // Report scala info, if its present. Otherwise, use existing Info
+        getScalaInfo(r.info) match {
+          case Some(scalaInfo: FileInfo) => updateViolations(scalaInfo, "", violations, mname)
+          case None                      => updateViolations(r.info, "", violations, mname)
+        }
+      case other =>
+    }
+    super.lintStatement(violations, mname)(s)
+  }
+}

src/main/scala/linting/rule/LintRule.scala

@@ -0,0 +1,92 @@
+// See LICENSE.SiFive for license details.
+
+package freechips.rocketchip.linting
+package rule
+
+import firrtl._
+import firrtl.ir._
+import firrtl.traversals.Foreachers._
+import firrtl.options.{RegisteredLibrary, ShellOption, Dependency, PreservesAll}
+import firrtl.stage.RunFirrtlTransformAnnotation
+
+/** Template class for lint rules
+  * @note After extending this class, be sure to update the linter list in [[Linter]]
+  */
+abstract class LintRule extends Transform with RegisteredLibrary with DependencyAPIMigration with PreservesAll[Transform] {
+
+  // Name of this rule. Cannot contain spaces!
+  val lintName: String
+
+  // Recommended fix to the user
+  val recommendedFix: String
+
+  lazy val disableCLI: String = s"Omit $lintName from --lint option"
+
+  /** A string representation of using the Chisel/Scala API to whitelist files
+    *
+    * @param files a list of scala files to whitelist
+    */
+  def scalaAPI(files: Seq[String]): String = {
+    val setArg = files.map(f => s""""$f"""").mkString(",")
+    s"""whitelist("$lintName", Set($setArg))"""
+  }
+
+  /** A string representation of using the commandline API to whitelist files
+    *
+    * @param files a list of scala files to whitelist
+    */
+  def whitelistAPI(files: Seq[String]): String = {
+    val arg = files.mkString(",")
+    s"""--${options.head.longOption} $arg"""
+  }
+
+  /** A utiltiy functions to find whitelisted files in the given annotations
+    *
+    * @param annotations Input annotations to find all whitelisted files for this lint rule
+    */
+  def collectWhitelist(annotations: AnnotationSeq): Set[String] = annotations.flatMap {
+    case Whitelist(name, whitelist) if name == lintName => whitelist.toSeq
+    case other => Nil
+  }.toSet
+
+
+  lazy val options = Seq(
+    new ShellOption[String](
+      longOption = s"lint-whitelist:$lintName",
+      toAnnotationSeq = {
+        case whitelist: String => Seq(
+          RunFirrtlTransformAnnotation(this),
+          Whitelist(lintName, whitelist.split(',').toSet)
+        )
+      },
+      helpText = "Enable linting anonymous registers for all files except provided files.",
+      helpValueName = Some("<filename1>.scala[,<filename2>.scala]*")
+    )
+  )
+
+  // Run lint rules after deduplication
+  override def optionalPrerequisites: Seq[Dependency[Transform]] = Seq(Dependency[firrtl.transforms.DedupModules])
+
+  // Run lint rules before the Lint Reporter
+  override def optionalPrerequisiteOf: Seq[Dependency[Transform]] = Seq(Dependency[LintReporter])
+
+  override def execute(state: CircuitState): CircuitState = {
+    val violations = new Violations()
+    val whitelist = collectWhitelist(state.annotations)
+    state.circuit.foreach(lintModule(violations))
+    val errorList = violations.collect {
+      case ((info, message), mods) if !isWhitelisted(info, whitelist) => Violation(this, info, message, mods)
+    }.toSeq.sortBy { _.toString }
+    state.copy(annotations = errorList ++ state.annotations )
+  }
+
+  // Can be overridden by subclass implementations
+  protected def lintModule(violations: Violations)(m: DefModule): Unit = {
+    m.foreach(lintStatement(violations, m.name))
+  }
+
+  // Can be overridden by subclass implementations
+  protected def lintStatement(violations: Violations, mname: String)(s: Statement): Unit = {
+    s.foreach(lintStatement(violations, mname))
+  }
+}