diff --git a/src/dbSta/include/db_sta/dbNetwork.hh b/src/dbSta/include/db_sta/dbNetwork.hh index 93a0774c3b4..36a5e2997e7 100644 --- a/src/dbSta/include/db_sta/dbNetwork.hh +++ b/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; diff --git a/src/dbSta/src/dbNetwork.cc b/src/dbSta/src/dbNetwork.cc index d07b4e14549..b5e593b1377 100644 --- a/src/dbSta/src/dbNetwork.cc +++ b/src/dbSta/src/dbNetwork.cc @@ -752,8 +752,9 @@ ObjectId dbNetwork::id(const Port* port) const const dbObject* obj = reinterpret_cast(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(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(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(cell); - return ccell->id(); + return (ccell->id() << DBIDTAG_WIDTH) | CONCRETE_OBJECT_ID; } //////////////////////////////////////////////////////////////// diff --git a/src/dbSta/test/BUILD b/src/dbSta/test/BUILD index af672f2959e..1dd7a9ae414 100644 --- a/src/dbSta/test/BUILD +++ b/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", + ], +) diff --git a/src/dbSta/test/cpp/CMakeLists.txt b/src/dbSta/test/cpp/CMakeLists.txt index 4561c062121..13a139377da 100644 --- a/src/dbSta/test/cpp/CMakeLists.txt +++ b/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 +) diff --git a/src/dbSta/test/cpp/TestWriteVerilog.cpp b/src/dbSta/test/cpp/TestWriteVerilog.cpp new file mode 100644 index 00000000000..17dbe34a969 --- /dev/null +++ b/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 +#include +#include +#include + +#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(ifs)), + std::istreambuf_iterator()); + 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 diff --git a/src/dbSta/test/cpp/TestWriteVerilog_RemoveCellsIdCollision_pre.v b/src/dbSta/test/cpp/TestWriteVerilog_RemoveCellsIdCollision_pre.v new file mode 100644 index 00000000000..a9182cedaab --- /dev/null +++ b/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