From 0b01e34ce1190adf4471f5da4b8c29d61a65a550 Mon Sep 17 00:00:00 2001 From: Matt Liberty Date: Tue, 29 Sep 2026 04:27:46 +0000 Subject: [PATCH 1/2] odb: resume the outer ECO journal after a nested commit/undo A nested beginEco suspended the enclosing journal on the ECO stack, but commitEco/undoEco of the inner ECO left no active journal. Edits made after the inner ECO ended were not recorded, so the outer undoEco did not revert them. The stack now records whether an entry was suspended by a nested beginEco (resumed after the inner ECO ends) or stopped by endEco (stays stopped). Fixes The-OpenROAD-Project-private/OpenROAD#3836 Signed-off-by: Matt Liberty --- src/odb/src/db/dbBlock.h | 11 ++++++- src/odb/src/db/dbDatabase.cpp | 31 +++++++++++++----- src/odb/test/cpp/TestJournal.cpp | 54 ++++++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 9 deletions(-) diff --git a/src/odb/src/db/dbBlock.h b/src/odb/src/db/dbBlock.h index 8c0eba09e05..65ef5e51290 100644 --- a/src/odb/src/db/dbBlock.h +++ b/src/odb/src/db/dbBlock.h @@ -328,8 +328,17 @@ class _dbBlock : public _dbObject std::list callbacks_; void* extmi_; + struct JournalStackEntry + { + dbJournal* journal; + // True when a nested beginEco suspended this journal; it resumes + // recording once the nested ECO is committed or undone. False when + // endEco stopped it. + bool resume; + }; + dbJournal* journal_; - std::stack journal_stack_; + std::stack journal_stack_; }; dbOStream& operator<<(dbOStream& stream, const _dbBlock& block); diff --git a/src/odb/src/db/dbDatabase.cpp b/src/odb/src/db/dbDatabase.cpp index 32dd8d48fb8..897fc6203bc 100644 --- a/src/odb/src/db/dbDatabase.cpp +++ b/src/odb/src/db/dbDatabase.cpp @@ -940,11 +940,24 @@ void dbDatabase::write(std::ostream& file) file.flush(); } +// Make the enclosing ECO the active journal again if a nested beginEco +// suspended it, so later edits are recorded and can be undone with it. +static void resumeSuspendedEco(_dbBlock* block) +{ + if (block->journal_stack_.empty() || !block->journal_stack_.top().resume) { + return; + } + block->journal_ = block->journal_stack_.top().journal; + block->journal_stack_.pop(); +} + void dbDatabase::beginEco(dbBlock* block_) { _dbBlock* block = (_dbBlock*) block_; if (block->journal_) { - endEco(block_); + // Suspend the enclosing ECO; it resumes when this one is committed + // or undone. + block->journal_stack_.push({block->journal_, true}); } block->journal_ = new dbJournal(block_); assert(block->journal_); @@ -960,7 +973,7 @@ void dbDatabase::endEco(dbBlock* block_) { _dbBlock* block = (_dbBlock*) block_; assert(block->journal_); - block->journal_stack_.push(block->journal_); + block->journal_stack_.push({block->journal_, false}); block->journal_ = nullptr; debugPrint(block_->getImpl()->getLogger(), utl::ODB, @@ -968,7 +981,7 @@ void dbDatabase::endEco(dbBlock* block_) 2, "ECO: Ended ECO #{} (size {}) and pushed to ECO stack", block->journal_stack_.size() - 1, - block->journal_stack_.top()->size()); + block->journal_stack_.top().journal->size()); } void dbDatabase::commitEco(dbBlock* block_) @@ -977,11 +990,11 @@ void dbDatabase::commitEco(dbBlock* block_) // Commit the current ECO or the last ECO into stack assert(block->journal_ || !block->journal_stack_.empty()); if (!block->journal_) { - block->journal_ = block->journal_stack_.top(); + block->journal_ = block->journal_stack_.top().journal; block->journal_stack_.pop(); } if (!block->journal_stack_.empty()) { - dbJournal* prev_journal = block->journal_stack_.top(); + dbJournal* prev_journal = block->journal_stack_.top().journal; int old_size = prev_journal->size(); prev_journal->append(block->journal_); debugPrint(block_->getImpl()->getLogger(), @@ -993,7 +1006,7 @@ void dbDatabase::commitEco(dbBlock* block_) block->journal_->size(), block->journal_stack_.size() - 1, old_size, - block->journal_stack_.top()->size()); + prev_journal->size()); } else { debugPrint(block_->getImpl()->getLogger(), utl::ODB, @@ -1005,6 +1018,7 @@ void dbDatabase::commitEco(dbBlock* block_) } delete block->journal_; block->journal_ = nullptr; + resumeSuspendedEco(block); } void dbDatabase::undoEco(dbBlock* block_) @@ -1012,7 +1026,7 @@ void dbDatabase::undoEco(dbBlock* block_) _dbBlock* block = (_dbBlock*) block_; assert(block->journal_ || !block->journal_stack_.empty()); if (!block->journal_) { - block->journal_ = block->journal_stack_.top(); + block->journal_ = block->journal_stack_.top().journal; block->journal_stack_.pop(); } debugPrint(block_->getImpl()->getLogger(), @@ -1026,6 +1040,7 @@ void dbDatabase::undoEco(dbBlock* block_) block->journal_ = nullptr; journal->undo(); delete journal; + resumeSuspendedEco(block); } bool dbDatabase::ecoEmpty(dbBlock* block_) @@ -1084,7 +1099,7 @@ void dbDatabase::writeEco(dbBlock* block_, const char* filename) stream.flush(); } else if (!block->journal_stack_.empty()) { dbOStream stream(block->getDatabase(), file); - stream << *block->journal_stack_.top(); + stream << *block->journal_stack_.top().journal; stream.flush(); } } diff --git a/src/odb/test/cpp/TestJournal.cpp b/src/odb/test/cpp/TestJournal.cpp index d7c3bd1cca9..890a4956892 100644 --- a/src/odb/test/cpp/TestJournal.cpp +++ b/src/odb/test/cpp/TestJournal.cpp @@ -286,5 +286,59 @@ TEST_F(JournalFixture, RenameModNet) dbModule::destroy(module); } +TEST_F(JournalFixture, NestedUndoThenEditIsUndoneByOuter) +{ + dbInst::create(block, and2, "a"); + + dbDatabase::beginEco(block); // outer + dbDatabase::beginEco(block); // inner + dbInst::destroy(block->findInst("a")); + dbDatabase::undoEco(block); // inner undo recreates "a" + ASSERT_NE(block->findInst("a"), nullptr); + + // The outer ECO must record edits made after the inner one ends. + dbInst::destroy(block->findInst("a")); + dbInst::create(block, or2, "b"); + EXPECT_FALSE(dbDatabase::ecoEmpty(block)); + dbDatabase::undoEco(block); // outer undo + + EXPECT_NE(block->findInst("a"), nullptr); + EXPECT_EQ(block->findInst("b"), nullptr); + EXPECT_TRUE(dbDatabase::ecoStackEmpty(block)); +} + +TEST_F(JournalFixture, NestedCommitThenEditIsUndoneByOuter) +{ + dbDatabase::beginEco(block); // outer + dbDatabase::beginEco(block); // inner + dbInst::create(block, and2, "a"); + dbDatabase::commitEco(block); // inner merges into outer + + dbInst::create(block, or2, "b"); + dbDatabase::undoEco(block); // outer undo + + EXPECT_EQ(block->findInst("a"), nullptr); + EXPECT_EQ(block->findInst("b"), nullptr); + EXPECT_TRUE(dbDatabase::ecoStackEmpty(block)); +} + +TEST_F(JournalFixture, EndedEcoStaysStoppedAfterNestedCommit) +{ + dbDatabase::beginEco(block); + dbDatabase::endEco(block); // stop recording the first ECO + dbDatabase::beginEco(block); + dbInst::create(block, and2, "a"); + dbDatabase::commitEco(block); // merges into the stopped ECO + + // The stopped ECO does not resume recording. + EXPECT_FALSE(dbDatabase::ecoStackEmpty(block)); + dbInst::create(block, or2, "b"); + dbDatabase::undoEco(block); + + EXPECT_EQ(block->findInst("a"), nullptr); + EXPECT_NE(block->findInst("b"), nullptr); + EXPECT_TRUE(dbDatabase::ecoStackEmpty(block)); +} + } // namespace } // namespace odb From 6dff73b0476e8d5385954d37d6ce7cec36e1dd71 Mon Sep 17 00:00:00 2001 From: Matt Liberty Date: Tue, 29 Sep 2026 17:26:49 +0000 Subject: [PATCH 2/2] odb: assert ECO stack invariants when resuming a suspended journal Signed-off-by: Matt Liberty --- src/odb/src/db/dbDatabase.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/odb/src/db/dbDatabase.cpp b/src/odb/src/db/dbDatabase.cpp index 897fc6203bc..c03fceb2ed0 100644 --- a/src/odb/src/db/dbDatabase.cpp +++ b/src/odb/src/db/dbDatabase.cpp @@ -947,6 +947,8 @@ static void resumeSuspendedEco(_dbBlock* block) if (block->journal_stack_.empty() || !block->journal_stack_.top().resume) { return; } + assert(block->journal_ == nullptr); + assert(block->journal_stack_.top().journal != nullptr); block->journal_ = block->journal_stack_.top().journal; block->journal_stack_.pop(); } @@ -958,6 +960,7 @@ void dbDatabase::beginEco(dbBlock* block_) // Suspend the enclosing ECO; it resumes when this one is committed // or undone. block->journal_stack_.push({block->journal_, true}); + block->journal_ = nullptr; } block->journal_ = new dbJournal(block_); assert(block->journal_);