Skip to content

Commit dc54a98

Browse files
mergify[bot]adkian-sifivejackkoenig
authored
Detect bound hardware when processing record elements (backport #3037) (#3115)
* Detect bound hardware when processing record elements (#3037) Use new private Record._elements to validate Record elements. This allows us to check invariants earlier than on cloning or binding. (cherry picked from commit a851566) # Conflicts: # core/src/main/scala/chisel3/internal/BiConnect.scala # core/src/main/scala/chisel3/internal/MonoConnect.scala * Fix backport conflicts --------- Co-authored-by: Aditya Naik <91489422+adkian-sifive@users.noreply.github.com> Co-authored-by: Jack Koenig <koenig@sifive.com>
1 parent 1a485a0 commit dc54a98

File tree

11 files changed

+93
-49
lines changed

11 files changed

+93
-49
lines changed

‎core/src/main/scala/chisel3/Aggregate.scala

Lines changed: 36 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -952,8 +952,8 @@ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptio
952952
}
953953

954954
private def checkClone(clone: Record): Unit = {
955-
for ((name, field) <- elements) {
956-
if (clone.elements(name) eq field) {
955+
for ((name, field) <- _elements) {
956+
if (clone._elements(name) eq field) {
957957
throw new AutoClonetypeException(
958958
s"The bundle plugin was unable to clone $clone that has field '$name' aliased with base $this." +
959959
"This likely happened because you tried nesting Data arguments inside of other data structures." +
@@ -980,10 +980,10 @@ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptio
980980
// which can cause collisions
981981
val _namespace = Namespace.empty
982982
require(
983-
!opaqueType || (elements.size == 1 && elements.head._1 == ""),
984-
s"Opaque types must have exactly one element with an empty name, not ${elements.size}: ${elements.keys.mkString(", ")}"
983+
!opaqueType || (_elements.size == 1 && _elements.head._1 == ""),
984+
s"Opaque types must have exactly one element with an empty name, not ${_elements.size}: ${elements.keys.mkString(", ")}"
985985
)
986-
for ((name, elt) <- elements) {
986+
for ((name, elt) <- _elements) {
987987
elt.setRef(this, _namespace.name(name, leadingDigitOk = true), opaque = opaqueType)
988988
}
989989
}
@@ -1007,7 +1007,7 @@ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptio
10071007
val dupNames = duplicates.toSeq
10081008
.sortBy(_._id)
10091009
.map { duplicate =>
1010-
this.elements.collect { case x if x._2._id == duplicate._id => x }.toSeq
1010+
this._elements.collect { case x if x._2._id == duplicate._id => x }.toSeq
10111011
.sortBy(_._2._id)
10121012
.map(_._1)
10131013
.reverse
@@ -1028,7 +1028,12 @@ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptio
10281028

10291029
checkForAndReportDuplicates()
10301030

1031-
for ((child, sameChild) <- this.elementsIterator.zip(this.elementsIterator)) {
1031+
// This check is for making sure that elements always returns the
1032+
// same object, which will not be the case if the user makes it a
1033+
// def inside the Record. Checking elementsIterator against itself
1034+
// is not useful for this check because it's a lazy val which will
1035+
// always return the same thing.
1036+
for (((_, child), sameChild) <- this.elements.iterator.zip(this.elementsIterator)) {
10321037
if (child != sameChild) {
10331038
throwException(
10341039
s"${this.className} does not return the same objects when calling .elements multiple times. Did you make it a def by mistake?"
@@ -1188,7 +1193,7 @@ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptio
11881193
override def toString: String = {
11891194
topBindingOpt match {
11901195
case Some(BundleLitBinding(_)) =>
1191-
val contents = elements.toList.reverse.map {
1196+
val contents = _elements.toList.reverse.map {
11921197
case (name, data) =>
11931198
s"$name=$data"
11941199
}.mkString(", ")
@@ -1199,6 +1204,22 @@ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptio
11991204

12001205
def elements: SeqMap[String, Data]
12011206

1207+
// Internal representation of Record elements. _elements makes it
1208+
// possible to check for rebinding issues with Record elements
1209+
// without having to recurse over all elements after the Record is
1210+
// constructed. Laziness of _elements means that this check will
1211+
// occur (only) at the first instance _elements is referenced.
1212+
private[chisel3] lazy val _elements: SeqMap[String, Data] = {
1213+
for ((name, field) <- elements) {
1214+
if (field.binding.isDefined) {
1215+
throw RebindingException(
1216+
s"Cannot create Record ${this.className}; element ${field} of Record must be a Chisel type, not hardware."
1217+
)
1218+
}
1219+
}
1220+
elements
1221+
}
1222+
12021223
/** Name for Pretty Printing */
12031224
def className: String = try {
12041225
this.getClass.getSimpleName
@@ -1210,11 +1231,11 @@ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptio
12101231
private[chisel3] override def typeEquivalent(that: Data): Boolean = that match {
12111232
case that: Record =>
12121233
this.getClass == that.getClass &&
1213-
this.elements.size == that.elements.size &&
1214-
this.elements.forall {
1234+
this._elements.size == that._elements.size &&
1235+
this._elements.forall {
12151236
case (name, model) =>
1216-
that.elements.contains(name) &&
1217-
(that.elements(name).typeEquivalent(model))
1237+
that._elements.contains(name) &&
1238+
(that._elements(name).typeEquivalent(model))
12181239
}
12191240
case _ => false
12201241
}
@@ -1223,7 +1244,7 @@ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptio
12231244

12241245
override def getElements: Seq[Data] = elementsIterator.toIndexedSeq
12251246

1226-
final override private[chisel3] def elementsIterator: Iterator[Data] = elements.iterator.map(_._2)
1247+
final override private[chisel3] def elementsIterator: Iterator[Data] = _elements.iterator.map(_._2)
12271248

12281249
// Helper because Bundle elements are reversed before printing
12291250
private[chisel3] def toPrintableHelper(elts: Seq[(String, Data)]): Printable = {
@@ -1241,7 +1262,7 @@ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptio
12411262
* Analogous to printing a Map
12421263
* Results in "`\$className(elt0.name -> elt0.value, ...)`"
12431264
*/
1244-
def toPrintable: Printable = toPrintableHelper(elements.toList)
1265+
def toPrintable: Printable = toPrintableHelper(_elements.toList)
12451266

12461267
/** Implementation of cloneType that is [optionally for Record] overridden by the compiler plugin
12471268
*
@@ -1445,5 +1466,5 @@ abstract class Bundle(implicit compileOptions: CompileOptions) extends Record {
14451466
* @note The order is reversed from the order of elements in order to print
14461467
* the fields in the order they were defined
14471468
*/
1448-
override def toPrintable: Printable = toPrintableHelper(elements.toList.reverse)
1469+
override def toPrintable: Printable = toPrintableHelper(_elements.toList.reverse)
14491470
}

‎core/src/main/scala/chisel3/Data.scala

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ private[chisel3] object cloneSupertype {
219219
private[chisel3] object getRecursiveFields {
220220
def noPath(data: Data): Seq[Data] = data match {
221221
case data: Record =>
222-
data.elements.map {
222+
data._elements.map {
223223
case (_, fieldData) =>
224224
getRecursiveFields.noPath(fieldData)
225225
}.fold(Seq(data)) {
@@ -236,7 +236,7 @@ private[chisel3] object getRecursiveFields {
236236
}
237237
def apply(data: Data, path: String): Seq[(Data, String)] = data match {
238238
case data: Record =>
239-
data.elements.map {
239+
data._elements.map {
240240
case (fieldName, fieldData) =>
241241
getRecursiveFields(fieldData, s"$path.$fieldName")
242242
}.fold(Seq(data -> path)) {
@@ -255,7 +255,7 @@ private[chisel3] object getRecursiveFields {
255255
def lazily(data: Data, path: String): Seq[(Data, String)] = data match {
256256
case data: Record =>
257257
LazyList(data -> path) ++
258-
data.elements.view.flatMap {
258+
data._elements.view.flatMap {
259259
case (fieldName, fieldData) =>
260260
getRecursiveFields(fieldData, s"$path.$fieldName")
261261
}
@@ -270,7 +270,7 @@ private[chisel3] object getRecursiveFields {
270270
def lazilyNoPath(data: Data): Seq[Data] = data match {
271271
case data: Record =>
272272
LazyList(data) ++
273-
data.elements.view.flatMap {
273+
data._elements.view.flatMap {
274274
case (fieldName, fieldData) =>
275275
getRecursiveFields.lazilyNoPath(fieldData)
276276
}
@@ -292,13 +292,13 @@ private[chisel3] object getMatchedFields {
292292
require(x.typeEquivalent(y))
293293
Seq(x -> y)
294294
case (x: Record, y: Record) =>
295-
(x.elements
296-
.zip(y.elements))
295+
(x._elements
296+
.zip(y._elements))
297297
.map {
298298
case ((xName, xElt), (yName, yElt)) =>
299299
require(
300300
xName == yName,
301-
s"$xName != $yName, ${x.elements}, ${y.elements}, $x, $y"
301+
s"$xName != $yName, ${x._elements}, ${y._elements}, $x, $y"
302302
) // assume fields returned in same, deterministic order
303303
getMatchedFields(xElt, yElt)
304304
}
@@ -839,8 +839,8 @@ object Data {
839839
}
840840
implicit class RecordOptGet(rOpt: Option[Record]) {
841841
// Like .get, but its already defined on Option
842-
def grab(k: String): Option[Data] = rOpt.flatMap { _.elements.get(k) }
843-
def keys: Iterable[String] = rOpt.map { r => r.elements.map(_._1) }.getOrElse(Seq.empty[String])
842+
def grab(k: String): Option[Data] = rOpt.flatMap { _._elements.get(k) }
843+
def keys: Iterable[String] = rOpt.map { r => r._elements.map(_._1) }.getOrElse(Seq.empty[String])
844844
}
845845
//TODO(azidar): Rewrite this to be more clear, probably not the cleanest way to express this
846846
private def isDifferent(l: Option[Data], r: Option[Data]): Boolean =
@@ -917,17 +917,17 @@ object Data {
917917
.reduce(_ && _)
918918
}
919919
case (thiz: Record, that: Record) =>
920-
if (thiz.elements.size != that.elements.size) {
920+
if (thiz._elements.size != that._elements.size) {
921921
throwException(s"Cannot compare Bundles $thiz and $that: Bundle types differ")
922922
} else {
923-
thiz.elements.map {
923+
thiz._elements.map {
924924
case (thisName, thisData) =>
925-
if (!that.elements.contains(thisName))
925+
if (!that._elements.contains(thisName))
926926
throwException(
927927
s"Cannot compare Bundles $thiz and $that: field $thisName (from $thiz) was not found in $that"
928928
)
929929

930-
val thatData = that.elements(thisName)
930+
val thatData = that._elements(thisName)
931931

932932
try {
933933
thisData === thatData

‎core/src/main/scala/chisel3/experimental/dataview/DataView.scala

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -608,8 +608,8 @@ object PartialDataView {
608608
mkView,
609609
{
610610
case (a, b) =>
611-
val aElts = a.elements
612-
val bElts = b.elements
611+
val aElts = a._elements
612+
val bElts = b._elements
613613
val bKeys = bElts.keySet
614614
val keys = aElts.keysIterator.filter(bKeys.contains)
615615
keys.map(k => aElts(k) -> bElts(k)).toSeq

‎core/src/main/scala/chisel3/experimental/hierarchy/ModuleClone.scala

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,9 @@ private[chisel3] class ModuleClone[T <: BaseModule](val getProto: T) extends Pse
3333
getProto match {
3434
// BlackBox needs special handling for its pseduo-io Bundle
3535
case protoBB: BlackBox =>
36-
Map(protoBB._io.get -> getPorts.elements("io"))
36+
Map(protoBB._io.get -> getPorts._elements("io"))
3737
case _ =>
38-
val name2Port = getPorts.elements
38+
val name2Port = getPorts._elements
3939
getProto.getChiselPorts.map { case (name, data) => data -> name2Port(name) }.toMap
4040
}
4141
}
@@ -62,7 +62,7 @@ private[chisel3] class ModuleClone[T <: BaseModule](val getProto: T) extends Pse
6262
case _: BlackBox =>
6363
// Override the io Bundle's ref so that it thinks it is the top for purposes of
6464
// generating FIRRTL
65-
record.elements("io").setRef(ref, force = true)
65+
record._elements("io").setRef(ref, force = true)
6666
case _ => // Do nothing
6767
}
6868

‎core/src/main/scala/chisel3/experimental/hierarchy/core/Lookupable.scala

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -133,9 +133,9 @@ object Lookupable {
133133
if (coor.isEmpty) d
134134
else {
135135
val next = (coor.head, d) match {
136-
case (Slot(_, name), rec: Record) => rec.elements(name)
136+
case (Slot(_, name), rec: Record) => rec._elements(name)
137137
case (Index(_, ILit(n)), vec: Vec[_]) => vec.apply(n.toInt)
138-
case (ModuleIO(_, name), rec: Record) => rec.elements(name)
138+
case (ModuleIO(_, name), rec: Record) => rec._elements(name)
139139
case (arg, _) => err(s"Unexpected Arg '$arg' applied to '$d'! Root was '$start'.")
140140
}
141141
applyCoordinates(coor.tail, next)

‎core/src/main/scala/chisel3/experimental/package.scala

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ package object experimental {
100100
}
101101
}
102102
val ports: Seq[Data] =
103-
gen.elements.toSeq.reverse.map {
103+
gen._elements.toSeq.reverse.map {
104104
case (name, data) =>
105105
val p = chisel3.IO(coerceDirection(chiselTypeClone(data).asInstanceOf[Data]))
106106
p.suggestName(name)
@@ -110,7 +110,7 @@ package object experimental {
110110

111111
implicit val dv: DataView[Seq[Data], T] = DataView.mapping(
112112
_ => chiselTypeClone(gen).asInstanceOf[T],
113-
(seq, rec) => seq.zip(rec.elements.toSeq.reverse).map { case (port, (_, field)) => port -> field }
113+
(seq, rec) => seq.zip(rec._elements.toSeq.reverse).map { case (port, (_, field)) => port -> field }
114114
)
115115
ports.viewAs[T]
116116
}

‎core/src/main/scala/chisel3/internal/BiConnect.scala

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -203,7 +203,7 @@ private[chisel3] object BiConnect {
203203
left.firrtlPartialConnect(right)(sourceInfo)
204204
} else {
205205
// For each field in left, descend with right
206-
for ((field, left_sub) <- left_r.elements) {
206+
for ((field, left_sub) <- left_r._elements) {
207207
try {
208208
connect(sourceInfo, connectCompileOptions, left_sub, right, context_mod)
209209
} catch {
@@ -218,7 +218,7 @@ private[chisel3] object BiConnect {
218218
left.firrtlPartialConnect(right)(sourceInfo)
219219
} else {
220220
// For each field in left, descend with right
221-
for ((field, right_sub) <- right_r.elements) {
221+
for ((field, right_sub) <- right_r._elements) {
222222
try {
223223
connect(sourceInfo, connectCompileOptions, left, right_sub, context_mod)
224224
} catch {
@@ -245,16 +245,16 @@ private[chisel3] object BiConnect {
245245
// For each field in left, descend with right.
246246
// Don't bother doing this check if we don't expect it to necessarily pass.
247247
if (connectCompileOptions.connectFieldsMustMatch) {
248-
for ((field, right_sub) <- right_r.elements) {
249-
if (!left_r.elements.isDefinedAt(field)) {
248+
for ((field, right_sub) <- right_r._elements) {
249+
if (!left_r._elements.isDefinedAt(field)) {
250250
throw MissingLeftFieldException(field)
251251
}
252252
}
253253
}
254254
// For each field in left, descend with right
255-
for ((field, left_sub) <- left_r.elements) {
255+
for ((field, left_sub) <- left_r._elements) {
256256
try {
257-
right_r.elements.get(field) match {
257+
right_r._elements.get(field) match {
258258
case Some(right_sub) => connect(sourceInfo, connectCompileOptions, left_sub, right_sub, context_mod)
259259
case None => {
260260
if (connectCompileOptions.connectFieldsMustMatch) {

‎core/src/main/scala/chisel3/internal/MonoConnect.scala

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -182,9 +182,9 @@ private[chisel3] object MonoConnect {
182182
pushCommand(Connect(sourceInfo, sinkReified.get.lref, sourceReified.get.ref))
183183
} else {
184184
// For each field, descend with right
185-
for ((field, sink_sub) <- sink_r.elements) {
185+
for ((field, sink_sub) <- sink_r._elements) {
186186
try {
187-
source_r.elements.get(field) match {
187+
source_r._elements.get(field) match {
188188
case Some(source_sub) => connect(sourceInfo, connectCompileOptions, sink_sub, source_sub, context_mod)
189189
case None => {
190190
if (connectCompileOptions.connectFieldsMustMatch) {
@@ -200,7 +200,7 @@ private[chisel3] object MonoConnect {
200200
// Handle Record connected to DontCare. Apply the DontCare to individual elements.
201201
case (sink_r: Record, DontCare) =>
202202
// For each field, descend with right
203-
for ((field, sink_sub) <- sink_r.elements) {
203+
for ((field, sink_sub) <- sink_r._elements) {
204204
try {
205205
connect(sourceInfo, connectCompileOptions, sink_sub, source, context_mod)
206206
} catch {

‎core/src/main/scala/chisel3/internal/firrtl/Converter.scala

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -342,9 +342,9 @@ private[chisel3] object Converter {
342342
fir.Field(getRef(elt, info).name, fir.Flip, extractType(elt, false, info))
343343
}
344344
if (!d._isOpaqueType)
345-
fir.BundleType(d.elements.toIndexedSeq.reverse.map { case (_, e) => eltField(e) })
345+
fir.BundleType(d._elements.toIndexedSeq.reverse.map { case (_, e) => eltField(e) })
346346
else
347-
extractType(d.elements.head._2, childClearDir, info)
347+
extractType(d._elements.head._2, childClearDir, info)
348348
}
349349
}
350350

‎core/src/main/scala/chisel3/reflect/DataMirror.scala

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,7 @@ object DataMirror {
149149
def getPortNames(name: String, data: Data): Seq[(String, Data)] = Seq(name -> data) ++ (data match {
150150
case _: Element => Seq()
151151
case r: Record =>
152-
r.elements.toSeq.flatMap {
152+
r._elements.toSeq.flatMap {
153153
case (eltName, elt) =>
154154
if (r._isOpaqueType) { getPortNames(s"${name}", elt) }
155155
else { getPortNames(s"${name}_${eltName}", elt) }

‎src/test/scala/chiselTests/RecordSpec.scala

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -422,4 +422,27 @@ class RecordSpec extends ChiselFlatSpec with Utils {
422422

423423
err.getMessage should include("bundle plugin was unable to clone")
424424
}
425+
426+
"Attempting to create a Record with bound nested elements" should "error" in {
427+
class InnerNestedRecord[T <: Data](gen: T) extends Record {
428+
val elements = SeqMap("a" -> gen)
429+
}
430+
class NestedRecord[T <: Data](gen: T) extends Record {
431+
val inner1 = new InnerNestedRecord(gen)
432+
val inner2 = new InnerNestedRecord(UInt(4.W))
433+
val elements = SeqMap("a" -> inner1, "b" -> inner2)
434+
}
435+
class MyRecord[T <: Data](gen: T) extends Record {
436+
val nested = new NestedRecord(gen)
437+
val elements = SeqMap("a" -> nested)
438+
}
439+
440+
val e = the[ChiselException] thrownBy {
441+
ChiselStage.elaborate(new Module {
442+
val myReg = RegInit(0.U(8.W))
443+
val io = IO(Input(new MyRecord(myReg)))
444+
})
445+
}
446+
e.getMessage should include("must be a Chisel type, not hardware")
447+
}
425448
}

0 commit comments

Comments
 (0)