dbSta: fix ObjectId collision between ConcreteCell and dbModule

src/dbSta/include/db_sta/dbNetwork.hh

@@ -470,6 +470,7 @@ class dbNetwork : public ConcreteNetwork
   static constexpr unsigned DBMODINST_ID = 0x6;
   static constexpr unsigned DBMODNET_ID = 0x7;
   static constexpr unsigned DBMODULE_ID = 0x8;
+  static constexpr unsigned CONCRETE_OBJECT_ID = 0xF;
   // Number of lower bits used
   static constexpr unsigned DBIDTAG_WIDTH = 0x4;
 

src/dbSta/src/dbNetwork.cc

@@ -752,8 +752,9 @@ ObjectId dbNetwork::id(const Port* port) const
     const dbObject* obj = reinterpret_cast<const dbObject*>(port);
     return getDbNwkObjectId(obj);
   }
-  // default
-  return ConcreteNetwork::id(port);
+
+  // Tag ID to avoid ID collision b/w dbObject and STA object
+  return (ConcreteNetwork::id(port) << DBIDTAG_WIDTH) | CONCRETE_OBJECT_ID;
 }
 
 // Note:
@@ -766,16 +767,16 @@ ObjectId dbNetwork::id(const Port* port) const
 
 ObjectId dbNetwork::id(const Cell* cell) const
 {
-  // in hierarchical flow we use the object id for the index
-  if (hasHierarchy()) {
-    if (!isConcreteCell(cell)) {
-      const dbObject* obj = reinterpret_cast<const dbObject*>(cell);
-      return getDbNwkObjectId(obj);
-    }
+  // top_cell_ is a ConcreteCell but isConcreteCell() returns false for it;
+  // guard against reinterpret_cast to dbObject which would crash.
+  if (hasHierarchy() && !isConcreteCell(cell) && cell != top_cell_) {
+    const dbObject* obj = reinterpret_cast<const dbObject*>(cell);
+    return getDbNwkObjectId(obj);
   }
-  // default behaviour use the concrete cell.
+
+  // Tag ID to avoid ID collision b/w dbObject and STA object
   const ConcreteCell* ccell = reinterpret_cast<const ConcreteCell*>(cell);
-  return ccell->id();
+  return (ccell->id() << DBIDTAG_WIDTH) | CONCRETE_OBJECT_ID;
 }
 
 ////////////////////////////////////////////////////////////////

src/dbSta/test/BUILD

@@ -318,3 +318,20 @@ cc_test(
         "@googletest//:gtest_main",
     ],
 )
+
+cc_test(
+    name = "test_write_verilog",
+    srcs = ["cpp/TestWriteVerilog.cpp"],
+    data = glob(["cpp/TestWriteVerilog*.v"]),
+    deps = [
+        "//src/dbSta",
+        "//src/dbSta:dbNetwork",
+        "//src/dbSta:dbReadVerilog",
+        "//src/odb",
+        "//src/sta:opensta_lib",
+        "//src/tst:integrated_fixture",
+        "//src/utl",
+        "@googletest//:gtest",
+        "@googletest//:gtest_main",
+    ],
+)

src/dbSta/test/cpp/CMakeLists.txt

@@ -82,3 +82,32 @@ gtest_discover_tests(TestReadVerilog
 
 add_dependencies(build_and_test TestReadVerilog
 )
+
+add_executable(TestWriteVerilog TestWriteVerilog.cpp)
+target_link_libraries(TestWriteVerilog
+        OpenSTA
+        GTest::gtest
+        GTest::gtest_main
+        GTest::gmock
+        dbSta_lib
+        utl_lib
+        rsz_lib
+        grt_lib
+        dpl_lib
+        stt_lib
+        odb
+        tst
+        ${TCL_LIBRARY}
+)
+
+target_include_directories(TestWriteVerilog
+    PRIVATE
+      ${PROJECT_SOURCE_DIR}/src/dbSta/src
+)
+
+gtest_discover_tests(TestWriteVerilog
+    WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/..
+)
+
+add_dependencies(build_and_test TestWriteVerilog
+)

src/dbSta/test/cpp/TestWriteVerilog.cpp

@@ -0,0 +1,142 @@
+// SPDX-License-Identifier: BSD-3-Clause
+// Copyright (c) 2025, The OpenROAD Authors
+
+// Regression test: write_verilog -remove_cells must not drop hierarchical
+// module instances.
+//
+// Background:
+//   CellSet uses CellIdLess which compares cells by network_->id(cell).
+//   Before the fix, ConcreteCell used raw sequential id_ (0,1,2,...) while
+//   dbModule used (db_id << 4) | 0x8 (24,40,56,...).  These ID spaces
+//   overlapped, so CellSet::contains could treat a dbModule as equal to a
+//   ConcreteCell, causing writeChild to silently drop hierarchical instances.
+//
+//   The fix tags ConcreteCell IDs with CONCRETE_OBJECT_ID (0xF) so they
+//   never collide with dbModule IDs (tag 0x8).
+
+#include <cstdio>
+#include <fstream>
+#include <iterator>
+#include <string>
+
+#include "gtest/gtest.h"
+#include "sta/Network.hh"
+#include "sta/NetworkClass.hh"
+#include "sta/PatternMatch.hh"
+#include "sta/VerilogWriter.hh"
+#include "tst/IntegratedFixture.h"
+
+namespace sta {
+
+class TestWriteVerilog : public tst::IntegratedFixture
+{
+ public:
+  TestWriteVerilog()
+      : tst::IntegratedFixture(tst::IntegratedFixture::Technology::Nangate45,
+                               "_main/src/dbSta/test/")
+  {
+  }
+};
+
+// Verifies that write_verilog -remove_cells does not drop hierarchical
+// instances when a liberty cell is removed.
+//
+// Puts a liberty cell (INV_X1) in remove_cells, then verifies that the
+// hierarchical instance (sub_inst) is still present in the output.
+// Before the fix, this could fail when the ConcreteCell's raw id_
+// collided with the dbModule's computed id.
+TEST_F(TestWriteVerilog, RemoveCellsIdCollision)
+{
+  const std::string test_name = "TestWriteVerilog_RemoveCellsIdCollision";
+
+  // Read a simple hierarchical design: top -> sub_mod -> INV_X1
+  readVerilogAndSetup(test_name + "_pre.v", /*init_default_sdc=*/false);
+
+  Network* network = sta_->network();
+  Instance* top_inst = network->topInstance();
+
+  // Find the hierarchical instance and its Cell* (which is a dbModule*)
+  Cell* hier_cell = nullptr;
+  std::string hier_inst_name;
+  InstanceChildIterator* child_iter = network->childIterator(top_inst);
+  while (child_iter->hasNext()) {
+    Instance* child = child_iter->next();
+    if (network->isHierarchical(child)) {
+      hier_cell = network->cell(child);
+      hier_inst_name = network->name(child);
+      break;
+    }
+  }
+  delete child_iter;
+  ASSERT_NE(hier_cell, nullptr) << "No hierarchical instance found in design";
+
+  ObjectId hier_id = network->id(hier_cell);
+
+  // Verify no ConcreteCell shares the same id as the hier module.
+  // After the fix, ConcreteCell IDs use tag 0xF while dbModule IDs
+  // use tag 0x8, so no collision is possible.
+  PatternMatch match_all("*",
+                         /*is_regexp=*/false,
+                         /*nocase=*/false,
+                         sta_->tclInterp());
+
+  LibraryIterator* lib_iter = network->libraryIterator();
+  while (lib_iter->hasNext()) {
+    Library* lib = lib_iter->next();
+    CellSeq lib_cells = network->findCellsMatching(lib, &match_all);
+    for (Cell* cell : lib_cells) {
+      if (cell != hier_cell) {
+        EXPECT_NE(network->id(cell), hier_id)
+            << "ObjectId collision between ConcreteCell '"
+            << network->name(cell) << "' and dbModule '"
+            << network->name(hier_cell) << "' — tagging should prevent this";
+      }
+    }
+  }
+  delete lib_iter;
+
+  // Find INV_X1 liberty cell to use in remove_cells
+  Cell* inv_cell = nullptr;
+  LibraryIterator* lib_iter2 = network->libraryIterator();
+  while (lib_iter2->hasNext()) {
+    Library* lib = lib_iter2->next();
+    PatternMatch inv_match("INV_X1",
+                           /*is_regexp=*/false,
+                           /*nocase=*/false,
+                           sta_->tclInterp());
+    CellSeq cells = network->findCellsMatching(lib, &inv_match);
+    if (cells.empty() == false) {
+      inv_cell = cells[0];
+      break;
+    }
+  }
+  delete lib_iter2;
+  ASSERT_NE(inv_cell, nullptr) << "INV_X1 not found in libraries";
+
+  // Put INV_X1 in remove_cells and write verilog
+  CellSeq remove_cells;
+  remove_cells.push_back(inv_cell);
+
+  const std::string output_file = test_name + "_out.v";
+  writeVerilog(output_file.c_str(), false, &remove_cells, network);
+
+  // Read the output
+  std::ifstream ifs(output_file);
+  ASSERT_TRUE(ifs.is_open()) << "Failed to open output file: " << output_file;
+  std::string content((std::istreambuf_iterator<char>(ifs)),
+                      std::istreambuf_iterator<char>());
+  ifs.close();
+
+  // The hierarchical instance must be present in the output.
+  // Only leaf INV_X1 instances should be removed, not hierarchical modules.
+  EXPECT_NE(content.find(hier_inst_name), std::string::npos)
+      << "Hierarchical instance '" << hier_inst_name
+      << "' was incorrectly removed from write_verilog output.\n"
+      << "  write_verilog -remove_cells should never drop hierarchical "
+         "instances.";
+
+  // Clean up
+  std::remove(output_file.c_str());
+}
+
+}  // namespace sta

src/dbSta/test/cpp/TestWriteVerilog_RemoveCellsIdCollision_pre.v

@@ -0,0 +1,22 @@
+/*
+ Bug reproduction for write_verilog -remove_cells.
+
+ This hierarchical design contains a sub-module "sub_mod" instantiated
+ in "top". When a liberty cell whose ConcreteCell ID collides with the
+ dbModule computed ID is placed in remove_cells, the CellIdLess
+ comparator in CellSet treats them as equal, causing writeChild to
+ incorrectly drop the hierarchical instance.
+ */
+module top(in, out);
+  input in;
+  output out;
+  wire w1;
+  sub_mod sub_inst (.A(in), .Z(w1));
+  INV_X1 u1 (.A(w1), .ZN(out));
+endmodule
+
+module sub_mod(A, Z);
+  input A;
+  output Z;
+  INV_X1 inv1 (.A(A), .ZN(Z));
+endmodule