From c51fcfea32b6c73e623657442460fb782ff0733b Mon Sep 17 00:00:00 2001 From: Aditya Naik Date: Thu, 10 Nov 2022 13:41:00 -0800 Subject: Warn on S-interpolator usage for assert, assume and printf (backport #2751) (#2757) * Add internal methods to maintain binary compatibility Co-authored-by: Megan Wachs Co-authored-by: Jack Koenig --- build.sbt | 1 - core/src/main/scala/chisel3/Printf.scala | 52 ++++++++++++++++++-- .../main/scala/chisel3/VerificationStatement.scala | 56 ++++++++++++++++++++-- docs/src/explanations/printing.md | 14 +++++- src/test/scala/chiselTests/IntervalSpec.scala | 7 +-- src/test/scala/chiselTests/Vec.scala | 4 +- src/test/scala/chiselTests/VecLiteralSpec.scala | 4 +- 7 files changed, 120 insertions(+), 18 deletions(-) diff --git a/build.sbt b/build.sbt index 88374949..a51d1d2e 100644 --- a/build.sbt +++ b/build.sbt @@ -297,7 +297,6 @@ lazy val docs = project // new documentation project .settings(commonSettings) .settings( scalacOptions ++= Seq( - "-Xfatal-warnings", "-language:reflectiveCalls", "-language:implicitConversions" ), diff --git a/core/src/main/scala/chisel3/Printf.scala b/core/src/main/scala/chisel3/Printf.scala index 9410a409..a7338072 100644 --- a/core/src/main/scala/chisel3/Printf.scala +++ b/core/src/main/scala/chisel3/Printf.scala @@ -2,10 +2,11 @@ package chisel3 -import scala.language.experimental.macros import chisel3.internal._ import chisel3.internal.Builder.pushCommand import chisel3.internal.sourceinfo.SourceInfo +import scala.language.experimental.macros +import scala.reflect.macros.blackbox /** Prints a message in simulation * @@ -76,7 +77,44 @@ object printf { * @param data format string varargs containing data to print */ def apply(fmt: String, data: Bits*)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): Printf = - apply(Printable.pack(fmt, data: _*)) + macro _applyMacroWithInterpolatorCheck + + def _applyMacroWithInterpolatorCheck( + c: blackbox.Context + )(fmt: c.Tree, + data: c.Tree* + )(sourceInfo: c.Tree, + compileOptions: c.Tree + ): c.Tree = { + import c.universe._ + fmt match { + case q"scala.StringContext.apply(..$_).s(..$_)" => + c.warning( + c.enclosingPosition, + "The s-interpolator prints the Scala .toString of Data objects rather than the value " + + "of the hardware wire during simulation. Use the cf-interpolator instead. If you want " + + "an elaboration time print, use println." + ) + case _ => + } + val apply_impl_do = symbolOf[this.type].asClass.module.info.member(TermName("printfWithReset")) + q"$apply_impl_do(_root_.chisel3.Printable.pack($fmt, ..$data))($sourceInfo, $compileOptions)" + } + + // Private internal methods that serve to maintain binary + // compatibility after interpolator check updates + @deprecated("This Printf.apply method has been deprecated and will be removed in Chisel 3.6") + def apply(fmt: String, sourceInfo: SourceInfo, compileOptions: CompileOptions): Printf = + apply(fmt, Nil, sourceInfo, compileOptions) + + @deprecated("This Printf.apply method has been deprecated and will be removed in Chisel 3.6") + def apply( + fmt: String, + data: Seq[Bits], + sourceInfo: SourceInfo, + compileOptions: CompileOptions + ): Printf = + apply(Printable.pack(fmt, data: _*))(sourceInfo, compileOptions) /** Prints a message in simulation * @@ -92,7 +130,15 @@ object printf { * @see [[Printable]] documentation * @param pable [[Printable]] to print */ - def apply(pable: Printable)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): Printf = { + def apply(pable: Printable)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): Printf = + printfWithReset(pable)(sourceInfo, compileOptions) + + private[chisel3] def printfWithReset( + pable: Printable + )( + implicit sourceInfo: SourceInfo, + compileOptions: CompileOptions + ): Printf = { var printfId: Printf = null when(!Module.reset.asBool) { printfId = printfWithoutReset(pable) diff --git a/core/src/main/scala/chisel3/VerificationStatement.scala b/core/src/main/scala/chisel3/VerificationStatement.scala index 10cece60..a0040d78 100644 --- a/core/src/main/scala/chisel3/VerificationStatement.scala +++ b/core/src/main/scala/chisel3/VerificationStatement.scala @@ -48,7 +48,7 @@ object assert extends VerifPrintMacrosDoc { )( implicit sourceInfo: SourceInfo, compileOptions: CompileOptions - ): Assert = macro _applyMacroWithStringMessage + ): Assert = macro _applyMacroWithInterpolatorCheck /** Checks for a condition to be valid in the circuit at all times. If the * condition evaluates to false, the circuit simulation stops with an error. @@ -79,6 +79,32 @@ object assert extends VerifPrintMacrosDoc { def apply(cond: Bool)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): Assert = macro _applyMacroWithNoMessage + import VerificationStatement._ + + /** @group VerifPrintMacros */ + def _applyMacroWithInterpolatorCheck( + c: blackbox.Context + )(cond: c.Tree, + message: c.Tree, + data: c.Tree* + )(sourceInfo: c.Tree, + compileOptions: c.Tree + ): c.Tree = { + import c.universe._ + message match { + case q"scala.StringContext.apply(..$_).s(..$_)" => + c.warning( + c.enclosingPosition, + "The s-interpolator prints the Scala .toString of Data objects rather than the value " + + "of the hardware wire during simulation. Use the cf-interpolator instead. If you want " + + "an elaboration time check, call assert with a Boolean condition." + ) + case _ => + } + val apply_impl_do = symbolOf[this.type].asClass.module.info.member(TermName("_applyWithSourceLinePrintable")) + q"$apply_impl_do($cond, ${getLine(c)}, _root_.scala.Some(_root_.chisel3.Printable.pack($message, ..$data)))($sourceInfo, $compileOptions)" + } + /** An elaboration-time assertion. Calls the built-in Scala assert function. */ def apply(cond: Boolean, message: => String): Unit = Predef.assert(cond, message) @@ -88,8 +114,6 @@ object assert extends VerifPrintMacrosDoc { /** Named class for assertions. */ final class Assert private[chisel3] () extends VerificationStatement - import VerificationStatement._ - /** @group VerifPrintMacros */ @deprecated( "This method has been deprecated in favor of _applyMacroWithStringMessage. Please use the same.", @@ -215,7 +239,7 @@ object assume extends VerifPrintMacrosDoc { )( implicit sourceInfo: SourceInfo, compileOptions: CompileOptions - ): Assume = macro _applyMacroWithStringMessage + ): Assume = macro _applyMacroWithInterpolatorCheck /** Assumes a condition to be valid in the circuit at all times. * Acts like an assertion in simulation and imposes a declarative @@ -256,6 +280,30 @@ object assume extends VerifPrintMacrosDoc { import VerificationStatement._ + /** @group VerifPrintMacros */ + def _applyMacroWithInterpolatorCheck( + c: blackbox.Context + )(cond: c.Tree, + message: c.Tree, + data: c.Tree* + )(sourceInfo: c.Tree, + compileOptions: c.Tree + ): c.Tree = { + import c.universe._ + message match { + case q"scala.StringContext.apply(..$_).s(..$_)" => + c.warning( + c.enclosingPosition, + "The s-interpolator prints the Scala .toString of Data objects rather than the value " + + "of the hardware wire during simulation. Use the cf-interpolator instead. If you want " + + "an elaboration time check, call assert with a Boolean condition." + ) + case _ => + } + val apply_impl_do = symbolOf[this.type].asClass.module.info.member(TermName("_applyWithSourceLinePrintable")) + q"$apply_impl_do($cond, ${getLine(c)}, _root_.scala.Some(_root_.chisel3.Printable.pack($message, ..$data)))($sourceInfo, $compileOptions)" + } + /** @group VerifPrintMacros */ @deprecated( "This method has been deprecated in favor of _applyMacroWithStringMessage. Please use the same.", diff --git a/docs/src/explanations/printing.md b/docs/src/explanations/printing.md index 71a6f5cf..80e9ec70 100644 --- a/docs/src/explanations/printing.md +++ b/docs/src/explanations/printing.md @@ -13,11 +13,23 @@ Chisel provides the `printf` function for debugging purposes. It comes in two fl ### Scala-style -Chisel also supports printf in a style similar to [Scala's String Interpolation](http://docs.scala-lang.org/overviews/core/string-interpolation.html). Chisel provides a custom string interpolator `cf` which follows C-style format specifiers (see section [C-style](#c-style) below). Here's a few examples of using the `cf` interpolator: +Chisel also supports printf in a style similar to [Scala's String Interpolation](http://docs.scala-lang.org/overviews/core/string-interpolation.html). Chisel provides a custom string interpolator `cf` which follows C-style format specifiers (see section [C-style](#c-style) below). + +Note that the Scala s-interpolator is not supported in Chisel constructs and will issue a compile-time warning: ```scala mdoc:invisible import chisel3._ ``` + +```scala mdoc:warn +class MyModule extends Module { + val in = IO(Input(UInt(8.W))) + printf(s"in = $in\n") +} +``` + +Instead, use Chisel's `cf` interpolator as in the following examples: + ```scala mdoc:compile-only val myUInt = 33.U printf(cf"myUInt = $myUInt") // myUInt = 33 diff --git a/src/test/scala/chiselTests/IntervalSpec.scala b/src/test/scala/chiselTests/IntervalSpec.scala index c0338f6d..a2d36579 100644 --- a/src/test/scala/chiselTests/IntervalSpec.scala +++ b/src/test/scala/chiselTests/IntervalSpec.scala @@ -211,7 +211,7 @@ class ClipSqueezeWrapDemo( val wrapped = counter.wrap(0.U.asInterval(targetRange)) when(counter === startValue) { - printf(s"Target range is $range\n") + printf(cf"Target range is $range\n") printf("value clip squeeze wrap\n") } @@ -245,10 +245,7 @@ class SqueezeFunctionalityTester(range: IntervalRange, startNum: BigDecimal, end squeezeTemplate := toSqueeze.squeeze(squeezeInterval) printf( - s"SqueezeTest %d %d.squeeze($range) => %d\n", - counter, - toSqueeze.asSInt(), - squeezeTemplate.asSInt() + cf"SqueezeTest $counter%d ${toSqueeze.asSInt()}%d.squeeze($range) => ${squeezeTemplate.asSInt()}%d\n" ) } diff --git a/src/test/scala/chiselTests/Vec.scala b/src/test/scala/chiselTests/Vec.scala index 4a871890..e46774dd 100644 --- a/src/test/scala/chiselTests/Vec.scala +++ b/src/test/scala/chiselTests/Vec.scala @@ -111,7 +111,7 @@ class FillTester(n: Int, value: Int) extends BasicTester { val x = VecInit(Array.fill(n)(value.U)) val u = VecInit.fill(n)(value.U) - assert(x.asUInt() === u.asUInt(), s"Expected Vec to be filled like $x, instead VecInit.fill created $u") + assert(x.asUInt() === u.asUInt(), cf"Expected Vec to be filled like $x, instead VecInit.fill created $u") stop() } @@ -235,7 +235,7 @@ class IterateTester(start: Int, len: Int)(f: UInt => UInt) extends BasicTester { val testVec = VecInit.iterate(start.U, len)(f) assert( controlVec.asUInt() === testVec.asUInt(), - s"Expected Vec to be filled like $controlVec, instead creaeted $testVec\n" + cf"Expected Vec to be filled like $controlVec, instead created $testVec\n" ) stop() } diff --git a/src/test/scala/chiselTests/VecLiteralSpec.scala b/src/test/scala/chiselTests/VecLiteralSpec.scala index fa97a8c8..e2eb791d 100644 --- a/src/test/scala/chiselTests/VecLiteralSpec.scala +++ b/src/test/scala/chiselTests/VecLiteralSpec.scala @@ -205,7 +205,7 @@ class VecLiteralSpec extends ChiselFreeSpec with Utils { assertTesterPasses { new BasicTester { - chisel3.assert(outsideVecLit(0) === 0xdd.U, s"v(0)") + chisel3.assert(outsideVecLit(0) === 0xdd.U, "v(0)") stop() } } @@ -216,7 +216,7 @@ class VecLiteralSpec extends ChiselFreeSpec with Utils { assertTesterPasses { new BasicTester { - chisel3.assert(outsideVecLit(0) === 0xdd.U, s"v(0)") + chisel3.assert(outsideVecLit(0) === 0xdd.U, "v(0)") chisel3.assert(outsideVecLit(1) === 0xcc.U) chisel3.assert(outsideVecLit(2) === 0xbb.U) chisel3.assert(outsideVecLit(3) === 0xaa.U) -- cgit v1.2.3