From 0d71c69ae104f1400020af23a11334bed00c8f7e Mon Sep 17 00:00:00 2001 From: Adam Izraelevitz Date: Wed, 6 May 2020 10:04:21 -0700 Subject: [PATCH] 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 | 3 +- .../services/firrtl.options.RegisteredLibrary | 3 + src/main/scala/linting/LintAnnotation.scala | 36 ++++++++ src/main/scala/linting/LintException.scala | 58 ++++++++++++ src/main/scala/linting/LintReporter.scala | 82 +++++++++++++++++ src/main/scala/linting/Linter.scala | 33 +++++++ .../linting/rule/LintAnonymousRegisters.scala | 34 +++++++ src/main/scala/linting/rule/LintRule.scala | 92 +++++++++++++++++++ .../linting/rule/LintTruncatingWidths.scala | 42 +++++++++ src/main/scala/linting/rule/package.scala | 86 +++++++++++++++++ 10 files changed, 468 insertions(+), 1 deletion(-) create mode 100644 src/main/resources/META-INF/services/firrtl.options.RegisteredLibrary create mode 100644 src/main/scala/linting/LintAnnotation.scala create mode 100644 src/main/scala/linting/LintException.scala create mode 100644 src/main/scala/linting/LintReporter.scala create mode 100644 src/main/scala/linting/Linter.scala create mode 100644 src/main/scala/linting/rule/LintAnonymousRegisters.scala create mode 100644 src/main/scala/linting/rule/LintRule.scala create mode 100644 src/main/scala/linting/rule/LintTruncatingWidths.scala create mode 100644 src/main/scala/linting/rule/package.scala diff --git a/emulator/Makefrag-verilator b/emulator/Makefrag-verilator index 2f4ff576ba5..2ad4ed4bb2f 100644 --- a/emulator/Makefrag-verilator +++ b/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) && \ diff --git a/src/main/resources/META-INF/services/firrtl.options.RegisteredLibrary b/src/main/resources/META-INF/services/firrtl.options.RegisteredLibrary new file mode 100644 index 00000000000..3400674a0dc --- /dev/null +++ b/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 diff --git a/src/main/scala/linting/LintAnnotation.scala b/src/main/scala/linting/LintAnnotation.scala new file mode 100644 index 00000000000..ecf180b914f --- /dev/null +++ b/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 diff --git a/src/main/scala/linting/LintException.scala b/src/main/scala/linting/LintException.scala new file mode 100644 index 00000000000..10c1dedad6e --- /dev/null +++ b/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}=,... + |""".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") + } +} diff --git a/src/main/scala/linting/LintReporter.scala b/src/main/scala/linting/LintReporter.scala new file mode 100644 index 00000000000..3127fd25d83 --- /dev/null +++ b/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("[*]|[,,...]") + ), + 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=][,display:=]") + ) + ) + + // 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) + } +} diff --git a/src/main/scala/linting/Linter.scala b/src/main/scala/linting/Linter.scala new file mode 100644 index 00000000000..7f790f203fa --- /dev/null +++ b/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 +} diff --git a/src/main/scala/linting/rule/LintAnonymousRegisters.scala b/src/main/scala/linting/rule/LintAnonymousRegisters.scala new file mode 100644 index 00000000000..0192746c6f9 --- /dev/null +++ b/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) + } +} diff --git a/src/main/scala/linting/rule/LintRule.scala b/src/main/scala/linting/rule/LintRule.scala new file mode 100644 index 00000000000..4b053a8cbc2 --- /dev/null +++ b/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(".scala[,.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)) + } +} diff --git a/src/main/scala/linting/rule/LintTruncatingWidths.scala b/src/main/scala/linting/rule/LintTruncatingWidths.scala new file mode 100644 index 00000000000..9f819b68ba7 --- /dev/null +++ b/src/main/scala/linting/rule/LintTruncatingWidths.scala @@ -0,0 +1,42 @@ +// See LICENSE.SiFive for license details. + +package freechips.rocketchip.linting +package rule + +import firrtl.ir._ +import firrtl.options.Dependency + +/** Reports all connections from a wider signal to a smaller signal + * Includes subfields of bulk connections + */ +final class LintTruncatingWidths extends LintRule { + + override def optionalPrerequisites = Seq( + Dependency(firrtl.passes.ExpandConnects), // Require expanding connects to see subfield bulk assignments + Dependency[firrtl.passes.InferWidths] // Require widths to have been inferred + ) + + // Run prior to expand whens to get better fileinfo information + override def optionalPrerequisiteOf = super.optionalPrerequisiteOf :+ Dependency[firrtl.passes.ExpandWhensAndCheck] + + val lintName: String = "trunc-widths" + + val recommendedFix: String = "Truncate width prior to connections" + + override protected def lintStatement(violations: Violations, mname: String)(s: Statement): Unit = { + s match { + case c@Connect(info, loc, expr) => (loc.tpe, expr.tpe) match { + case (GroundType(IntWidth(locWidth)), GroundType(IntWidth(exprWidth))) if exprWidth > locWidth => + val message = s"${c.copy(info = NoInfo).serialize} // Connecting width ${exprWidth} to width ${locWidth}" + getScalaInfo(info) match { + case Some(scalaInfo: FileInfo) => + updateViolations(scalaInfo, message, violations, mname) + case None => updateViolations(info, message, violations, mname) + } + case other => + } + case other => + } + super.lintStatement(violations, mname)(s) + } +} diff --git a/src/main/scala/linting/rule/package.scala b/src/main/scala/linting/rule/package.scala new file mode 100644 index 00000000000..46b74da6fa1 --- /dev/null +++ b/src/main/scala/linting/rule/package.scala @@ -0,0 +1,86 @@ +// See LICENSE.SiFive for license details. + +package freechips.rocketchip.linting + +import firrtl._ +import firrtl.ir._ +import scala.collection.mutable + +package object rule { + /** Determines whether name is prepended with an underscore, indicating a bad name + * + * @param name a signal's name + */ + private [linting] def isTemporary(name: String): Boolean = name.nonEmpty && name.head == '_' + + /** Determines whether name is prepended with an underscore, indicating a bad name + * + * @param expr a expression of a signal name + */ + private [linting] def isTemporary(expr: Expression): Boolean = isTemporary(getName(expr)) + + /** Returns the root reference name of an Expression + * + * @throws Exception + * @param expr an expression of a signal name. Cannot contain DoPrims or Muxes etc. + */ + private [linting] def getName(expr: Expression): String = expr match { + case r: WRef => r.name + case f: WSubField => getName(f.expr) + case i: WSubIndex => getName(i.expr) + case a: WSubAccess => getName(a.expr) + case other => throw new Exception(s"Unexpected match! $other") + } + + /** Splits an info into non-nested Infos + * Right now the FIRRTL parser concatenates the infos if using both .fir and .scala source locators, instead of using + * MultiInfo. This code will work with both the current concatenation and a future FIRRTL change to migrate to MultiInfo. + * + * @param info given fileinfo to flatten + */ + private [linting] def flatten(info: Info): Seq[FileInfo] = info match { + case MultiInfo(seq) => seq.flatMap(flatten) + case f: FileInfo => + val infoRegex = "\\s*(.*\\.scala \\d+:\\d+):(.*\\.fir@\\d+\\.\\d+)\\s*".r + f.info.serialize match { + case infoRegex(scala, fir) => Seq(FileInfo(StringLit(scala)), FileInfo(StringLit(fir))) + case other => Seq(f) + } + case other => Nil + } + + /** Returns the first .scala source location contained inside info + * + * @param info given fileinfo to find scala fileinfo + */ + private [linting] def getScalaInfo(info: Info): Option[FileInfo] = flatten(info).collectFirst { + case i: FileInfo if i.serialize.contains("scala") => i + } + + /** Returns whether the given file is contained in the whiteList + * + * @param info given fileinfo to determine if it is whitelisted + * @param whiteList list of files to exempt from lint rule + */ + private [linting] def isWhitelisted(info: Info, whiteList: Set[String]): Boolean = { + flatten(info).exists { i => + val file = i.info.serialize.split(' ').head + whiteList.contains(file) + } + } + + /** Records a linting violation + * + * @param info given fileinfo of the violation + * @param message Message to include in the violation report + * @param violations container of existing violations + * @param mname module name containing the violation + */ + private [linting] def updateViolations(info: Info, message: String, violations: Violations, mname: String): Unit = { + val mods = violations.getOrElse((info, message), Set.empty) + violations((info, message)) = mods + mname + } + + // Container of violations + private [linting] type Violations = mutable.HashMap[(Info, String), Set[String]] +}