From 81872d41f0a6f577c1401dd0ae896392a2c662a3 Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Sun, 26 May 2024 01:03:24 +0200 Subject: [PATCH] Fixed a problem triggered by the new foreign layout handling Problem was that with forward-referenced text strings in OASIS, StringRef objects were created (string placeholders). Those where not properly migrated when transferring the foreign layouts into DEF space, causing a memory corruption issue. This solution is to provide a central, singleton string repository. This needs to be MT safe (reference counting) which should also fix potential problems when accessing StringRef-based Text objects from multiple threads. --- src/db/db/dbLayout.cc | 1 - src/db/db/dbLayout.h | 17 -- src/db/db/dbText.cc | 72 +++++++- src/db/db/dbText.h | 155 +++--------------- src/db/unit_tests/dbLayoutTests.cc | 37 +++++ src/db/unit_tests/dbTextTests.cc | 22 +-- .../oasis/db_plugin/dbOASISReader.cc | 4 +- 7 files changed, 145 insertions(+), 163 deletions(-) diff --git a/src/db/db/dbLayout.cc b/src/db/db/dbLayout.cc index 319f10bae..bcd76d706 100644 --- a/src/db/db/dbLayout.cc +++ b/src/db/db/dbLayout.cc @@ -816,7 +816,6 @@ Layout::mem_stat (MemStatistics *stat, MemStatistics::purpose_t purpose, int cat db::mem_stat (stat, purpose, cat, m_pcell_ids, true, (void *) this); db::mem_stat (stat, purpose, cat, m_lib_proxy_map, true, (void *) this); db::mem_stat (stat, purpose, cat, m_meta_info, true, (void *) this); - db::mem_stat (stat, purpose, cat, m_string_repository, true, (void *) this); db::mem_stat (stat, purpose, cat, m_shape_repository, true, (void *) this); db::mem_stat (stat, purpose, cat, m_properties_repository, true, (void *) this); db::mem_stat (stat, purpose, cat, m_array_repository, true, (void *) this); diff --git a/src/db/db/dbLayout.h b/src/db/db/dbLayout.h index 4ab44a12e..f5b28cd0f 100644 --- a/src/db/db/dbLayout.h +++ b/src/db/db/dbLayout.h @@ -588,22 +588,6 @@ public: return m_array_repository; } - /** - * @brief Accessor to the string repository - */ - StringRepository &string_repository () - { - return m_string_repository; - } - - /** - * @brief Accessor to the string repository (const version) - */ - const StringRepository &string_repository () const - { - return m_string_repository; - } - /** * @brief Accessor to the shape repository */ @@ -2170,7 +2154,6 @@ private: cell_map_type m_cell_map; double m_dbu; db::properties_id_type m_prop_id; - StringRepository m_string_repository; GenericRepository m_shape_repository; PropertiesRepository m_properties_repository; ArrayRepository m_array_repository; diff --git a/src/db/db/dbText.cc b/src/db/db/dbText.cc index 4bd2bbf7d..04af70434 100644 --- a/src/db/db/dbText.cc +++ b/src/db/db/dbText.cc @@ -22,10 +22,12 @@ #include "dbText.h" +#include "tlThreads.h" namespace db { + static char halign2code (db::HAlign ha) { if (ha == db::HAlignCenter) { @@ -78,13 +80,79 @@ static db::VAlign extract_valign (tl::Extractor &ex) } } +// ---------------------------------------------------------------------------- +// StringRepository implementation + +static StringRepository s_repository; +static StringRepository *sp_repository = 0; +static tl::Mutex s_repository_lock; + +StringRepository * +StringRepository::instance () +{ + return sp_repository; +} + +StringRepository::StringRepository () +{ + sp_repository = this; +} + +StringRepository::~StringRepository () +{ + if (sp_repository == this) { + sp_repository = 0; + } + + for (std::set::const_iterator s = m_string_refs.begin (); s != m_string_refs.end (); ++s) { + delete *s; + } +} + +const StringRef * +StringRepository::create_string_ref () +{ + tl::MutexLocker locker (&s_repository_lock); + StringRef *ref = new StringRef (); + m_string_refs.insert (ref); + return ref; +} + +void +StringRepository::unregister_ref (StringRef *ref) +{ + tl::MutexLocker locker (&s_repository_lock); + if (! m_string_refs.empty ()) { + m_string_refs.erase (ref); + } +} + // ---------------------------------------------------------------------------- // StringRef implementation +static tl::Mutex s_ref_lock; + StringRef::~StringRef () { - if (mp_rep) { - mp_rep->unregister_ref (this); + if (StringRepository::instance ()) { + StringRepository::instance ()->unregister_ref (this); + } +} + +void +StringRef::add_ref () +{ + tl::MutexLocker locker (&s_ref_lock); + ++m_ref_count; +} + +void +StringRef::remove_ref () +{ + tl::MutexLocker locker (&s_ref_lock); + --m_ref_count; + if (m_ref_count == 0) { + delete this; } } diff --git a/src/db/db/dbText.h b/src/db/db/dbText.h index 6ad6a43de..f018fd026 100644 --- a/src/db/db/dbText.h +++ b/src/db/db/dbText.h @@ -61,21 +61,12 @@ public: /** * @brief Increment the reference counter */ - void add_ref () - { - ++m_ref_count; - } + void add_ref (); /** * @brief Decrement the reference counter and remove the object when it reaches 0 */ - void remove_ref () - { - --m_ref_count; - if (m_ref_count == 0) { - delete this; - } - } + void remove_ref (); /** * @brief Assignment of a std::string object @@ -102,14 +93,6 @@ public: return m_value; } - /** - * @brief Access to the repository the strings are in - */ - const StringRepository *rep () const - { - return mp_rep; - } - void mem_stat (MemStatistics *stat, MemStatistics::purpose_t purpose, int cat, bool no_self = false, void *parent = 0) const { if (! no_self) { @@ -120,15 +103,15 @@ public: private: friend class StringRepository; - StringRepository *mp_rep; + std::string m_value; size_t m_ref_count; /** * @brief Hidden constructor attaching the reference to a repository */ - StringRef (StringRepository *rep) - : mp_rep (rep), m_ref_count (0) + StringRef () + : m_ref_count (0) { // .. nothing yet .. } @@ -165,22 +148,12 @@ public: /** * @brief Constructor */ - StringRepository () - { - // .. nothing yet .. - } + StringRepository (); /** * @brief Destructor */ - ~StringRepository () - { - std::set st; - m_string_refs.swap (st); - for (std::set::const_iterator s = st.begin (); s != st.end (); ++s) { - delete *s; - } - } + ~StringRepository (); /** * @brief Create a string reference object. @@ -196,77 +169,37 @@ public: * A string reference's text can be set by using the change_string_ref * method. */ - const StringRef *create_string_ref () - { - StringRef *ref = new StringRef (this); - m_string_refs.insert (ref); - return ref; - } + const StringRef *create_string_ref (); /** * @brief Change the string associated with a StringRef */ - void change_string_ref (const StringRef *ref, const std::string &s) + static void change_string_ref (const StringRef *ref, const std::string &s) { *(const_cast (ref)) = s; } /** - * @brief For debugging purposes: get the number of entries + * @brief The singleton instance of the string repository */ - size_t size () const + static StringRepository *instance (); + + /** + * @brief For testing purposes: size of the repository + */ + size_t size () { return m_string_refs.size (); } - /** - * @brief Iterates over the string refs (begin) - */ - iterator begin () const - { - return m_string_refs.begin (); - } - - /** - * @brief Iterates over the string refs (end) - */ - iterator end () const - { - return m_string_refs.end (); - } - - void mem_stat (MemStatistics *stat, MemStatistics::purpose_t purpose, int cat, bool no_self = false, void *parent = 0) const - { - if (! no_self) { - stat->add (typeid (*this), (void *) this, sizeof (*this), sizeof (*this), parent, purpose, cat); - } - db::mem_stat (stat, purpose, cat, &m_string_refs, true, (void *) this); - for (std::set::const_iterator r = m_string_refs.begin (); r != m_string_refs.end (); ++r) { - db::mem_stat (stat, purpose, cat, **r, true, parent); - } - } - private: friend class StringRef; std::set m_string_refs; - void unregister_ref (StringRef *ref) - { - if (! m_string_refs.empty ()) { - m_string_refs.erase (ref); - } - } + void unregister_ref (StringRef *ref); }; -/** - * @brief Collect memory statistics - */ -inline void mem_stat (MemStatistics *stat, MemStatistics::purpose_t purpose, int cat, const StringRepository &x, bool no_self = false, void *parent = 0) -{ - x.mem_stat (stat, purpose, cat, no_self, parent); -} - /** * @brief A text object * @@ -457,14 +390,7 @@ public: */ void translate (const text &d, db::generic_repository &, db::ArrayRepository &) { - // don't use StringRef's on translate - since those live in the source layout, we must not copy them - m_trans = d.m_trans; - m_size = d.m_size; - m_font = d.m_font; - m_halign = d.m_halign; - m_valign = d.m_valign; - - string (d.string ()); + *this = d; } /** @@ -473,7 +399,7 @@ public: template void translate (const text &d, const T &t, db::generic_repository &r, db::ArrayRepository &a) { - translate (d, r, a); + *this = d; transform (t); } @@ -854,24 +780,11 @@ private: if (c != 0) { return c < 0; } - } else { - if (mp_ptr != b.mp_ptr) { - // if references are present, use their pointers rather than the strings - // if they belong to the same collection - const StringRef *r1 = reinterpret_cast (mp_ptr - 1); - const StringRef *r2 = reinterpret_cast (b.mp_ptr - 1); - if (r1->rep () != r2->rep ()) { - int c = strcmp (r1->value ().c_str (), r2->value ().c_str ()); - if (c != 0) { - return c < 0; - } - } else { - return mp_ptr < b.mp_ptr; - } - } + } else if (mp_ptr != b.mp_ptr) { + // if references are present, use their pointers rather than the strings + return mp_ptr < b.mp_ptr; } -#if 1 // Compare size and presentation flags - without that, the text repository does not work properly. if (m_size != b.m_size) { return m_size < b.m_size; @@ -885,7 +798,6 @@ private: if (m_valign != b.m_valign) { return m_valign < b.m_valign; } -#endif return false; } @@ -900,33 +812,16 @@ private: if (c != 0) { return false; } - } else { - if (mp_ptr != b.mp_ptr) { - // if references are present, use their pointers rather than the strings - // if they belong to the same collection - const StringRef *r1 = reinterpret_cast (mp_ptr - 1); - const StringRef *r2 = reinterpret_cast (b.mp_ptr - 1); - if (r1->rep () != r2->rep ()) { - int c = strcmp (r1->value ().c_str (), r2->value ().c_str ()); - if (c != 0) { - return false; - } - } else { - return false; - } - } + } else if (mp_ptr != b.mp_ptr) { + // if references are present, use their pointers rather than the strings + return false; } -#if 1 // Compare size and presentation flags - without that, the text repository does not work properly. if (m_size != b.m_size) { return false; } return m_font == b.m_font && m_halign == b.m_halign && m_valign == b.m_valign; -#else - // Don't compare size, font and alignment - return true; -#endif } }; diff --git a/src/db/unit_tests/dbLayoutTests.cc b/src/db/unit_tests/dbLayoutTests.cc index 73fa63e36..4c10d442e 100644 --- a/src/db/unit_tests/dbLayoutTests.cc +++ b/src/db/unit_tests/dbLayoutTests.cc @@ -26,6 +26,7 @@ #include "dbColdProxy.h" #include "dbLibraryProxy.h" #include "dbTextWriter.h" +#include "dbCellMapping.h" #include "tlString.h" #include "tlUnitTest.h" @@ -808,3 +809,39 @@ TEST(9_ErrorLayer) EXPECT_EQ (l.is_special_layer (1), true); EXPECT_EQ (int (l.layers ()), 2); } + +TEST(10_TranslateStringRefs) +{ + db::Manager m; + db::Layout l (&m); + db::Cell &top = l.cell (l.add_cell ("TOP")); + l.insert_layer (db::LayerProperties (1, 0)); + + { + std::unique_ptr t (new db::Layout ()); + db::Cell &ttop = t->cell (t->add_cell ("TOP")); + unsigned int tl1 = t->insert_layer (db::LayerProperties (1, 0)); + + const db::StringRef *string_ref = db::StringRepository::instance ()->create_string_ref (); + db::StringRepository::change_string_ref (string_ref, "TEXT"); + db::Text txt (string_ref, db::Trans ()); + ttop.shapes (tl1).insert (db::TextRef (txt, t->shape_repository ())); + ttop.shapes (tl1).insert (txt); + + EXPECT_EQ (l2s (*t), "begin_lib 0.001\nbegin_cell {TOP}\ntext 1 0 0 0 {0 0} {TEXT}\ntext 1 0 0 0 {0 0} {TEXT}\nend_cell\nend_lib\n"); + + db::CellMapping cm; + cm.create_single_mapping (l, top.cell_index (), *t, ttop.cell_index ()); + l.copy_tree_shapes (*t, cm); + EXPECT_EQ (l2s (l), "begin_lib 0.001\nbegin_cell {TOP}\ntext 1 0 0 0 {0 0} {TEXT}\ntext 1 0 0 0 {0 0} {TEXT}\nend_cell\nend_lib\n"); + + db::StringRepository::change_string_ref (string_ref, "TEXT_NEW"); + + EXPECT_EQ (l2s (*t), "begin_lib 0.001\nbegin_cell {TOP}\ntext 1 0 0 0 {0 0} {TEXT_NEW}\ntext 1 0 0 0 {0 0} {TEXT_NEW}\nend_cell\nend_lib\n"); + // also the copy changes: + EXPECT_EQ (l2s (l), "begin_lib 0.001\nbegin_cell {TOP}\ntext 1 0 0 0 {0 0} {TEXT_NEW}\ntext 1 0 0 0 {0 0} {TEXT_NEW}\nend_cell\nend_lib\n"); + } + + // after deleting the tmp layout, l is still valid + EXPECT_EQ (l2s (l), "begin_lib 0.001\nbegin_cell {TOP}\ntext 1 0 0 0 {0 0} {TEXT_NEW}\ntext 1 0 0 0 {0 0} {TEXT_NEW}\nend_cell\nend_lib\n"); +} diff --git a/src/db/unit_tests/dbTextTests.cc b/src/db/unit_tests/dbTextTests.cc index 97adc5ebf..a1628d6c1 100644 --- a/src/db/unit_tests/dbTextTests.cc +++ b/src/db/unit_tests/dbTextTests.cc @@ -55,10 +55,10 @@ TEST(1) TEST(2) { - db::StringRepository rep; + size_t n = db::StringRepository::instance ()->size (); - const db::StringRef *ref = rep.create_string_ref (); - rep.change_string_ref (ref, "ABER"); + const db::StringRef *ref = db::StringRepository::instance ()->create_string_ref (); + db::StringRepository::change_string_ref (ref, "ABER"); db::Text t (ref, db::Trans ()); db::Text tt (t); @@ -69,9 +69,9 @@ TEST(2) EXPECT_EQ (t < tt, false); EXPECT_EQ (tt < t, false); - EXPECT_EQ (rep.size (), size_t (1)); + EXPECT_EQ (db::StringRepository::instance ()->size (), n + size_t (1)); - rep.change_string_ref (ref, "NOCHWAS"); + db::StringRepository::change_string_ref (ref, "NOCHWAS"); EXPECT_EQ (std::string (t.string ()), "NOCHWAS"); EXPECT_EQ (std::string (tt.string ()), "NOCHWAS"); @@ -80,12 +80,12 @@ TEST(2) EXPECT_EQ (t < tt, false); EXPECT_EQ (tt < t, false); - EXPECT_EQ (rep.size (), size_t (1)); + EXPECT_EQ (db::StringRepository::instance ()->size (), n + size_t (1)); t = db::Text (); tt = db::Text (); - EXPECT_EQ (rep.size (), size_t (0)); + EXPECT_EQ (db::StringRepository::instance ()->size (), n); } TEST(3) @@ -98,8 +98,8 @@ TEST(3) unsigned int l2 = ly2.insert_layer (); db::Cell *c2 = &ly2.cell (ly2.add_cell ("TOP")); - const db::StringRef *ref1 = ly1.string_repository ().create_string_ref (); - ly1.string_repository ().change_string_ref (ref1, "X"); + const db::StringRef *ref1 = db::StringRepository::instance ()->create_string_ref (); + db::StringRepository::change_string_ref (ref1, "X"); db::Text t (ref1, db::Trans ()); db::Shape s1 = c1->shapes (l1).insert (t); @@ -111,7 +111,7 @@ TEST(3) db::Shape s1dup = *c1dup->shapes (l1dup).begin (db::ShapeIterator::All); EXPECT_EQ (std::string (s1dup.text_string ()), "X"); - ly1.string_repository ().change_string_ref (ref1, "U"); + db::StringRepository::change_string_ref (ref1, "U"); EXPECT_EQ (std::string (s1.text_string ()), "U"); EXPECT_EQ (std::string (s1dup.text_string ()), "X"); @@ -125,7 +125,7 @@ TEST(3) EXPECT_EQ (std::string (s2a.text_string ()), "U"); EXPECT_EQ (std::string (s2b.text_string ()), "U"); - ly1.string_repository ().change_string_ref (ref1, "A"); + db::StringRepository::change_string_ref (ref1, "A"); EXPECT_EQ (std::string (tt.string ()), "U"); EXPECT_EQ (std::string (s1.text_string ()), "A"); diff --git a/src/plugins/streamers/oasis/db_plugin/dbOASISReader.cc b/src/plugins/streamers/oasis/db_plugin/dbOASISReader.cc index d9fd91ddd..a5f63ce65 100644 --- a/src/plugins/streamers/oasis/db_plugin/dbOASISReader.cc +++ b/src/plugins/streamers/oasis/db_plugin/dbOASISReader.cc @@ -1229,7 +1229,7 @@ OASISReader::do_read (db::Layout &layout) if (ts == m_textstrings.end ()) { error (tl::sprintf (tl::to_string (tr ("No text string defined for text string id %ld")), fw->first)); } else { - layout.string_repository ().change_string_ref (fw->second, ts->second); + StringRepository::change_string_ref (fw->second, ts->second); } } @@ -2017,7 +2017,7 @@ OASISReader::do_read_text (bool xy_absolute, mm_text_string.reset (); mm_text_string_id = id; - const db::StringRef *string_ref = layout.string_repository ().create_string_ref (); + const db::StringRef *string_ref = db::StringRepository::instance ()->create_string_ref (); m_text_forward_references.insert (std::make_pair (id, string_ref)); } else {