diff --git a/core/src/main/scala/chisel3/Aggregate.scala b/core/src/main/scala/chisel3/Aggregate.scala index 0ef52b2084a..9cf4c65d0dd 100644 --- a/core/src/main/scala/chisel3/Aggregate.scala +++ b/core/src/main/scala/chisel3/Aggregate.scala @@ -981,16 +981,13 @@ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptio // of hardware created when connecting to one of these elements private def setElementRefs(): Unit = { val opaqueType = this._isOpaqueType - // Since elements is a map, it is impossible for two elements to have the same - // identifier; however, Namespace sanitizes identifiers to make them legal for Firrtl/Verilog - // which can cause collisions - val _namespace = Namespace.empty require( !opaqueType || (_elements.size == 1 && _elements.head._1 == ""), s"Opaque types must have exactly one element with an empty name, not ${_elements.size}: ${elements.keys.mkString(", ")}" ) + // Names of _elements have already been namespaced (and therefore sanitized) for ((name, elt) <- _elements) { - elt.setRef(this, _namespace.name(name, leadingDigitOk = true), opaque = opaqueType) + elt.setRef(this, name, opaque = opaqueType) } } @@ -1215,15 +1212,25 @@ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptio // without having to recurse over all elements after the Record is // constructed. Laziness of _elements means that this check will // occur (only) at the first instance _elements is referenced. + // Also used to sanitize names private[chisel3] lazy val _elements: SeqMap[String, Data] = { - for ((name, field) <- elements) { - if (field.binding.isDefined) { - throw RebindingException( - s"Cannot create Record ${this.className}; element ${field} of Record must be a Chisel type, not hardware." - ) - } + // Since elements is a map, it is impossible for two elements to have the same + // identifier; however, Namespace sanitizes identifiers to make them legal for Firrtl/Verilog + // which can cause collisions + // Note that OpaqueTypes cannot have sanitization (the name of the element needs to stay empty) + // Use an empty Namespace to indicate OpaqueType + val namespace = if (!this._isOpaqueType) Some(Namespace.empty) else None + elements.map { + case (name, field) => + if (field.binding.isDefined) { + throw RebindingException( + s"Cannot create Record ${this.className}; element ${field} of Record must be a Chisel type, not hardware." + ) + } + // namespace.name also sanitizes for firrtl, leave name alone for OpaqueTypes + val sanitizedName = namespace.map(_.name(name, leadingDigitOk = true)).getOrElse(name) + sanitizedName -> field } - elements } /** Name for Pretty Printing */ diff --git a/src/test/scala/chiselTests/PrintableSpec.scala b/src/test/scala/chiselTests/PrintableSpec.scala index 2d334f838f1..387feedd0ca 100644 --- a/src/test/scala/chiselTests/PrintableSpec.scala +++ b/src/test/scala/chiselTests/PrintableSpec.scala @@ -177,6 +177,20 @@ class PrintableSpec extends AnyFlatSpec with Matchers with Utils { } } + it should "print use the sanitized names of Bundle elements" in { + class MyModule extends Module { + class UnsanitaryBundle extends Bundle { + val `a-x` = UInt(8.W) + } + val myBun = Wire(new UnsanitaryBundle) + myBun.`a-x` := 0.U + printf(p"$myBun") + } + generateAndCheck(new MyModule) { + case Seq(Printf("UnsanitaryBundle(aminusx -> %d)", Seq("myBun.aminusx"))) => + } + } + // Unit tests for cf it should "print regular scala variables with cf format specifier" in { diff --git a/src/test/scala/chiselTests/experimental/hierarchy/Examples.scala b/src/test/scala/chiselTests/experimental/hierarchy/Examples.scala index 27725c4979b..64cba72fbe1 100644 --- a/src/test/scala/chiselTests/experimental/hierarchy/Examples.scala +++ b/src/test/scala/chiselTests/experimental/hierarchy/Examples.scala @@ -294,6 +294,17 @@ object Examples { @public val nested = new NestedInstantiable(in = leafIn, out = leafOut) } + @instantiable + class HasUnsanitaryBundleField extends Module { + class Interface extends Bundle { + val `a-x` = UInt(8.W) + } + val realIn = IO(Input(new Interface)) + // It's important to have this redirection to trip an old bug + @public val in = realIn.`a-x` + @public val out = IO(Output(UInt(8.W))) + out := in + } class AddTwoNestedInstantiableData(width: Int) extends Module { val in = IO(Input(UInt(width.W))) diff --git a/src/test/scala/chiselTests/experimental/hierarchy/InstanceSpec.scala b/src/test/scala/chiselTests/experimental/hierarchy/InstanceSpec.scala index 483c4920582..d9491b89cc9 100644 --- a/src/test/scala/chiselTests/experimental/hierarchy/InstanceSpec.scala +++ b/src/test/scala/chiselTests/experimental/hierarchy/InstanceSpec.scala @@ -43,7 +43,7 @@ class InstanceSpec extends ChiselFunSpec with Utils { val chirrtl = circt.stage.ChiselStage.emitCHIRRTL(new Top) chirrtl should include("inst i0 of AddOne") } - it("0.3: BlackBoxes should be supported") { + it("(0.d): BlackBoxes should be supported") { class Top extends Module { val in = IO(Input(UInt(32.W))) val out = IO(Output(UInt(32.W))) @@ -66,6 +66,15 @@ class InstanceSpec extends ChiselFunSpec with Utils { chirrtl should include("i1.in <= io.in") chirrtl should include("io.out <= i1.out") } + it("(0.e): Instances with Bundles with unsanitary names should be supported") { + class Top extends Module { + val definition = Definition(new HasUnsanitaryBundleField) + val i0 = Instance(definition) + i0.in := 123.U + } + val chirrtl = circt.stage.ChiselStage.emitCHIRRTL(new Top) + chirrtl should include("""i0.realIn.aminusx <= UInt<7>("h7b")""") + } } describe("(1) Annotations on instances in same chisel compilation") { it("(1.a): should work on a single instance, annotating the instance") {