Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check field referential equality in autoclonetype #1047

Merged
merged 9 commits into from
Mar 25, 2019
36 changes: 24 additions & 12 deletions chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala
Original file line number Diff line number Diff line change
Expand Up @@ -658,6 +658,22 @@ abstract class Bundle(implicit compileOptions: CompileOptions) extends Record {
throw new AutoClonetypeException(s"Unable to automatically infer cloneType on $clazz: $desc")
}

def validateClone(clone: Bundle, equivDiagnostic: String): Unit = {
if (!clone.typeEquivalent(this)) {
autoClonetypeError(s"Automatically cloned $clone not type-equivalent to base $this. " + equivDiagnostic)
}

for ((name, field) <- elements) {
if (clone.elements(name) eq field) {
autoClonetypeError(s"Automatically cloned $clone has field $name aliased with base $this." +
" In the future, this can be solved by wrapping the field in Field(...)," +
" see https://github.com/freechipsproject/chisel3/pull/909." +
" For now, ensure Chisel types used in the Bundle definition are passed through constructor arguments," +
" or wrapped in Input(...), Output(...), or Flipped(...) if appropriate.")
}
}
}

val mirror = runtimeMirror(clazz.getClassLoader)
val classSymbolOption = try {
Some(mirror.reflect(this).symbol)
Expand Down Expand Up @@ -741,10 +757,7 @@ abstract class Bundle(implicit compileOptions: CompileOptions) extends Record {
clone match {
case Some(clone) =>
clone._outerInst = this._outerInst
if (!clone.typeEquivalent(this)) {
autoClonetypeError(s"automatically cloned $clone not type-equivalent to base." +
" Constructor argument values were not inferred, ensure constructor is deterministic.")
}
validateClone(clone, "Constructor argument values were not inferred, ensure constructor is deterministic.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not run this on creation of elements? That will always happen on or before cloning

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jackkoenig does #1050 address this concern?

return clone.asInstanceOf[this.type]
case None =>
}
Expand Down Expand Up @@ -781,6 +794,8 @@ abstract class Bundle(implicit compileOptions: CompileOptions) extends Record {
try {
val clone = ctors.head.newInstance(outerClassInstance.get._2).asInstanceOf[this.type]
clone._outerInst = this._outerInst

validateClone(clone, "Outer class instance was inferred, ensure constructor is deterministic.")
return clone
} catch {
case e @ (_: java.lang.reflect.InvocationTargetException | _: IllegalArgumentException) =>
Expand Down Expand Up @@ -844,14 +859,11 @@ abstract class Bundle(implicit compileOptions: CompileOptions) extends Record {
val clone = classMirror.reflectConstructor(ctor).apply(ctorParamsVals:_*).asInstanceOf[this.type]
clone._outerInst = this._outerInst

if (!clone.typeEquivalent(this)) {
// scalastyle:off line.size.limit
autoClonetypeError(s"Automatically cloned $clone not type-equivalent to base $this." +
" Constructor argument values were inferred: ensure that variable names are consistent and have the same value throughout the constructor chain," +
" and that the constructor is deterministic.")
// scalastyle:on line.size.limit
}

validateClone(clone,
"Constructor argument values were inferred:" +
" ensure that variable names are consistent and have the same value throughout the constructor chain," +
" and that the constructor is deterministic."
)
clone
}

Expand Down
25 changes: 25 additions & 0 deletions src/test/scala/chiselTests/AutoClonetypeSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -172,4 +172,29 @@ class AutoClonetypeSpec extends ChiselFlatSpec {
val a = WireDefault(io)
} }
}

"Aliased fields" should "be caught" in {
a [ChiselException] should be thrownBy {
elaborate { new Module {
val bundleFieldType = UInt(8.W)
val io = IO(Output(new Bundle {
val a = bundleFieldType
}))
io.a := 0.U
} }
}
}

"Aliased fields from inadequate autoclonetype" should "be caught" in {
a [ChiselException] should be thrownBy {
class BadBundle(val typeTuple: (Data, Int)) extends Bundle {
val a = typeTuple._1
}

elaborate { new Module {
val io = IO(Output(new BadBundle(UInt(8.W), 1)))
io.a := 0.U
} }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the following also subject to issues related to aliased fields?

new Module {
        val io = IO(Output(new Bundle {
          val a = UInt(8.W)
        }))
      }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't, because it creates a new UInt on the constructor invocation, and auto-clone-type constructs a new Bundle each time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the following would also have aliasing issues?

class AliasedBundle(foo: UInt) extends Bundle {
  val a = foo
  val b = foo
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I don't think that's in the domain of auto-cloneType to check, and under current code it will probably cause a rebinding error anyways.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I don't understand is why autoclonetype can't check for AliasedBundle when it can check for the example in your test...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a automatically generated clone type issue (as in, issue between the original bundle and cloned bundle), it's your Bundle definition is wrong (problem is solely within one Bundle object). And I think it's also caught by the rebinding checks. If you want I'll add a test case for that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

}
}
}