From 4c4419296b8430d754a96da0eaae731c594f4d06 Mon Sep 17 00:00:00 2001 From: Gabriel Busnot Date: Wed, 19 Jul 2023 17:45:29 +0200 Subject: [PATCH] base: Unit tests miscellaneous patches (#73) * base: Fix Memoizer constructor parameter type * base: switch from new to mk_unq in amo.test.cc * base: Fix memory management in IniFile * base: Fix memory management in Trie * sim: Fix out-of-bounds access in CheckpointIn::setDir Change-Id: Iac50bbf01b6d7acc458c786da8ac371582a4ce09 --------- Co-authored-by: Gabriel Busnot --- src/base/amo.test.cc | 74 ++++++++++++++++++------------ src/base/inifile.cc | 105 +++++++++++++++++++------------------------ src/base/inifile.hh | 23 +++++----- src/base/memoizer.hh | 2 +- src/base/trie.hh | 64 +++++++++++--------------- src/sim/serialize.cc | 5 ++- 6 files changed, 135 insertions(+), 138 deletions(-) diff --git a/src/base/amo.test.cc b/src/base/amo.test.cc index 10e5540da4..e511117eea 100644 --- a/src/base/amo.test.cc +++ b/src/base/amo.test.cc @@ -64,9 +64,10 @@ TEST(AmoTest, AtomicOpMin) std::string test_string_smaller = "apple"; std::string test_string_bigger = "cat"; - TypedAtomicOpFunctor *amo_op_int = new AtomicOpMin(10); - TypedAtomicOpFunctor *amo_op_string = - new AtomicOpMin("base"); + std::unique_ptr> amo_op_int = + std::make_unique>(10); + std::unique_ptr> amo_op_string = + std::make_unique>("base"); amo_op_int->execute(&test_int_smaller); amo_op_int->execute(&test_int_bigger); amo_op_string->execute(&test_string_smaller); @@ -85,9 +86,10 @@ TEST(AmoTest, AtomicOpMax) std::string test_string_smaller = "apple"; std::string test_string_bigger = "cat"; - TypedAtomicOpFunctor *amo_op_int = new AtomicOpMax(10); - TypedAtomicOpFunctor *amo_op_string = - new AtomicOpMax("base"); + std::unique_ptr> amo_op_int = + std::make_unique>(10); + std::unique_ptr> amo_op_string = + std::make_unique>("base"); amo_op_int->execute(&test_int_smaller); amo_op_int->execute(&test_int_bigger); amo_op_string->execute(&test_string_smaller); @@ -104,8 +106,10 @@ TEST(AmoTest, AtomicOpDec) int test_int = 10; char test_char = 'c'; - TypedAtomicOpFunctor *amo_op_int = new AtomicOpDec(); - TypedAtomicOpFunctor *amo_op_char = new AtomicOpDec(); + std::unique_ptr> amo_op_int = + std::make_unique>(); + std::unique_ptr> amo_op_char = + std::make_unique>(); amo_op_int->execute(&test_int); amo_op_char->execute(&test_char); @@ -118,8 +122,10 @@ TEST(AmoTest, AtomicOpInc) int test_int = 10; char test_char = 'c'; - TypedAtomicOpFunctor *amo_op_int = new AtomicOpInc(); - TypedAtomicOpFunctor *amo_op_char = new AtomicOpInc(); + std::unique_ptr> amo_op_int = + std::make_unique>(); + std::unique_ptr> amo_op_char = + std::make_unique>(); amo_op_int->execute(&test_int); amo_op_char->execute(&test_char); @@ -132,8 +138,10 @@ TEST(AmoTest, AtomicOpSub) int test_int = 10; char test_char = 'c'; - TypedAtomicOpFunctor *amo_op_int = new AtomicOpSub(2); - TypedAtomicOpFunctor *amo_op_char = new AtomicOpSub('a'); + std::unique_ptr> amo_op_int = + std::make_unique>(2); + std::unique_ptr> amo_op_char = + std::make_unique>('a'); amo_op_int->execute(&test_int); amo_op_char->execute(&test_char); @@ -146,8 +154,10 @@ TEST(AmoTest, AtomicOpAdd) int test_int = 10; char test_char = 'c'; - TypedAtomicOpFunctor *amo_op_int = new AtomicOpAdd(2); - TypedAtomicOpFunctor *amo_op_char = new AtomicOpAdd(2); + std::unique_ptr> amo_op_int = + std::make_unique>(2); + std::unique_ptr> amo_op_char = + std::make_unique>(2); amo_op_int->execute(&test_int); amo_op_char->execute(&test_char); @@ -160,8 +170,10 @@ TEST(AmoTest, AtomicOpExch) int test_int = 10; char test_char = 'c'; - TypedAtomicOpFunctor *amo_op_int = new AtomicOpExch(2); - TypedAtomicOpFunctor *amo_op_char = new AtomicOpExch('a'); + std::unique_ptr> amo_op_int = + std::make_unique>(2); + std::unique_ptr> amo_op_char = + std::make_unique>('a'); amo_op_int->execute(&test_int); amo_op_char->execute(&test_char); @@ -174,8 +186,10 @@ TEST(AmoTest, AtomicOpXor) int test_int = 10; char test_char = 'c'; - TypedAtomicOpFunctor *amo_op_int = new AtomicOpXor(2); - TypedAtomicOpFunctor *amo_op_char = new AtomicOpXor('a'); + std::unique_ptr> amo_op_int = + std::make_unique>(2); + std::unique_ptr> amo_op_char = + std::make_unique>('a'); amo_op_int->execute(&test_int); amo_op_char->execute(&test_char); @@ -188,8 +202,10 @@ TEST(AmoTest, AtomicOpOr) int test_int = 8; bool test_bool = true; - TypedAtomicOpFunctor *amo_op_int = new AtomicOpOr(2); - TypedAtomicOpFunctor *amo_op_bool = new AtomicOpOr(false); + std::unique_ptr> amo_op_int = + std::make_unique>(2); + std::unique_ptr> amo_op_bool = + std::make_unique>(false); amo_op_int->execute(&test_int); amo_op_bool->execute(&test_bool); @@ -202,8 +218,10 @@ TEST(AmoTest, AtomicOpAnd) int test_int = 10; char test_char = 'c'; - TypedAtomicOpFunctor *amo_op_int = new AtomicOpAnd(6); - TypedAtomicOpFunctor *amo_op_char = new AtomicOpAnd('a'); + std::unique_ptr> amo_op_int = + std::make_unique>(6); + std::unique_ptr> amo_op_char = + std::make_unique>('a'); amo_op_int->execute(&test_int); amo_op_char->execute(&test_char); @@ -215,8 +233,8 @@ TEST(AmoTest, AtomicGeneric2Op) { int test_int = 9; - TypedAtomicOpFunctor *amo_op_int = - new AtomicGeneric2Op(9, multiply2Op); + std::unique_ptr> amo_op_int = + std::make_unique>(9, multiply2Op); amo_op_int->execute(&test_int); EXPECT_EQ(test_int, 81); @@ -226,8 +244,8 @@ TEST(AmoTest, AtomicGeneric3Op) { int test_int = 2; - TypedAtomicOpFunctor *amo_op_int = - new AtomicGeneric3Op(4, 3, multiply3Op); + std::unique_ptr> amo_op_int = + std::make_unique>(4, 3, multiply3Op); amo_op_int->execute(&test_int); EXPECT_EQ(test_int, 24); @@ -239,8 +257,8 @@ TEST(AmoTest, AtomicGenericPair3Op) std::array a = {6, 3}; std::array c = {10, 8}; - TypedAtomicOpFunctor *amo_op_int = - new AtomicGenericPair3Op(a, c, addSubColumns); + std::unique_ptr> amo_op_int = + std::make_unique>(a, c, addSubColumns); amo_op_int->execute(&test_int); EXPECT_EQ(test_int, 10); diff --git a/src/base/inifile.cc b/src/base/inifile.cc index 8c0662d0e3..23253e9df5 100644 --- a/src/base/inifile.cc +++ b/src/base/inifile.cc @@ -42,17 +42,6 @@ namespace gem5 IniFile::IniFile() {} -IniFile::~IniFile() -{ - SectionTable::iterator i = table.begin(); - SectionTable::iterator end = table.end(); - - while (i != end) { - delete (*i).second; - ++i; - } -} - bool IniFile::load(const std::string &file) { @@ -82,15 +71,15 @@ IniFile::Section::addEntry(const std::string &entryName, if (ei == table.end()) { // new entry - table[entryName] = new Entry(value); + table.emplace(entryName, value); } else if (append) { // append new reult to old entry - ei->second->appendValue(value); + ei->second.appendValue(value); } else { // override old entry - ei->second->setValue(value); + ei->second.setValue(value); } } @@ -120,39 +109,42 @@ IniFile::Section::add(const std::string &assignment) IniFile::Entry * +IniFile::Section::findEntry(const std::string &entryName) +{ + return const_cast( + std::as_const(*this).findEntry(entryName)); +} + +const IniFile::Entry * IniFile::Section::findEntry(const std::string &entryName) const { referenced = true; - EntryTable::const_iterator ei = table.find(entryName); + auto ei = table.find(entryName); - return (ei == table.end()) ? NULL : ei->second; + return (ei == table.end()) ? nullptr : &ei->second; } IniFile::Section * IniFile::addSection(const std::string §ionName) { - SectionTable::iterator i = table.find(sectionName); - - if (i != table.end()) { - return i->second; - } - else { - // new entry - Section *sec = new Section(); - table[sectionName] = sec; - return sec; - } + return &table[sectionName]; } - IniFile::Section * +IniFile::findSection(const std::string §ionName) +{ + return const_cast( + std::as_const(*this).findSection(sectionName)); +} + +const IniFile::Section * IniFile::findSection(const std::string §ionName) const { - SectionTable::const_iterator i = table.find(sectionName); + auto i = table.find(sectionName); - return (i == table.end()) ? NULL : i->second; + return (i == table.end()) ? nullptr : &i->second; } @@ -215,11 +207,11 @@ bool IniFile::find(const std::string §ionName, const std::string &entryName, std::string &value) const { - Section *section = findSection(sectionName); + auto* section = findSection(sectionName); if (section == NULL) return false; - Entry *entry = section->findEntry(entryName); + auto* entry = section->findEntry(entryName); if (entry == NULL) return false; @@ -232,7 +224,7 @@ bool IniFile::entryExists(const std::string §ionName, const std::string &entryName) const { - Section *section = findSection(sectionName); + auto* section = findSection(sectionName); if (!section) return false; @@ -248,13 +240,13 @@ IniFile::sectionExists(const std::string §ionName) const bool -IniFile::Section::printUnreferenced(const std::string §ionName) +IniFile::Section::printUnreferenced(const std::string §ionName) const { bool unref = false; bool search_unref_entries = false; std::vector unref_ok_entries; - Entry *entry = findEntry("unref_entries_ok"); + auto* entry = findEntry("unref_entries_ok"); if (entry != NULL) { tokenize(unref_ok_entries, entry->getValue(), ' '); if (unref_ok_entries.size()) { @@ -262,10 +254,9 @@ IniFile::Section::printUnreferenced(const std::string §ionName) } } - for (EntryTable::iterator ei = table.begin(); - ei != table.end(); ++ei) { - const std::string &entryName = ei->first; - entry = ei->second; + for (auto& ei: table) { + const std::string &entryName = ei.first; + entry = &ei.second; if (entryName == "unref_section_ok" || entryName == "unref_entries_ok") @@ -294,32 +285,29 @@ IniFile::Section::printUnreferenced(const std::string §ionName) void IniFile::getSectionNames(std::vector &list) const { - for (SectionTable::const_iterator i = table.begin(); - i != table.end(); ++i) - { - list.push_back((*i).first); + for (auto& entry: table) { + auto& sectionName = entry.first; + list.push_back(sectionName); } } bool -IniFile::printUnreferenced() +IniFile::printUnreferenced() const { bool unref = false; - for (SectionTable::iterator i = table.begin(); - i != table.end(); ++i) { - const std::string §ionName = i->first; - Section *section = i->second; + for (auto& entry: table) { + auto& [sectionName, section] = entry; - if (!section->isReferenced()) { - if (section->findEntry("unref_section_ok") == NULL) { + if (!section.isReferenced()) { + if (section.findEntry("unref_section_ok") == NULL) { std::cerr << "Section " << sectionName << " not referenced." << std::endl; unref = true; } } else { - if (section->printUnreferenced(sectionName)) { + if (section.printUnreferenced(sectionName)) { unref = true; } } @@ -330,12 +318,11 @@ IniFile::printUnreferenced() void -IniFile::Section::dump(const std::string §ionName) +IniFile::Section::dump(const std::string §ionName) const { - for (EntryTable::iterator ei = table.begin(); - ei != table.end(); ++ei) { - std::cout << sectionName << ": " << (*ei).first << " => " - << (*ei).second->getValue() << "\n"; + for (auto& ei: table) { + std::cout << sectionName << ": " << ei.first << " => " + << ei.second.getValue() << "\n"; } } @@ -344,7 +331,7 @@ IniFile::dump() { for (SectionTable::iterator i = table.begin(); i != table.end(); ++i) { - i->second->dump(i->first); + i->second.dump(i->first); } } @@ -364,9 +351,9 @@ void IniFile::visitSection(const std::string §ionName, IniFile::VisitSectionCallback cb) { - const auto& section = *table.at(sectionName); + const auto& section = table.at(sectionName); for (const auto& pair : section) { - cb(pair.first, pair.second->getValue()); + cb(pair.first, pair.second.getValue()); } } diff --git a/src/base/inifile.hh b/src/base/inifile.hh index 72f1df7b05..d17193d5bf 100644 --- a/src/base/inifile.hh +++ b/src/base/inifile.hh @@ -72,7 +72,7 @@ class IniFile } /// Has this entry been used? - bool isReferenced() { return referenced; } + bool isReferenced() const { return referenced; } /// Fetch the value. const std::string &getValue() const; @@ -94,7 +94,7 @@ class IniFile class Section { /// EntryTable type. Map of strings to Entry object pointers. - typedef std::unordered_map EntryTable; + typedef std::unordered_map EntryTable; EntryTable table; ///< Table of entries. mutable bool referenced; ///< Has this section been used? @@ -107,7 +107,7 @@ class IniFile } /// Has this section been used? - bool isReferenced() { return referenced; } + bool isReferenced() const { return referenced; } /// Add an entry to the table. If an entry with the same name /// already exists, the 'append' parameter is checked If true, @@ -125,24 +125,25 @@ class IniFile /// Find the entry with the given name. /// @retval Pointer to the entry object, or NULL if none. - Entry *findEntry(const std::string &entryName) const; + Entry *findEntry(const std::string &entryName); + const Entry *findEntry(const std::string &entryName) const; /// Print the unreferenced entries in this section to cerr. /// Messages can be suppressed using "unref_section_ok" and /// "unref_entries_ok". /// @param sectionName Name of this section, for use in output message. /// @retval True if any entries were printed. - bool printUnreferenced(const std::string §ionName); + bool printUnreferenced(const std::string §ionName) const; /// Print the contents of this section to cout (for debugging). - void dump(const std::string §ionName); + void dump(const std::string §ionName) const; EntryTable::const_iterator begin() const; EntryTable::const_iterator end() const; }; /// SectionTable type. Map of strings to Section object pointers. - typedef std::unordered_map SectionTable; + typedef std::unordered_map SectionTable; protected: /// Hash of section names to Section object pointers. @@ -155,15 +156,13 @@ class IniFile /// Look up section with the given name. /// @retval Pointer to section object, or NULL if not found. - Section *findSection(const std::string §ionName) const; + Section *findSection(const std::string §ionName); + const Section *findSection(const std::string §ionName) const; public: /// Constructor. IniFile(); - /// Destructor. - ~IniFile(); - /// Load parameter settings from given istream. This is a helper /// function for load(string) and loadCPP(), which open a file /// and then pass it here. @@ -206,7 +205,7 @@ class IniFile /// Print unreferenced entries in object. Iteratively calls /// printUnreferend() on all the constituent sections. - bool printUnreferenced(); + bool printUnreferenced() const; /// Dump contents to cout. For debugging. void dump(); diff --git a/src/base/memoizer.hh b/src/base/memoizer.hh index c3390887b7..4d3816e9ea 100644 --- a/src/base/memoizer.hh +++ b/src/base/memoizer.hh @@ -85,7 +85,7 @@ class Memoizer using ret_type = Ret; using args_type = std::tuple; - constexpr Memoizer(Ret _func(Args...)) + constexpr Memoizer(Ret (*_func)(Args...)) : func(_func) { validateMemoizer(); diff --git a/src/base/trie.hh b/src/base/trie.hh index 477bfbde14..47d60b7632 100644 --- a/src/base/trie.hh +++ b/src/base/trie.hh @@ -70,7 +70,7 @@ class Trie Value *value; Node *parent; - Node *kids[2]; + std::unique_ptr kids[2]; Node(Key _key, Key _mask, Value *_val) : key(_key & _mask), mask(_mask), value(_val), @@ -83,16 +83,8 @@ class Trie void clear() { - if (kids[1]) { - kids[1]->clear(); - delete kids[1]; - kids[1] = NULL; - } - if (kids[0]) { - kids[0]->clear(); - delete kids[0]; - kids[0] = NULL; - } + kids[1].reset(); + kids[0].reset(); } void @@ -188,9 +180,9 @@ class Trie return node; if (node->kids[0] && node->kids[0]->matches(key)) - node = node->kids[0]; + node = node->kids[0].get(); else if (node->kids[1] && node->kids[1]->matches(key)) - node = node->kids[1]; + node = node->kids[1].get(); else node = NULL; } @@ -225,8 +217,8 @@ class Trie // Walk past all the nodes this new node will be inserted after. They // can be ignored for the purposes of this function. Node *node = &head; - while (goesAfter(&node, node->kids[0], key, new_mask) || - goesAfter(&node, node->kids[1], key, new_mask)) + while (goesAfter(&node, node->kids[0].get(), key, new_mask) || + goesAfter(&node, node->kids[1].get(), key, new_mask)) {} assert(node); @@ -239,14 +231,13 @@ class Trie } for (unsigned int i = 0; i < 2; i++) { - Node *&kid = node->kids[i]; - Node *new_node; + auto& kid = node->kids[i]; if (!kid) { // No kid. Add a new one. - new_node = new Node(key, new_mask, val); + auto new_node = std::make_unique(key, new_mask, val); new_node->parent = node; - kid = new_node; - return new_node; + kid = std::move(new_node); + return kid.get(); } // Walk down the leg until something doesn't match or we run out @@ -266,23 +257,23 @@ class Trie continue; // At the point we walked to above, add a new node. - new_node = new Node(key, cur_mask, NULL); + auto new_node = std::make_unique(key, cur_mask, nullptr); new_node->parent = node; - kid->parent = new_node; - new_node->kids[0] = kid; - kid = new_node; + kid->parent = new_node.get(); + new_node->kids[0] = std::move(kid); + kid = std::move(new_node); // If we ran out of bits, the value goes right here. if (cur_mask == new_mask) { - new_node->value = val; - return new_node; + kid->value = val; + return kid.get(); } // Still more bits to deal with, so add a new node for that path. - new_node = new Node(key, new_mask, val); - new_node->parent = kid; - kid->kids[1] = new_node; - return new_node; + new_node = std::make_unique(key, new_mask, val); + new_node->parent = kid.get(); + kid->kids[1] = std::move(new_node); + return kid->kids[1].get(); } panic("Reached the end of the Trie insert function!\n"); @@ -332,23 +323,22 @@ class Trie if (node->kids[0]) node->kids[0]->parent = parent; // Figure out which kid we are, and update our parent's pointers. - if (parent->kids[0] == node) - parent->kids[0] = node->kids[0]; - else if (parent->kids[1] == node) - parent->kids[1] = node->kids[0]; + if (parent->kids[0].get() == node) + parent->kids[0] = std::move(node->kids[0]); + else if (parent->kids[1].get() == node) + parent->kids[1] = std::move(node->kids[0]); else panic("Trie: Inconsistent parent/kid relationship.\n"); // Make sure if the parent only has one kid, it's kid[0]. if (parent->kids[1] && !parent->kids[0]) { - parent->kids[0] = parent->kids[1]; - parent->kids[1] = NULL; + parent->kids[0] = std::move(parent->kids[1]); + parent->kids[1] = nullptr; } // If the parent has less than two kids and no cargo and isn't the // root, delete it too. if (!parent->kids[1] && !parent->value && parent->parent) remove(parent); - delete node; return val; } diff --git a/src/sim/serialize.cc b/src/sim/serialize.cc index 2b1bd35f16..0f722a017e 100644 --- a/src/sim/serialize.cc +++ b/src/sim/serialize.cc @@ -145,8 +145,11 @@ CheckpointIn::setDir(const std::string &name) // appears to have a format placeholder in it. currentDirectory = (name.find("%") != std::string::npos) ? csprintf(name, curTick()) : name; - if (currentDirectory[currentDirectory.size() - 1] != '/') + auto isEmptyPath = currentDirectory.empty(); + auto endsWithSlash = !isEmptyPath && currentDirectory.back() == '/'; + if (!endsWithSlash) { currentDirectory += "/"; + } return currentDirectory; }