Fix Compatibility Module io wrapping (#2355) (#2358)

The new reflection based IO autowrapping for compatibility mode Modules
would previously throw a NullPointerExceptions if any hardware were
constructed in the Module before "val io" was initialized. The logic is
now more robust for this case.

Co-authored-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
(cherry picked from commit 50e6099fbecc041973564514e55f67ffe069459b)

Co-authored-by: Jack Koenig <koenig@sifive.com>

2 files changed, 35 insertions, 25 deletions

diff --git a/core/src/main/scala/chisel3/BlackBox.scala b/core/src/main/scala/chisel3/BlackBox.scala
index 89c4ccd3..2f04fb2e 100644
--- a/core/src/main/scala/chisel3/BlackBox.scala
+++ b/core/src/main/scala/chisel3/BlackBox.scala
@@ -145,27 +145,30 @@ abstract class BlackBox(
     extends BaseBlackBox {
 
   // Find a Record port named "io" for purposes of stripping the prefix
-  private[chisel3] lazy val _io: Record =
+  private[chisel3] lazy val _io: Option[Record] =
     this
       .findPort("io")
       .collect { case r: Record => r } // Must be a Record
-      .getOrElse(null) // null handling occurs in generateComponent
 
   // Allow access to bindings from the compatibility package
-  protected def _compatIoPortBound() = portsContains(_io)
+  protected def _compatIoPortBound() = _io.exists(portsContains(_))
 
   private[chisel3] override def generateComponent(): Option[Component] = {
     _compatAutoWrapPorts() // pre-IO(...) compatibility hack
 
     // Restrict IO to just io, clock, and reset
-    require(_io != null, "BlackBox must have a port named 'io' of type Record!")
-    require(portsContains(_io), "BlackBox must have io wrapped in IO(...)")
+    if (!_io.forall(portsContains)) {
+      throwException(s"BlackBox '$this' must have a port named 'io' of type Record wrapped in IO(...)!")
+    }
+
     require(portsSize == 1, "BlackBox must only have one IO, called `io`")
 
     require(!_closed, "Can't generate module more than once")
     _closed = true
 
-    val namedPorts = _io.elements.toSeq.reverse // ListMaps are stored in reverse order
+    val io = _io.get
+
+    val namedPorts = io.elements.toSeq.reverse // ListMaps are stored in reverse order
 
     // There is a risk of user improperly attempting to connect directly with io
     // Long term solution will be to define BlackBox IO differently as part of
@@ -181,18 +184,18 @@ abstract class BlackBox(
     // of the io bundle, but NOT on the io bundle itself.
     // Doing so would cause the wrong names to be assigned, since their parent
     // is now the module itself instead of the io bundle.
-    for (id <- getIds; if id ne _io) {
+    for (id <- getIds; if id ne io) {
       id._onModuleClose
     }
 
     val firrtlPorts = namedPorts.map { namedPort => Port(namedPort._2, namedPort._2.specifiedDirection) }
-    val component = DefBlackBox(this, name, firrtlPorts, _io.specifiedDirection, params)
+    val component = DefBlackBox(this, name, firrtlPorts, io.specifiedDirection, params)
     _component = Some(component)
     _component
   }
 
   private[chisel3] def initializeInParent(parentCompileOptions: CompileOptions): Unit = {
-    for ((_, port) <- _io.elements) {
+    for ((_, port) <- _io.map(_.elements).getOrElse(Nil)) {
       pushCommand(DefInvalid(UnlocatableSourceInfo, port.ref))
     }
   }
diff --git a/core/src/main/scala/chisel3/RawModule.scala b/core/src/main/scala/chisel3/RawModule.scala
index b81372d8..12b6a76a 100644
--- a/core/src/main/scala/chisel3/RawModule.scala
+++ b/core/src/main/scala/chisel3/RawModule.scala
@@ -178,11 +178,12 @@ package object internal {
   // Private reflective version of "val io" to maintain Chisel.Module semantics without having
   // io as a virtual method. See https://github.com/freechipsproject/chisel3/pull/1550 for more
   // information about the removal of "val io"
-  private def reflectivelyFindValIO(self: BaseModule): Record = {
+  private def reflectivelyFindValIO(self: BaseModule): Option[Record] = {
     // Java reflection is faster and works for the common case
     def tryJavaReflect: Option[Record] = Try {
       self.getClass.getMethod("io").invoke(self).asInstanceOf[Record]
     }.toOption
+      .filter(_ != null)
     // Anonymous subclasses don't work with Java reflection, so try slower, Scala reflection
     def tryScalaReflect: Option[Record] = {
       val ru = scala.reflect.runtime.universe
@@ -199,6 +200,7 @@ package object internal {
         Try {
           im.reflectField(term).get.asInstanceOf[Record]
         }.toOption
+          .filter(_ != null)
       }
     }
 
@@ -209,13 +211,6 @@ package object internal {
         // Fallback if reflection fails, user can wrap in IO(...)
         self.findPort("io").collect { case r: Record => r }
       }
-      .getOrElse(
-        throwException(
-          s"Compatibility mode Module '$this' must have a 'val io' Bundle. " +
-            "If there is such a field and you still see this error, autowrapping has failed (sorry!). " +
-            "Please wrap the Bundle declaration in IO(...)."
-        )
-      )
   }
 
   /** Legacy Module class that restricts IOs to just io, clock, and reset, and provides a constructor
@@ -243,17 +238,29 @@ package object internal {
     def this(_clock: Clock, _reset: Bool)(implicit moduleCompileOptions: CompileOptions) =
       this(Option(_clock), Option(_reset))(moduleCompileOptions)
 
-    private lazy val _io: Record = reflectivelyFindValIO(this)
+    // Sort of a DIY lazy val because if the user tries to construct hardware before val io is
+    // constructed, _compatAutoWrapPorts will try to access it but it will be null
+    // In that case, we basically need to delay setting this var until later
+    private var _ioValue: Option[Record] = None
+    private def _io: Option[Record] = _ioValue.orElse {
+      _ioValue = reflectivelyFindValIO(this)
+      _ioValue
+    }
 
     // Allow access to bindings from the compatibility package
-    protected def _compatIoPortBound() = portsContains(_io)
+    protected def _compatIoPortBound() = _io.exists(portsContains(_))
 
     private[chisel3] override def generateComponent(): Option[Component] = {
       _compatAutoWrapPorts() // pre-IO(...) compatibility hack
 
       // Restrict IO to just io, clock, and reset
-      require(_io != null, "Module must have io")
-      require(portsContains(_io), "Module must have io wrapped in IO(...)")
+      if (_io.isEmpty || !_compatIoPortBound) {
+        throwException(
+          s"Compatibility mode Module '$this' must have a 'val io' Bundle. " +
+            "If there is such a field and you still see this error, autowrapping has failed (sorry!). " +
+            "Please wrap the Bundle declaration in IO(...)."
+        )
+      }
       require(
         (portsContains(clock)) && (portsContains(reset)),
         "Internal error, module did not have clock or reset as IO"
@@ -264,8 +271,8 @@ package object internal {
     }
 
     override def _compatAutoWrapPorts(): Unit = {
-      if (!_compatIoPortBound() && _io != null) {
-        _bindIoInPlace(_io)
+      if (!_compatIoPortBound()) {
+        _io.foreach(_bindIoInPlace(_))
       }
     }
   }
@@ -283,13 +290,13 @@ package object internal {
     implicit moduleCompileOptions: CompileOptions)
       extends chisel3.BlackBox(params) {
 
-    override private[chisel3] lazy val _io: Record = reflectivelyFindValIO(this)
+    override private[chisel3] lazy val _io: Option[Record] = reflectivelyFindValIO(this)
 
     // This class auto-wraps the BlackBox with IO(...), allowing legacy code (where IO(...) wasn't
     // required) to build.
     override def _compatAutoWrapPorts(): Unit = {
       if (!_compatIoPortBound()) {
-        _bindIoInPlace(_io)
+        _io.foreach(_bindIoInPlace(_))
       }
     }
   }