feat(dbSta): store timing constraints in the .odb - #11260
Conversation
.odb already subsumes the netlist, tech, placement, routing and even dont_touch. Timing constraints are the conspicuous omission, so a flow has to carry a matching .sdc next to every .odb and then work out which .sdc goes with which .odb. That inference is a guess, and the only new freedom it buys anyone is the freedom to load the wrong constraints. write_db now stores the constraints in the block, and read_db -sdc replays them, so an .odb can describe itself. The payload is a dbStringProperty, opaque to odb, so no odb -> sta dependency is created and an older binary still reads the file. Replay is opt-in, so read_db behaves exactly as before unless asked otherwise. write_sdc is untouched and remains the way to export constraints for inspection or for a standalone OpenSTA bug report. Verified on the largest asap7 designs: write_db, then read_db -sdc, then write_sdc reproduces the original constraints byte for byte (swerv_wrapper, 89615 lines; mock-cpu, 94253 lines). Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
There was a problem hiding this comment.
Code Review
This pull request enables storing and restoring timing constraints (SDC) directly within the OpenDB (.odb) database file, making it self-describing and removing the need for a separate .sdc file. Feedback on the changes suggests using the secure utl::ScopedTemporaryFile utility instead of a custom temporary file class to prevent insecure temporary file creation, and ensuring that SDC constraints are only restored if the database is successfully loaded without throwing an exception.
| #include <sstream> | ||
| #include <string> | ||
| #include <string_view> |
| namespace { | ||
|
|
||
| // sta::writeSdc only writes to a file. Round-trip through a temporary one | ||
| // until an ostream overload exists upstream in OpenSTA. | ||
| class TempSdcFile | ||
| { | ||
| public: | ||
| TempSdcFile() | ||
| { | ||
| path_ = std::filesystem::temp_directory_path() | ||
| / ("openroad-sdc-" + std::to_string(::getpid()) + "-" | ||
| + std::to_string( | ||
| reinterpret_cast<uintptr_t>(static_cast<void*>(this))) | ||
| + ".sdc"); | ||
| } | ||
| ~TempSdcFile() | ||
| { | ||
| std::error_code ec; | ||
| std::filesystem::remove(path_, ec); | ||
| } | ||
| TempSdcFile(const TempSdcFile&) = delete; | ||
| TempSdcFile& operator=(const TempSdcFile&) = delete; | ||
|
|
||
| const std::filesystem::path& path() const { return path_; } | ||
|
|
||
| std::string read() const | ||
| { | ||
| std::ifstream in(path_, std::ios::binary); | ||
| if (!in) { | ||
| return {}; | ||
| } | ||
| std::ostringstream buf; | ||
| buf << in.rdbuf(); | ||
| return buf.str(); | ||
| } | ||
|
|
||
| private: | ||
| std::filesystem::path path_; | ||
| }; | ||
|
|
||
| } // namespace | ||
|
|
||
| void dbSta::saveSdcToDb() | ||
| { | ||
| odb::dbChip* chip = db_->getChip(); | ||
| if (chip == nullptr) { | ||
| return; | ||
| } | ||
| odb::dbBlock* block = chip->getBlock(); | ||
| if (block == nullptr) { | ||
| return; | ||
| } | ||
| // A pure-odb flow (no liberty, no linked network) has no constraints to | ||
| // save. Skip quietly: write_db must behave exactly as it did before for | ||
| // the flows that never had constraints in the first place. | ||
| if (!network_->isLinked() || network_->defaultLibertyLibrary() == nullptr) { | ||
| return; | ||
| } | ||
| const Sdc* sdc = cmdSdc(); | ||
| if (sdc == nullptr) { | ||
| return; | ||
| } | ||
|
|
||
| std::string text; | ||
| try { | ||
| TempSdcFile temp; | ||
| writeSdc(sdc, | ||
| temp.path().string(), | ||
| /* leaf */ false, | ||
| /* native */ true, | ||
| /* digits */ 4, | ||
| /* gzip */ false, | ||
| /* no_timestamp */ true); | ||
| text = temp.read(); | ||
| } catch (const std::exception& e) { |
There was a problem hiding this comment.
Creating temporary files with predictable names in shared directories (like /tmp) poses security risks (CWE-377: Insecure Temporary File) and race conditions. Instead of rolling a custom TempSdcFile class, use the existing, secure utl::ScopedTemporaryFile utility which handles secure file creation and automatic cleanup.
namespace {
// No custom TempSdcFile needed as we use utl::ScopedTemporaryFile.
} // namespace
void dbSta::saveSdcToDb()
{
odb::dbChip* chip = db_->getChip();
if (chip == nullptr) {
return;
}
odb::dbBlock* block = chip->getBlock();
if (block == nullptr) {
return;
}
// A pure-odb flow (no liberty, no linked network) has no constraints to
// save. Skip quietly: write_db must behave exactly as it did before for
// the flows that never had constraints in the first place.
if (!network_->isLinked() || network_->defaultLibertyLibrary() == nullptr) {
return;
}
const Sdc* sdc = cmdSdc();
if (sdc == nullptr) {
return;
}
std::string text;
try {
utl::ScopedTemporaryFile temp;
writeSdc(sdc,
temp.path(),
/* leaf */ false,
/* native */ true,
/* digits */ 4,
/* gzip */ false,
/* no_timestamp */ true);
std::ifstream in(temp.path(), std::ios::binary);
if (in) {
std::ostringstream buf;
buf << in.rdbuf();
text = buf.str();
}
} catch (const std::exception& e) {| try { | ||
| utl::InStreamHandler handler(filename, true); | ||
| readDb(handler.getStream()); | ||
| } catch (const std::ios_base::failure& f) { | ||
| logger_->error(ORD, 54, "odb file {} is invalid: {}", filename, f.what()); | ||
| } | ||
| if (restore_sdc) { | ||
| restoreSdcFromDb(); | ||
| } |
There was a problem hiding this comment.
If reading the database stream fails and throws an exception, the database state might be invalid or incomplete. Replaying SDC constraints on an invalid database should be avoided. Only call restoreSdcFromDb() if the database was successfully loaded.
bool success = true;
try {
utl::InStreamHandler handler(filename, true);
readDb(handler.getStream());
} catch (const std::ios_base::failure& f) {
logger_->error(ORD, 54, "odb file {} is invalid: {}", filename, f.what());
success = false;
}
if (success && restore_sdc) {
restoreSdcFromDb();
}|
out of sight, out of mind for now |
Draft / proposal. Single concern: make an
.odbcarry its own timing constraints.Why
.odbalready subsumes the netlist, the tech, placement, routing, the GCell grid, PDN, scan chains, power domains, global connect rules — anddont_touch(dbInst::setDoNotTouch). Timing constraints are the conspicuous omission.So a flow carries a matching
.sdcnext to every.odb, and then has to work out which.sdcgoes with which.odb. In ORFS that is a glob ofresults/, a dictionary sort, and "pick the greatest.sdcat or below this.odb's stem". It has picked wrong before. The only new freedom the split buys anyone is the freedom to load the wrong constraints.This is probably the POLA
.odbthinking all along — nobody got around to it, and we keep stubbing our toes on it..sdcis not special. It is a human-authored input, consumed at one point in a flow, exactly like the LEF and the netlist that.odbalready absorbs.Small human-written
.sdcfiles are a feature worth protecting. This change does not touch them, and does not touchwrite_sdc: constraints still export to text for inspection and for standalone OpenSTA bug reports. What changes is that a flow no longer has to route constraints between its own stages through a text format.What this does
write_dbstores the constraints in the block (adbStringPropertynamedsta.sdc, the output ofwrite_sdc -no_timestamp).read_db -sdcreplays them. Without the flagread_dbbehaves exactly as it does today — no behavior change for any existing script.write_sdcis untouched.Chesterton's Fence
odbmust not depend on OpenSTAdbSta, on the existingdbStringPropertypersistence. No new dependency edge..odbread_dbsemantics would surprise peoplewrite_sdcfidelity — a user's.sdcis Tcl andSdcmodels less than the file sayswrite_sdcoutput. This adds no new exposure, and the tests below check the round trip byte for byte.write_dbmust not break on designs with no constraints.odbas before.Correctness
write_db→read_db -sdc→write_sdcreproduces the original constraints byte for byte on the largest public asap7 designs — swerv_wrapper (89,615 lines of constraints) and mock-cpu (94,253 lines).Three regression tests in
src/dbSta/test: constraints are stored bywrite_dband matchwrite_sdc;read_db -sdcbrings the design back constrained with no.sdcin sight; plainread_dbstill loads no constraints. Full//src/dbSta/...suite passes (2549 tests).Performance
The second toe-stub, and the one that is paid continuously — at every stage of every run, and again every time somebody opens a large design in the GUI.
A human writes a handful of lines.
write_sdcexpands them into one explicitget_cells/get_portsper object, and the next stage re-litigates every one of those name lookups against the netlist..sdc.sdcLoading
results/.../3_place.odbplus its.sdc, median of 3, same machine, ASAP7 CI liberty set:read_dbread_sdcread_db -sdc)The 12–14% is real but it is not the point. The point is the
read_sdccolumn: the constraints cost 4.2× (swerv_wrapper) and 15.1× (mock-cpu) what it costs to load the entire database. This PR stores the constraints as text and replays them through the same parser, so it only recovers the file andsourceoverhead. The rest of that column is what a long-hand binary form — object references stored as odb ids, no Tcl, no name matching — is worth, and this PR is the place to put it.cva6 is in the table as the honest control: its constraints do not expand, so there is nothing to win, and nothing is lost.
Reproducing on your own design
Nothing in the harness needs a public design. Given any
.odband its.sdc:The numbers that matter are timings and line counts, so results can be reported without disclosing anything about the design.
Deliberately not in this PR
set_dont_use. Same dichotomy — aLibertyCellattribute thatwrite_sdcnever emits, so flows re-apply it from the environment at every stage, or forget to. Its own concern..odbconstraint-complete, not run-complete.Testing