Merge pull request The-OpenROAD-Project#8721 from The-OpenROAD-Project-staging/secure-fix-replace-hier-mod2-reg

odb: Fixed a _dbModule::copyModuleInsts() bug that connects to a wrong external net

Author: maliberty

src/odb/include/odb/db.h

@@ -2504,6 +2504,13 @@ class dbNet : public dbObject
   ///
   void renameWithModNetInHighestHier();
 
+  ///
+  /// Check if this net is internal to the given module.
+  /// A net is internal if all its iterms belong to instances within the module
+  /// and it has no bterms.
+  ///
+  bool isInternalTo(dbModule* module) const;
+
   ///
   /// Check issues such as multiple drivers, no driver, or dangling net
   ///

src/odb/src/db/dbModule.cpp

@@ -519,7 +519,7 @@ std::vector<dbInst*> dbModule::getLeafInsts()
 dbModBTerm* dbModule::findModBTerm(const char* name)
 {
   std::string modbterm_name(name);
-  char hier_delimiter = getOwner()->getHierarchyDelimiter();
+  const char hier_delimiter = getOwner()->getHierarchyDelimiter();
   size_t last_idx = modbterm_name.find_last_of(hier_delimiter);
   if (last_idx != std::string::npos) {
     modbterm_name = modbterm_name.substr(last_idx + 1);
@@ -790,6 +790,7 @@ void _dbModule::copyModuleInsts(dbModule* old_module,
                                 ITMap& it_map)
 {
   dbBlock* block = new_module->getOwner();
+  const char hier_delimiter = block->getHierarchyDelimiter();
   utl::Logger* logger = old_module->getImpl()->getLogger();
 
   // Create a net name map (key: new net name, value: new dbNet*).
@@ -803,7 +804,7 @@ void _dbModule::copyModuleInsts(dbModule* old_module,
     std::string new_inst_name;
     if (new_mod_inst) {
       new_inst_name = new_mod_inst->getHierarchicalName();
-      new_inst_name += block->getHierarchyDelimiter();
+      new_inst_name += hier_delimiter;
     }
 
     new_inst_name += block->getBaseName(old_inst->getConstName());
@@ -851,45 +852,74 @@ void _dbModule::copyModuleInsts(dbModule* old_module,
                  old_iterm->getName(),
                  new_iterm->getName());
       dbNet* old_net = old_iterm->getNet();
-      if (old_net) {
-        // Create a local net only if it connects to iterms inside this module
-        std::string new_net_name;
-        if (new_mod_inst) {
-          new_net_name = new_mod_inst->getHierarchicalName();
-          new_net_name += block->getHierarchyDelimiter();
-        }
-        std::string old_net_name = old_net->getName();
-        new_net_name += block->getBaseName(old_net_name.c_str());
-
-        auto it = new_net_name_map.find(new_net_name);
-        if (it != new_net_name_map.end()) {
-          // Connect to an existing local net
-          dbNet* new_net = (*it).second;
-          new_iterm->connect(new_net);
-          debugPrint(logger,
-                     utl::ODB,
-                     "replace_design",
-                     1,
-                     "  connected iterm '{}' to existing local net '{}'",
-                     new_iterm->getName(),
-                     new_net->getName());
-        } else {
-          // Create and connect to a new local net
-          assert(block->findNet(new_net_name.c_str()) == nullptr);
-          dbNet* new_net
-              = dbNet::create(new_module->getOwner(), new_net_name.c_str());
-          new_iterm->connect(new_net);
-          debugPrint(logger,
-                     utl::ODB,
-                     "replace_design",
-                     1,
-                     "  Connected iterm '{}' to new local net '{}'",
-                     new_iterm->getName(),
-                     new_net->getName());
+      if (old_net == nullptr) {
+        continue;
+      }
 
-          // Insert it to the map
-          new_net_name_map[new_net_name] = new_net;
-        }
+      //
+      // Create a local net only if it connects to iterms inside this module
+      //
+      std::string new_net_name;
+      if (new_mod_inst) {
+        new_net_name = new_mod_inst->getHierarchicalName();
+        new_net_name += hier_delimiter;
+      }
+
+      // Check if the flat net is an internal net within old_module
+      // - If old_module is uninstantiated module, every net in the module is
+      //   an internal net.
+      //   e.g., No module instance.
+      //         net_name = "_001_"     <-- Internal net.
+      //
+      // - Otherwise, an internal net should have the hierarchy prefix
+      //   (= module instance hierarchical name).
+      //   e.g., modinst_name = "u0/alu0"
+      //         net_name = u0/alu0/_001_   <-- Internal net.
+      //         net_name = u0/_001_        <-- External net crossing module
+      //                                        boundary.
+      std::string old_net_name = old_net->getName();
+      if (old_net->isInternalTo(old_module) == false) {
+        // Skip external net crossing module boundary.
+        // It will be connected later.
+        debugPrint(logger,
+                   utl::ODB,
+                   "replace_design",
+                   3,
+                   "    Skip: non-internal dbNet '{}' of old_module '{}'.\n",
+                   old_net_name,
+                   old_module->getHierarchicalName());
+        continue;
+      }
+
+      new_net_name += block->getBaseName(old_net_name.c_str());
+      auto it = new_net_name_map.find(new_net_name);
+      if (it != new_net_name_map.end()) {
+        // Connect to an existing local net
+        dbNet* new_net = (*it).second;
+        new_iterm->connect(new_net);
+        debugPrint(logger,
+                   utl::ODB,
+                   "replace_design",
+                   1,
+                   "  connected iterm '{}' to existing local net '{}'",
+                   new_iterm->getName(),
+                   new_net->getName());
+      } else {
+        // Create and connect to a new local net
+        assert(block->findNet(new_net_name.c_str()) == nullptr);
+        dbNet* new_net
+            = dbNet::create(new_module->getOwner(), new_net_name.c_str());
+        new_iterm->connect(new_net);
+        debugPrint(logger,
+                   utl::ODB,
+                   "replace_design",
+                   1,
+                   "  Connected iterm '{}' to new local net '{}'",
+                   new_iterm->getName(),
+                   new_net->getName());
+
+        // Insert it to the map
+        new_net_name_map[new_net_name] = new_net;
       }
     }
   }

src/odb/src/db/dbNet.cpp

@@ -2599,4 +2599,21 @@ void dbNet::renameWithModNetInHighestHier()
   }
 }
 
+bool dbNet::isInternalTo(dbModule* module) const
+{
+  // If it's connected to any top-level ports (BTerms), it's not internal.
+  if (!getBTerms().empty()) {
+    return false;
+  }
+
+  // Check all instance terminals (ITerms) it's connected to.
+  for (dbITerm* iterm : getITerms()) {
+    if (iterm->getInst()->getModule() != module) {
+      return false;
+    }
+  }
+
+  return true;
+}
+
 }  // namespace odb

src/odb/test/BUILD

@@ -189,6 +189,7 @@ COMPULSORY_TESTS = [
     "replace_hier_mod3",
     "replace_hier_mod4",
     "replace_hier_mod5",
+    "replace_hier_mod6",
     "replace_hier_mod_report_checks",
     "rounding",
     "row_settings",
@@ -308,6 +309,7 @@ filegroup(
         "helper.tcl",
         "helpers.tcl",
         "replace_hier_mod1.v",
+        "replace_hier_mod6.v",
         "replace_hier_mod_undo.tcl",
         "replace_hier_mod_undo2.tcl",
         "sky130hd/sky130_fd_sc_hd__ff_n40C_1v95.lib",

src/odb/test/CMakeLists.txt

@@ -47,6 +47,7 @@ or_integration_tests(
     replace_hier_mod3
     replace_hier_mod4
     replace_hier_mod5
+    replace_hier_mod6
     replace_hier_mod_report_checks
     rounding
     row_settings

src/odb/test/replace_hier_mod1.ok

@@ -109,6 +109,8 @@ Corner: slow
            -0.409   slack (VIOLATED)
 
 
+Equivalence check - pre
+Repair timing output passed/skipped equivalence test
 ### swap bc1 to inv_chain ###
 [INFO GPL-0006] Execute incremental mode global placement.
 [INFO GPL-0002] DBU: 2000
@@ -260,6 +262,8 @@ Corner: slow
            -0.336   slack (VIOLATED)
 
 
+Equivalence check - swap (buffer_chain -> inv_chain)
+Repair timing output passed/skipped equivalence test
 ### swap bc1 back to buffer_chain ###
 [INFO GPL-0006] Execute incremental mode global placement.
 [INFO GPL-0002] DBU: 2000
@@ -388,6 +392,8 @@ Corner: slow
            -0.401   slack (VIOLATED)
 
 
+Equivalence check - swap for rollback (inv_chain -> buffer_chain)
+Repair timing output passed/skipped equivalence test
 ### swap bc1 back to inv_chain ###
 [INFO GPL-0006] Execute incremental mode global placement.
 [INFO GPL-0002] DBU: 2000
@@ -516,4 +522,5 @@ Corner: slow
            -0.330   slack (VIOLATED)
 
 
+Equivalence check - redo swap (buffer_chain -> inv_chain)
 Repair timing output passed/skipped equivalence test

src/odb/test/replace_hier_mod1.tcl

@@ -23,6 +23,9 @@ detailed_placement
 source Nangate45/Nangate45.rc
 set_wire_rc -layer metal3
 
+# For eqy, write a verilog before replace_hier_module
+write_verilog_for_eqy replace_hier_mod1 before "None"
+
 puts "### Initial bc1 is buffer_chain ###"
 report_cell_usage bc1
 report_net u1z -digits 3
@@ -32,6 +35,8 @@ report_checks -through r2/D -digits 3
 # Using "-through u1z" causes crash.
 # - it looks like "-through" creates a cache internally.
 #report_checks -through u1z -through r2/D -digits 3
+puts "Equivalence check - pre"
+run_equivalence_test replace_hier_mod1 ./Nangate45/work_around_yosys/ "None"
 
 puts "### swap bc1 to inv_chain ###"
 #set_debug_level ODB replace_design 1
@@ -44,6 +49,8 @@ report_net u3z -digits 3
 estimate_parasitics -placement
 report_checks -through r2/D -digits 3
 #report_checks -through u1z -through r2/D -digits 3
+puts "Equivalence check - swap (buffer_chain -> inv_chain)"
+run_equivalence_test replace_hier_mod1 ./Nangate45/work_around_yosys/ "None"
 
 puts "### swap bc1 back to buffer_chain ###"
 replace_hier_module bc1 buffer_chain
@@ -55,6 +62,8 @@ report_net u3z -digits 3
 estimate_parasitics -placement
 report_checks -through r2/D -digits 3
 #report_checks -through u1z -through r2/D -digits 3
+puts "Equivalence check - swap for rollback (inv_chain -> buffer_chain)"
+run_equivalence_test replace_hier_mod1 ./Nangate45/work_around_yosys/ "None"
 
 puts "### swap bc1 back to inv_chain ###"
 replace_hier_module bc1 inv_chain
@@ -66,5 +75,5 @@ report_net u3z -digits 3
 estimate_parasitics -placement
 report_checks -through r2/D -digits 3
 #report_checks -through u1z -through r2/D -digits 3
-
+puts "Equivalence check - redo swap (buffer_chain -> inv_chain)"
 run_equivalence_test replace_hier_mod1 ./Nangate45/work_around_yosys/ "None"