From 2fbbdad618a83f49597eea33f70be7c5b51ba5dd Mon Sep 17 00:00:00 2001 From: Richard Cooper Date: Thu, 30 Nov 2023 09:52:53 +0000 Subject: [PATCH 1/4] base: Add encapsulation to the loader::Symbol class This commit converts `gem5::loader::Symbol` to a full class with private members, enforcing encapsulation. Until now client code has been able to (and does) access members directly. This change will enable class invariants to be enforced via accessor methods. Change-Id: Ia0b5b080d4f656637a211808e13dce1ddca74541 Reviewed-by: Andreas Sandberg --- src/arch/arm/insts/static_inst.cc | 10 +-- src/arch/generic/linux/threadinfo.hh | 2 +- src/arch/mips/isa/formats/branch.isa | 4 +- src/arch/power/insts/branch.cc | 4 +- src/arch/power/process.cc | 4 +- src/arch/riscv/linux/fs_workload.cc | 8 +-- src/arch/sparc/insts/branch.cc | 6 +- src/base/loader/elf_object.cc | 33 +++++---- src/base/loader/symtab.cc | 29 +++++--- src/base/loader/symtab.hh | 77 ++++++++++++++++---- src/base/loader/symtab.test.cc | 104 ++++++++++++++------------- src/cpu/base.cc | 6 +- src/cpu/exetrace.cc | 6 +- src/cpu/profile.cc | 4 +- src/kern/linux/helpers.cc | 30 ++++---- src/sim/kernel_workload.cc | 4 +- src/sim/workload.hh | 2 +- 17 files changed, 201 insertions(+), 132 deletions(-) diff --git a/src/arch/arm/insts/static_inst.cc b/src/arch/arm/insts/static_inst.cc index 54045f2fb1..a645b39270 100644 --- a/src/arch/arm/insts/static_inst.cc +++ b/src/arch/arm/insts/static_inst.cc @@ -401,8 +401,8 @@ ArmStaticInst::printTarget(std::ostream &os, Addr target, if (symtab) { auto it = symtab->findNearest(target); if (it != symtab->end()) { - ccprintf(os, "<%s", it->name); - Addr delta = target - it->address; + ccprintf(os, "<%s", it->name()); + Addr delta = target - it->address(); if (delta) ccprintf(os, "+%d>", delta); else @@ -486,9 +486,9 @@ ArmStaticInst::printMemSymbol(std::ostream &os, if (symtab) { auto it = symtab->findNearest(addr); if (it != symtab->end()) { - ccprintf(os, "%s%s", prefix, it->name); - if (it->address != addr) - ccprintf(os, "+%d", addr - it->address); + ccprintf(os, "%s%s", prefix, it->name()); + if (it->address() != addr) + ccprintf(os, "+%d", addr - it->address()); ccprintf(os, suffix); } } diff --git a/src/arch/generic/linux/threadinfo.hh b/src/arch/generic/linux/threadinfo.hh index 70511c47fa..8af6d6f1de 100644 --- a/src/arch/generic/linux/threadinfo.hh +++ b/src/arch/generic/linux/threadinfo.hh @@ -60,7 +60,7 @@ class ThreadInfo return false; } - data = TranslatingPortProxy(tc).read(it->address, byteOrder); + data = TranslatingPortProxy(tc).read(it->address(), byteOrder); return true; } diff --git a/src/arch/mips/isa/formats/branch.isa b/src/arch/mips/isa/formats/branch.isa index 1b7c1057b6..0da32eb7ca 100644 --- a/src/arch/mips/isa/formats/branch.isa +++ b/src/arch/mips/isa/formats/branch.isa @@ -194,7 +194,7 @@ output decoder {{ loader::SymbolTable::const_iterator it; if (symtab && (it = symtab->find(target)) != symtab->end()) - ss << it->name; + ss << it->name(); else ccprintf(ss, "%#x", target); @@ -214,7 +214,7 @@ output decoder {{ } else if (_numSrcRegs == 0) { loader::SymbolTable::const_iterator it; if (symtab && (it = symtab->find(disp)) != symtab->end()) - ss << it->name; + ss << it->name(); else ccprintf(ss, "0x%x", disp); } else if (_numSrcRegs == 1) { diff --git a/src/arch/power/insts/branch.cc b/src/arch/power/insts/branch.cc index b68f89bf9b..c422c5d535 100644 --- a/src/arch/power/insts/branch.cc +++ b/src/arch/power/insts/branch.cc @@ -97,7 +97,7 @@ BranchOp::generateDisassembly( loader::SymbolTable::const_iterator it; if (symtab && (it = symtab->find(target)) != symtab->end()) - ss << it->name; + ss << it->name(); else ccprintf(ss, "%#x", target); @@ -149,7 +149,7 @@ BranchDispCondOp::generateDisassembly( loader::SymbolTable::const_iterator it; if (symtab && (it = symtab->find(target)) != symtab->end()) - ss << it->name; + ss << it->name(); else ccprintf(ss, "%#x", target); diff --git a/src/arch/power/process.cc b/src/arch/power/process.cc index d31aca2faf..88b462d7a2 100644 --- a/src/arch/power/process.cc +++ b/src/arch/power/process.cc @@ -122,8 +122,8 @@ PowerProcess::initState() loader::Symbol symbol = sym; // Try to read entry point from function descriptor - if (initVirtMem->tryReadBlob(sym.address, &entry, sizeof(Addr))) - symbol.address = gtoh(entry, byteOrder); + if (initVirtMem->tryReadBlob(sym.address(), &entry, sizeof(Addr))) + symbol.relocate(gtoh(entry, byteOrder)); symbolTable->insert(symbol); } diff --git a/src/arch/riscv/linux/fs_workload.cc b/src/arch/riscv/linux/fs_workload.cc index 2946d6c324..78572e647b 100644 --- a/src/arch/riscv/linux/fs_workload.cc +++ b/src/arch/riscv/linux/fs_workload.cc @@ -86,8 +86,8 @@ BootloaderKernelWorkload::loadBootloaderSymbolTable() bootloaderSymbolTable.offset( bootloader_paddr_offset )->functionSymbols()->rename( - [](std::string &name) { - name = "bootloader." + name; + [](const std::string &name) { + return "bootloader." + name; } ); loader::debugSymbolTable.insert(*renamedBootloaderSymbolTable); @@ -102,8 +102,8 @@ BootloaderKernelWorkload::loadKernelSymbolTable() kernelSymbolTable = kernel->symtab(); auto renamedKernelSymbolTable = \ kernelSymbolTable.functionSymbols()->rename( - [](std::string &name) { - name = "kernel." + name; + [](const std::string &name) { + return "kernel." + name; } ); loader::debugSymbolTable.insert(*renamedKernelSymbolTable); diff --git a/src/arch/sparc/insts/branch.cc b/src/arch/sparc/insts/branch.cc index 84861cc2c3..7109f9a95d 100644 --- a/src/arch/sparc/insts/branch.cc +++ b/src/arch/sparc/insts/branch.cc @@ -88,9 +88,9 @@ BranchDisp::generateDisassembly( loader::SymbolTable::const_iterator it; if (symtab && (it = symtab->findNearest(target)) != symtab->end()) { - ccprintf(response, " <%s", it->name); - if (it->address != target) - ccprintf(response, "+%d>", target - it->address); + ccprintf(response, " <%s", it->name()); + if (it->address() != target) + ccprintf(response, "+%d>", target - it->address()); else ccprintf(response, ">"); } diff --git a/src/base/loader/elf_object.cc b/src/base/loader/elf_object.cc index 5eef4cb44c..5b2cda6894 100644 --- a/src/base/loader/elf_object.cc +++ b/src/base/loader/elf_object.cc @@ -1,5 +1,5 @@ /* - * Copyright (c) 2011-2013, 2019 ARM Limited + * Copyright (c) 2011-2013, 2019, 2023 Arm Limited * All rights reserved * * The license below extends only to copyright in the software and shall @@ -178,48 +178,51 @@ ElfObject::ElfObject(ImageFileDataPtr ifd) : ObjectFile(ifd) if (!sym_name || sym_name[0] == '$') continue; - loader::Symbol symbol; - symbol.address = sym.st_value; - symbol.name = sym_name; - + loader::Symbol::Binding binding = + loader::Symbol::Binding::Global; switch (GELF_ST_BIND(sym.st_info)) { case STB_GLOBAL: - symbol.binding = loader::Symbol::Binding::Global; + binding = loader::Symbol::Binding::Global; break; case STB_LOCAL: - symbol.binding = loader::Symbol::Binding::Local; + binding = loader::Symbol::Binding::Local; break; case STB_WEAK: - symbol.binding = loader::Symbol::Binding::Weak; + binding = loader::Symbol::Binding::Weak; break; default: continue; } + loader::Symbol::SymbolType symbol_type = + loader::Symbol::SymbolType::NoType; switch (GELF_ST_TYPE(sym.st_info)) { case STT_NOTYPE: - symbol.type = loader::Symbol::SymbolType::NoType; + symbol_type = loader::Symbol::SymbolType::NoType; break; case STT_OBJECT: - symbol.type = loader::Symbol::SymbolType::Object; + symbol_type = loader::Symbol::SymbolType::Object; break; case STT_FUNC: - symbol.type = loader::Symbol::SymbolType::Function; + symbol_type = loader::Symbol::SymbolType::Function; break; case STT_SECTION: - symbol.type = loader::Symbol::SymbolType::Section; + symbol_type = loader::Symbol::SymbolType::Section; break; case STT_FILE: - symbol.type = loader::Symbol::SymbolType::File; + symbol_type = loader::Symbol::SymbolType::File; break; default: - symbol.type = loader::Symbol::SymbolType::Other; + symbol_type = loader::Symbol::SymbolType::Other; break; } + loader::Symbol symbol( + binding, symbol_type, sym_name, sym.st_value); + if (_symtab.insert(symbol)) { DPRINTF(Loader, "Symbol: %-40s value %#x.\n", - symbol.name, symbol.address); + symbol.name(), symbol.address()); } } } diff --git a/src/base/loader/symtab.cc b/src/base/loader/symtab.cc index cd7bec3d56..eb63ccbff6 100644 --- a/src/base/loader/symtab.cc +++ b/src/base/loader/symtab.cc @@ -1,4 +1,16 @@ /* + * Copyright (c) 2023 Arm Limited + * All rights reserved + * + * The license below extends only to copyright in the software and shall + * not be construed as granting a license to any other intellectual + * property including but not limited to intellectual property relating + * to a hardware implementation of the functionality of the software + * licensed hereunder. You may use the software subject to the license + * terms below provided that you ensure that this notice is replicated + * unmodified and in its entirety in all distributions of the software, + * modified or unmodified, in source code or in binary form. + * * Copyright (c) 2002-2005 The Regents of The University of Michigan * All rights reserved. * @@ -53,17 +65,17 @@ SymbolTable::clear() bool SymbolTable::insert(const Symbol &symbol) { - if (symbol.name.empty()) + if (symbol.name().empty()) return false; int idx = symbols.size(); - if (!nameMap.insert({ symbol.name, idx }).second) + if (!nameMap.insert({ symbol.name(), idx }).second) return false; // There can be multiple symbols for the same address, so always // update the addrTable multimap when we see a new symbol name. - addrMap.insert({ symbol.address, idx }); + addrMap.insert({ symbol.address(), idx }); symbols.emplace_back(symbol); @@ -98,10 +110,11 @@ SymbolTable::serialize(const std::string &base, CheckpointOut &cp) const int i = 0; for (auto &symbol: symbols) { - paramOut(cp, csprintf("%s.addr_%d", base, i), symbol.address); - paramOut(cp, csprintf("%s.symbol_%d", base, i), symbol.name); - paramOut(cp, csprintf("%s.binding_%d", base, i), (int)symbol.binding); - paramOut(cp, csprintf("%s.type_%d", base, i), (int)symbol.type); + paramOut(cp, csprintf("%s.addr_%d", base, i), symbol.address()); + paramOut(cp, csprintf("%s.symbol_%d", base, i), symbol.name()); + paramOut(cp, csprintf("%s.binding_%d", base, i), + (int)symbol.binding()); + paramOut(cp, csprintf("%s.type_%d", base, i), (int)symbol.type()); i++; } } @@ -125,7 +138,7 @@ SymbolTable::unserialize(const std::string &base, CheckpointIn &cp, binding = default_binding; if (!optParamIn(cp, csprintf("%s.type_%d", base, i), type)) type = Symbol::SymbolType::Other; - insert({binding, type, name, address}); + insert(Symbol(binding, type, name, address)); } } diff --git a/src/base/loader/symtab.hh b/src/base/loader/symtab.hh index 654064c9db..953ca39889 100644 --- a/src/base/loader/symtab.hh +++ b/src/base/loader/symtab.hh @@ -1,4 +1,16 @@ /* + * Copyright (c) 2023 Arm Limited + * All rights reserved + * + * The license below extends only to copyright in the software and shall + * not be construed as granting a license to any other intellectual + * property including but not limited to intellectual property relating + * to a hardware implementation of the functionality of the software + * licensed hereunder. You may use the software subject to the license + * terms below provided that you ensure that this notice is replicated + * unmodified and in its entirety in all distributions of the software, + * modified or unmodified, in source code or in binary form. + * * Copyright (c) 2021 Daniel R. Carvalho * Copyright (c) 2002-2005 The Regents of The University of Michigan * All rights reserved. @@ -47,8 +59,9 @@ namespace gem5 namespace loader { -struct Symbol +class Symbol { + public: enum class Binding { Global, @@ -67,12 +80,46 @@ struct Symbol Other }; - Binding binding; - SymbolType type; - std::string name; - Addr address; + Symbol(const Binding binding, const SymbolType type, + const std::string & name, const Addr addr) + : _binding(binding), _type(type), _name(name), _address(addr) + {} + + Symbol(const Symbol & other) = default; + Symbol & operator=(const Symbol & other) = default; + + Binding binding() const { + return _binding; + } + + SymbolType type() const { + return _type; + } + + std::string name() const { + return _name; + } + + void rename(const std::string & new_name) { + _name = new_name; + } + + Addr address() const { + return _address; + } + + void relocate(const Addr new_addr) { + _address = new_addr; + } + + private: + Binding _binding; + SymbolType _type; + std::string _name; + Addr _address; }; + class SymbolTable { public: @@ -171,7 +218,7 @@ class SymbolTable filterByBinding(Symbol::Binding binding) const { auto filt = [binding](const Symbol &symbol) { - return symbol.binding == binding; + return symbol.binding() == binding; }; return filter(filt); } @@ -187,7 +234,7 @@ class SymbolTable filterBySymbolType(const Symbol::SymbolType& symbol_type) const { auto filt = [symbol_type](const Symbol &symbol) { - return symbol.type == symbol_type; + return symbol.type() == symbol_type; }; return filter(filt); } @@ -242,9 +289,9 @@ class SymbolTable { SymTabOp op = [addr_offset](SymbolTable &symtab, const Symbol &symbol) { - Symbol sym = symbol; - sym.address += addr_offset; - symtab.insert(sym); + symtab.insert( + Symbol(symbol.binding(), symbol.type(), symbol.name(), + symbol.address() + addr_offset)); }; return operate(op); } @@ -260,9 +307,9 @@ class SymbolTable mask(Addr m) const { SymTabOp op = [m](SymbolTable &symtab, const Symbol &symbol) { - Symbol sym = symbol; - sym.address &= m; - symtab.insert(sym); + symtab.insert( + Symbol(symbol.binding(), symbol.type(), symbol.name(), + symbol.address() & m)); }; return operate(op); } @@ -275,11 +322,11 @@ class SymbolTable * @retval SymbolTablePtr A pointer to the modified SymbolTable copy. */ SymbolTablePtr - rename(std::function func) const + rename(std::function func) const { SymTabOp op = [func](SymbolTable &symtab, const Symbol &symbol) { Symbol sym = symbol; - func(sym.name); + sym.rename(func(sym.name())); symtab.insert(sym); }; return operate(op); diff --git a/src/base/loader/symtab.test.cc b/src/base/loader/symtab.test.cc index 1705a4165c..9a7b8c86b9 100644 --- a/src/base/loader/symtab.test.cc +++ b/src/base/loader/symtab.test.cc @@ -52,26 +52,26 @@ getSymbolError(const loader::Symbol& symbol, const loader::Symbol& expected) { std::stringstream ss; - if (symbol.binding != expected.binding) { + if (symbol.binding() != expected.binding()) { ss << " symbols' bindings do not match: seen `" << - (int)symbol.binding << "`, expected `" << - (int)expected.binding << "`.\n"; + (int)symbol.binding() << "`, expected `" << + (int)expected.binding() << "`.\n"; } - if (symbol.type != expected.type) { + if (symbol.type() != expected.type()) { ss << " symbols' types do not match: seen `" << - (int)symbol.type << "`, expected `" << - (int)expected.type << "`.\n"; + (int)symbol.type() << "`, expected `" << + (int)expected.type() << "`.\n"; } - if (symbol.name != expected.name) { - ss << " symbols' names do not match: seen `" << symbol.name << - "`, expected `" << expected.name << "`.\n"; + if (symbol.name() != expected.name()) { + ss << " symbols' names do not match: seen `" << symbol.name() << + "`, expected `" << expected.name() << "`.\n"; } - if (symbol.address != expected.address) { + if (symbol.address() != expected.address()) { ss << " symbols' addresses do not match: seen `" << - symbol.address << "`, expected `" << expected.address << "`.\n"; + symbol.address() << "`, expected `" << expected.address() << "`.\n"; } // No error, symbols match @@ -273,12 +273,12 @@ TEST(LoaderSymtabTest, Offset) // Check that the new table is offset loader::Symbol expected_symbols[] = { - {symbols[0].binding, symbols[0].type, symbols[0].name, - symbols[0].address + offset}, - {symbols[1].binding, symbols[1].type, symbols[1].name, - symbols[1].address + offset}, - {symbols[2].binding, symbols[2].type, symbols[2].name, - symbols[2].address + offset}, + {symbols[0].binding(), symbols[0].type(), symbols[0].name(), + symbols[0].address() + offset}, + {symbols[1].binding(), symbols[1].type(), symbols[1].name(), + symbols[1].address() + offset}, + {symbols[2].binding(), symbols[2].type(), symbols[2].name(), + symbols[2].address() + offset}, }; ASSERT_TRUE(checkTable(*symtab_new, {expected_symbols[0], expected_symbols[1], expected_symbols[2]})); @@ -316,14 +316,14 @@ TEST(LoaderSymtabTest, Mask) // Check that the new table is masked loader::Symbol expected_symbols[] = { - {symbols[0].binding, symbols[0].type, symbols[0].name, - symbols[0].address & mask}, - {symbols[1].binding, symbols[1].type, symbols[1].name, - symbols[1].address & mask}, - {symbols[2].binding, symbols[2].type, symbols[2].name, - symbols[2].address & mask}, - {symbols[3].binding, symbols[3].type, symbols[3].name, - symbols[3].address & mask}, + {symbols[0].binding(), symbols[0].type(), symbols[0].name(), + symbols[0].address() & mask}, + {symbols[1].binding(), symbols[1].type(), symbols[1].name(), + symbols[1].address() & mask}, + {symbols[2].binding(), symbols[2].type(), symbols[2].name(), + symbols[2].address() & mask}, + {symbols[3].binding(), symbols[3].type(), symbols[3].name(), + symbols[3].address() & mask}, }; ASSERT_TRUE(checkTable(*symtab_new, {expected_symbols[0], expected_symbols[1], expected_symbols[2], expected_symbols[3]})); @@ -353,7 +353,7 @@ TEST(LoaderSymtabTest, Rename) EXPECT_TRUE(symtab.insert(symbols[3])); const auto symtab_new = - symtab.rename([](std::string &name) { name = name + "_suffix"; }); + symtab.rename([](const std::string &name) { return name + "_suffix"; }); // Check that the original table is not modified ASSERT_TRUE(checkTable(symtab, {symbols[0], symbols[1], symbols[2], @@ -361,14 +361,14 @@ TEST(LoaderSymtabTest, Rename) // Check that the new table's symbols have been renamed loader::Symbol expected_symbols[] = { - {symbols[0].binding, symbols[0].type, symbols[0].name + "_suffix", - symbols[0].address}, - {symbols[1].binding, symbols[1].type, symbols[1].name + "_suffix", - symbols[1].address}, - {symbols[2].binding, symbols[2].type, symbols[2].name + "_suffix", - symbols[2].address}, - {symbols[3].binding, symbols[3].type, symbols[3].name + "_suffix", - symbols[3].address}, + {symbols[0].binding(), symbols[0].type(), symbols[0].name() + "_suffix", + symbols[0].address()}, + {symbols[1].binding(), symbols[1].type(), symbols[1].name() + "_suffix", + symbols[1].address()}, + {symbols[2].binding(), symbols[2].type(), symbols[2].name() + "_suffix", + symbols[2].address()}, + {symbols[3].binding(), symbols[3].type(), symbols[3].name() + "_suffix", + symbols[3].address()}, }; ASSERT_TRUE(checkTable(*symtab_new, {expected_symbols[0], expected_symbols[1], expected_symbols[2], expected_symbols[3]})); @@ -398,10 +398,12 @@ TEST(LoaderSymtabTest, RenameNonUnique) EXPECT_TRUE(symtab.insert(symbols[3])); int i = 0; - const auto symtab_new = symtab.rename([&i](std::string &name) + const auto symtab_new = symtab.rename([&i](const std::string &name) { if ((i++ % 2) == 0) { - name = "NonUniqueName"; + return std::string("NonUniqueName"); + } else { + return name; } }); @@ -412,12 +414,12 @@ TEST(LoaderSymtabTest, RenameNonUnique) // Check that the new table's symbols have been renamed, yet it does not // contain the symbols with duplicated names loader::Symbol expected_symbols[] = { - {symbols[0].binding, symbols[0].type, "NonUniqueName", - symbols[0].address}, - {symbols[1].binding, symbols[1].type, symbols[1].name, - symbols[1].address}, - {symbols[3].binding, symbols[3].type, symbols[3].name, - symbols[3].address}, + {symbols[0].binding(), symbols[0].type(), "NonUniqueName", + symbols[0].address()}, + {symbols[1].binding(), symbols[1].type(), symbols[1].name(), + symbols[1].address()}, + {symbols[3].binding(), symbols[3].type(), symbols[3].name(), + symbols[3].address()}, }; ASSERT_TRUE(checkTable(*symtab_new, {expected_symbols[0], expected_symbols[1], expected_symbols[2]})); @@ -597,7 +599,7 @@ TEST(LoaderSymtabTest, FindUniqueAddress) EXPECT_TRUE(symtab.insert(symbols[1])); EXPECT_TRUE(symtab.insert(symbols[2])); - const auto it = symtab.find(symbols[2].address); + const auto it = symtab.find(symbols[2].address()); ASSERT_NE(it, symtab.end()); ASSERT_PRED_FORMAT2(checkSymbol, *it, symbols[2]); } @@ -622,7 +624,7 @@ TEST(LoaderSymtabTest, FindNonUniqueAddress) EXPECT_TRUE(symtab.insert(symbols[1])); EXPECT_TRUE(symtab.insert(symbols[2])); - const auto it = symtab.find(symbols[1].address); + const auto it = symtab.find(symbols[1].address()); ASSERT_NE(it, symtab.end()); ASSERT_PRED_FORMAT2(checkSymbol, *it, symbols[1]); } @@ -658,7 +660,7 @@ TEST(LoaderSymtabTest, FindExistingName) EXPECT_TRUE(symtab.insert(symbols[1])); EXPECT_TRUE(symtab.insert(symbols[2])); - const auto it = symtab.find(symbols[1].name); + const auto it = symtab.find(symbols[1].name()); ASSERT_NE(it, symtab.end()); ASSERT_PRED_FORMAT2(checkSymbol, *it, symbols[1]); } @@ -677,7 +679,7 @@ TEST(LoaderSymtabTest, FindNearestExact) EXPECT_TRUE(symtab.insert(symbols[0])); EXPECT_TRUE(symtab.insert(symbols[1])); - const auto it = symtab.findNearest(symbols[1].address); + const auto it = symtab.findNearest(symbols[1].address()); ASSERT_NE(it, symtab.end()); ASSERT_PRED_FORMAT2(checkSymbol, *it, symbols[1]); } @@ -695,7 +697,7 @@ TEST(LoaderSymtabTest, FindNearestRound) "symbol", 0x10}; EXPECT_TRUE(symtab.insert(symbol)); - const auto it = symtab.findNearest(symbol.address + 0x1); + const auto it = symtab.findNearest(symbol.address() + 0x1); ASSERT_NE(it, symtab.end()); ASSERT_PRED_FORMAT2(checkSymbol, *it, symbol); } @@ -719,10 +721,10 @@ TEST(LoaderSymtabTest, FindNearestRoundWithNext) EXPECT_TRUE(symtab.insert(symbols[1])); Addr next_addr; - const auto it = symtab.findNearest(symbols[0].address + 0x1, next_addr); + const auto it = symtab.findNearest(symbols[0].address() + 0x1, next_addr); ASSERT_NE(it, symtab.end()); ASSERT_PRED_FORMAT2(checkSymbol, *it, symbols[0]); - ASSERT_EQ(next_addr, symbols[1].address); + ASSERT_EQ(next_addr, symbols[1].address()); } /** @@ -740,7 +742,7 @@ TEST(LoaderSymtabTest, FindNearestRoundWithNextNonExistent) EXPECT_TRUE(symtab.insert(symbol)); Addr next_addr; - const auto it = symtab.findNearest(symbol.address + 0x1, next_addr); + const auto it = symtab.findNearest(symbol.address() + 0x1, next_addr); ASSERT_NE(it, symtab.end()); ASSERT_PRED_FORMAT2(checkSymbol, *it, symbol); ASSERT_EQ(next_addr, 0); @@ -759,7 +761,7 @@ TEST(LoaderSymtabTest, FindNearestNonExistent) "symbol", 0x10}; EXPECT_TRUE(symtab.insert(symbol)); - const auto it = symtab.findNearest(symbol.address - 0x1); + const auto it = symtab.findNearest(symbol.address() - 0x1); ASSERT_EQ(it, symtab.end()); } diff --git a/src/cpu/base.cc b/src/cpu/base.cc index bea21a1928..9e015f8c16 100644 --- a/src/cpu/base.cc +++ b/src/cpu/base.cc @@ -1,5 +1,5 @@ /* - * Copyright (c) 2011-2012,2016-2017, 2019-2020 ARM Limited + * Copyright (c) 2011-2012,2016-2017, 2019-2020 Arm Limited * All rights reserved * * The license below extends only to copyright in the software and shall @@ -795,8 +795,8 @@ BaseCPU::traceFunctionsInternal(Addr pc) currentFunctionStart = pc; currentFunctionEnd = pc + 1; } else { - sym_str = it->name; - currentFunctionStart = it->address; + sym_str = it->name(); + currentFunctionStart = it->address(); } ccprintf(*functionTraceStream, " (%d)\n%d: %s", diff --git a/src/cpu/exetrace.cc b/src/cpu/exetrace.cc index 6cd5269fa7..2be3557d4b 100644 --- a/src/cpu/exetrace.cc +++ b/src/cpu/exetrace.cc @@ -81,11 +81,11 @@ ExeTracerRecord::traceInst(const StaticInstPtr &inst, bool ran) if (debug::ExecSymbol && (!FullSystem || !in_user_mode) && (it = loader::debugSymbolTable.findNearest(cur_pc)) != loader::debugSymbolTable.end()) { - Addr delta = cur_pc - it->address; + Addr delta = cur_pc - it->address(); if (delta) - ccprintf(outs, " @%s+%d", it->name, delta); + ccprintf(outs, " @%s+%d", it->name(), delta); else - ccprintf(outs, " @%s", it->name); + ccprintf(outs, " @%s", it->name()); } if (inst->isMicroop()) { diff --git a/src/cpu/profile.cc b/src/cpu/profile.cc index 25d739c381..06c8d46bc2 100644 --- a/src/cpu/profile.cc +++ b/src/cpu/profile.cc @@ -63,7 +63,7 @@ BaseStackTrace::tryGetSymbol(std::string &symbol, Addr addr, const auto it = symtab->find(addr); if (it == symtab->end()) return false; - symbol = it->name; + symbol = it->name(); return true; } @@ -151,7 +151,7 @@ FunctionProfile::sample(ProfileNode *node, Addr pc) auto it = symtab.findNearest(pc); if (it != symtab.end()) { - pc_count[it->address]++; + pc_count[it->address()]++; } else { // record PC even if we don't have a symbol to avoid // silently biasing the histogram diff --git a/src/kern/linux/helpers.cc b/src/kern/linux/helpers.cc index b024226985..4d1bb19542 100644 --- a/src/kern/linux/helpers.cc +++ b/src/kern/linux/helpers.cc @@ -1,5 +1,5 @@ /* - * Copyright (c) 2016, 2022 Arm Limited + * Copyright (c) 2016, 2022-2023 Arm Limited * All rights reserved * * The license below extends only to copyright in the software and shall @@ -125,9 +125,9 @@ dumpDmesg(ThreadContext *tc, std::ostream &os) return; } - uint32_t log_buf_len = proxy.read(lb_len->address, bo); - uint32_t log_first_idx = proxy.read(first->address, bo); - uint32_t log_next_idx = proxy.read(next->address, bo); + uint32_t log_buf_len = proxy.read(lb_len->address(), bo); + uint32_t log_first_idx = proxy.read(first->address(), bo); + uint32_t log_next_idx = proxy.read(next->address(), bo); if (log_first_idx >= log_buf_len || log_next_idx >= log_buf_len) { warn("dmesg pointers/length corrupted\n"); @@ -143,7 +143,7 @@ dumpDmesg(ThreadContext *tc, std::ostream &os) warn("Unexpected dmesg buffer length\n"); return; } - proxy.readBlob(lb->address + log_first_idx, log_buf.data(), length); + proxy.readBlob(lb->address() + log_first_idx, log_buf.data(), length); } else { const int length_2 = log_buf_len - log_first_idx; if (length_2 < 0 || length_2 + log_next_idx > log_buf.size()) { @@ -151,8 +151,10 @@ dumpDmesg(ThreadContext *tc, std::ostream &os) return; } length = log_buf_len; - proxy.readBlob(lb->address + log_first_idx, log_buf.data(), length_2); - proxy.readBlob(lb->address, log_buf.data() + length_2, log_next_idx); + proxy.readBlob( + lb->address() + log_first_idx, log_buf.data(), length_2); + proxy.readBlob( + lb->address(), log_buf.data() + length_2, log_next_idx); } // Print dmesg buffer content @@ -498,7 +500,8 @@ dumpDmesgImpl(ThreadContext *tc, std::ostream &os) ringbuffer_t dynamic_rb; auto dynamic_rb_symbol = symtab.find("printk_rb_dynamic"); if (dynamic_rb_symbol != symtab_end_it) { - dynamic_rb = ringbuffer_t::read(proxy, dynamic_rb_symbol->address, bo); + dynamic_rb = + ringbuffer_t::read(proxy, dynamic_rb_symbol->address(), bo); } else { warn("Failed to find required dmesg symbols.\n"); return; @@ -508,7 +511,7 @@ dumpDmesgImpl(ThreadContext *tc, std::ostream &os) ringbuffer_t static_rb; auto static_rb_symbol = symtab.find("printk_rb_static"); if (static_rb_symbol != symtab_end_it) { - static_rb = ringbuffer_t::read(proxy, static_rb_symbol->address, bo); + static_rb = ringbuffer_t::read(proxy, static_rb_symbol->address(), bo); } else { warn("Failed to find required dmesg symbols.\n"); return; @@ -521,21 +524,22 @@ dumpDmesgImpl(ThreadContext *tc, std::ostream &os) auto active_ringbuffer_ptr_symbol = symtab.find("prb"); if (active_ringbuffer_ptr_symbol != symtab_end_it) { active_ringbuffer_ptr = - proxy.read(active_ringbuffer_ptr_symbol->address, bo); + proxy.read(active_ringbuffer_ptr_symbol->address(), + bo); } else { warn("Failed to find required dmesg symbols.\n"); return; } if (active_ringbuffer_ptr == 0 || - (active_ringbuffer_ptr != dynamic_rb_symbol->address && - active_ringbuffer_ptr != static_rb_symbol->address)) { + (active_ringbuffer_ptr != dynamic_rb_symbol->address() && + active_ringbuffer_ptr != static_rb_symbol->address())) { warn("Kernel Dmesg ringbuffer appears to be invalid.\n"); return; } ringbuffer_t & rb = - (active_ringbuffer_ptr == dynamic_rb_symbol->address) + (active_ringbuffer_ptr == dynamic_rb_symbol->address()) ? dynamic_rb : static_rb; atomic_var_t head_offset = rb.data.head_offset; diff --git a/src/sim/kernel_workload.cc b/src/sim/kernel_workload.cc index c338b23b16..27e27137a9 100644 --- a/src/sim/kernel_workload.cc +++ b/src/sim/kernel_workload.cc @@ -66,8 +66,8 @@ KernelWorkload::KernelWorkload(const Params &p) : Workload(p), kernelSymtab = kernelObj->symtab(); auto initKernelSymtab = kernelSymtab.mask(_loadAddrMask) ->offset(_loadAddrOffset) - ->rename([](std::string &name) { - name = "kernel_init." + name; + ->rename([](const std::string &name) { + return "kernel_init." + name; }); loader::debugSymbolTable.insert(*initKernelSymtab); diff --git a/src/sim/workload.hh b/src/sim/workload.hh index 10129379e0..727a283fd4 100644 --- a/src/sim/workload.hh +++ b/src/sim/workload.hh @@ -141,7 +141,7 @@ class Workload : public SimObject if (it == symtab.end()) return nullptr; - return new T(system, desc, fixFuncEventAddr(it->address), + return new T(system, desc, fixFuncEventAddr(it->address()), std::forward(args)...); } From 7ecff99c25d3e07cbcb764e466fdff4d69733e11 Mon Sep 17 00:00:00 2001 From: Richard Cooper Date: Thu, 30 Nov 2023 12:11:28 +0000 Subject: [PATCH 2/4] base: Add a size field to the Symbol object Add a size field to the Symbol objects in the Symbol Table. This allows client code to read the data associated with a symbol in cases where the data type/size is not known beforehand (e.g. if an object's size might be different between different versions of a workload/kernel). Access is mediated via the `sizeOrDefault()` method, which requires client code to specify a fallback size. Since correct size data may not be available (for example in legacy checkpoints), this forces the client code to consider the 'missing size data' case. Change-Id: If1a47463790b25beadf94f84382e3b7392ab2f04 Reviewed-by: Andreas Sandberg --- src/base/loader/elf_object.cc | 3 ++- src/base/loader/symtab.cc | 15 ++++++++++++++- src/base/loader/symtab.hh | 30 +++++++++++++++++++++++++++++- 3 files changed, 45 insertions(+), 3 deletions(-) diff --git a/src/base/loader/elf_object.cc b/src/base/loader/elf_object.cc index 5b2cda6894..26fb443c79 100644 --- a/src/base/loader/elf_object.cc +++ b/src/base/loader/elf_object.cc @@ -218,7 +218,8 @@ ElfObject::ElfObject(ImageFileDataPtr ifd) : ObjectFile(ifd) } loader::Symbol symbol( - binding, symbol_type, sym_name, sym.st_value); + binding, symbol_type, sym_name, sym.st_value, + sym.st_size); if (_symtab.insert(symbol)) { DPRINTF(Loader, "Symbol: %-40s value %#x.\n", diff --git a/src/base/loader/symtab.cc b/src/base/loader/symtab.cc index eb63ccbff6..cea79c4cae 100644 --- a/src/base/loader/symtab.cc +++ b/src/base/loader/symtab.cc @@ -111,6 +111,10 @@ SymbolTable::serialize(const std::string &base, CheckpointOut &cp) const int i = 0; for (auto &symbol: symbols) { paramOut(cp, csprintf("%s.addr_%d", base, i), symbol.address()); + if (symbol.sizeIsValid()) { + paramOut(cp, csprintf("%s.size_%d", base, i), + symbol.sizeOrDefault(0x0)); + } paramOut(cp, csprintf("%s.symbol_%d", base, i), symbol.name()); paramOut(cp, csprintf("%s.binding_%d", base, i), (int)symbol.binding()); @@ -128,17 +132,26 @@ SymbolTable::unserialize(const std::string &base, CheckpointIn &cp, paramIn(cp, base + ".size", size); for (int i = 0; i < size; ++i) { Addr address; + size_t size; std::string name; Symbol::Binding binding = default_binding; Symbol::SymbolType type = Symbol::SymbolType::Other; paramIn(cp, csprintf("%s.addr_%d", base, i), address); + bool size_present = optParamIn( + cp, csprintf("%s.size_%d", base, i), size, false); paramIn(cp, csprintf("%s.symbol_%d", base, i), name); if (!optParamIn(cp, csprintf("%s.binding_%d", base, i), binding)) binding = default_binding; if (!optParamIn(cp, csprintf("%s.type_%d", base, i), type)) type = Symbol::SymbolType::Other; - insert(Symbol(binding, type, name, address)); + if (size_present) { + insert(Symbol(binding, type, name, address, size)); + } else { + warn_once( + "warning: one or more Symbols does not have a valid size."); + insert(Symbol(binding, type, name, address)); + } } } diff --git a/src/base/loader/symtab.hh b/src/base/loader/symtab.hh index 953ca39889..d33ed85f63 100644 --- a/src/base/loader/symtab.hh +++ b/src/base/loader/symtab.hh @@ -80,9 +80,16 @@ class Symbol Other }; + Symbol(const Binding binding, const SymbolType type, + const std::string & name, const Addr addr, const size_t size) + : _binding(binding), _type(type), _name(name), _address(addr), + _size(size), _sizeIsValid(true) + {} + Symbol(const Binding binding, const SymbolType type, const std::string & name, const Addr addr) - : _binding(binding), _type(type), _name(name), _address(addr) + : _binding(binding), _type(type), _name(name), _address(addr), + _size(0x0), _sizeIsValid(false) {} Symbol(const Symbol & other) = default; @@ -112,11 +119,32 @@ class Symbol _address = new_addr; } + /** + * Return the Symbol size if it is valid, otherwise return the + * default value supplied. + * + * This method forces the client code to consider the possibility + * that the `SymbolTable` may contain `Symbol`s that do not have + * valid sizes. + */ + size_t sizeOrDefault(const size_t default_size) const { + return _sizeIsValid ? _size : default_size; + } + + /** + * Return whether the Symbol size is valid or not. + */ + bool sizeIsValid() const { + return _sizeIsValid; + } + private: Binding _binding; SymbolType _type; std::string _name; Addr _address; + size_t _size; + bool _sizeIsValid; }; From f21df19fd7204f5b8a12d96adfef89fd9d8738e7 Mon Sep 17 00:00:00 2001 From: Richard Cooper Date: Wed, 22 Nov 2023 08:50:11 +0000 Subject: [PATCH 3/4] misc: Add function to extract the Linux Kernel Version This function, `extract_kernel_version`, attempts to find the Kernel version by searching the `uts_namespace` struct at the exported symbol `init_uts_ns`. This structure contains the Kernel version as a string, and is referenced by `procfs` to return the Kernel version in a running system. Different versions of the Kernel use different layouts for this structure, and the top level structure is marked as `__randomize_layout`, so the exact layout in memory cannot be relied on. Because of this, `extract_kernel_version` takes the approach of searching the memory holding the structure for printable strings, and parsing the version from those strings. Change-Id: If8b2e81a041af891fd6e56a87346a341df3c9728 Reviewed-by: Andreas Sandberg --- src/kern/linux/helpers.cc | 100 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+) diff --git a/src/kern/linux/helpers.cc b/src/kern/linux/helpers.cc index 4d1bb19542..d153a75504 100644 --- a/src/kern/linux/helpers.cc +++ b/src/kern/linux/helpers.cc @@ -37,7 +37,10 @@ #include "kern/linux/helpers.hh" +#include +#include #include +#include #include "base/compiler.hh" #include "base/loader/object_file.hh" @@ -585,6 +588,103 @@ dumpDmesgImpl(ThreadContext *tc, std::ostream &os) } } +/** Extract all null-terminated printable strings from a buffer and + * return them as a vector. + * + */ +std::vector +extract_printable_strings(const std::vector buffer) +{ + std::vector results; + std::string result; + bool reading_printable = false; + for (const uint8_t byte: buffer) { + if (std::isprint(byte)) { + result += static_cast(byte); + reading_printable = true; + } else if (reading_printable) { + if (byte == '\0') { + results.push_back(result); + } + result.clear(); + reading_printable = false; + } + } + return results; +} + +/** Try to extract the Linux Kernel Version. + * + * This function attempts to find the Kernel version by searching the + * `uts_namespace` struct at the exported symbol `init_uts_ns`. This + * structure contains the Kernel version as a string, and is + * referenced by `procfs` to return the Kernel version in a running + * system. + * + * Different versions of the Kernel use different layouts for this + * structure, and the top level structure is marked as + * `__randomize_layout`, so the exact layout in memory cannot be + * relied on. Because of this, `extract_kernel_version` takes the + * approach of searching the memory holding the structure for + * printable strings, and parsing the version from those strings. + * + * If a likely match is found, the version is packed into a uint32_t + * in the standard format used by the Linux kernel (which allows + * numerical comparison of version numbers) and returned. + * + * If no likely match is found, 0x0 is returned to indicate an error. + * + */ +[[maybe_unused]] +uint32_t +extract_kernel_version(ThreadContext* tc) { + System *system = tc->getSystemPtr(); + const auto &symtab = system->workload->symtab(tc); + auto symtab_end_it = symtab.end(); + + auto symbol = symtab.find("init_uts_ns"); + if (symbol == symtab_end_it) { + return 0x0; + } + + // Use size of `init_uts_ns` in Linux v5.18.0 as a default. + // (e.g. for upgraded checkpoints.) + const size_t INIT_UTS_NS_SIZE_DEFAULT = 432; + const size_t BUFFER_SIZE = + symbol->sizeOrDefault(INIT_UTS_NS_SIZE_DEFAULT); + + TranslatingPortProxy proxy(tc); + std::vector buffer(BUFFER_SIZE); + proxy.readBlob( + symbol->address(), buffer.data(), buffer.size() * sizeof(uint8_t)); + auto strings = extract_printable_strings(buffer); + + const std::regex version_re {"^(\\d+)\\.(\\d+)\\.(\\d)+$"}; + std::smatch match; + for (const auto& string: strings) { + if (std::regex_search(string, match, version_re)) { + try { + int major = std::stoi(match[1]); + int minor = std::stoi(match[2]); + int point = std::stoi(match[3]); + return ( + (major & 0xFF) << 16 + | (minor & 0xFF) << 8 + | std::min(point, 255)); + } + catch (const std::invalid_argument &) { + // This shouldn't be possible if the regex matched. + continue; + } + catch (const std::out_of_range &) { + continue; + } + } + } + + return 0x0; +} + /** Dump the kernel Dmesg ringbuffer for Linux versions post-v5.10. * * Delegates to an architecture specific template funtion instance. From ccb8b3096711bc9b371af371d399da7f47e4fe50 Mon Sep 17 00:00:00 2001 From: Richard Cooper Date: Wed, 22 Nov 2023 08:59:01 +0000 Subject: [PATCH 4/4] misc: Update Dmesg dump for changes to printk in Linux v5.18+. Linux v5.18+ changed the format of one of the data structures used to implement the printk ring buffer. This caused the gem5 feature to dump the printk messages on Kernel panic to fail for Kernels 5.18.0 and later. This patch updates the printk messages dump feature to support the new printk ring buffer format when Kernels 5.18 and later are detected. This patch addresses gem5 Issue #550: https://github.com/gem5/gem5/issues/550 Change-Id: I533d539eafb83c44eeb4b9fbbcdd9c172fd398e6 Reported-by: Hoa Nguyen Reviewed-by: Andreas Sandberg --- src/kern/linux/helpers.cc | 83 +++++++++++++++++++++++++++++++-------- 1 file changed, 67 insertions(+), 16 deletions(-) diff --git a/src/kern/linux/helpers.cc b/src/kern/linux/helpers.cc index d153a75504..97b363c864 100644 --- a/src/kern/linux/helpers.cc +++ b/src/kern/linux/helpers.cc @@ -266,6 +266,41 @@ struct GEM5_PACKED DmesgInfoRecord } }; +/** Metadata struct for Linux pre-v5.18.0 + * + */ +template +struct Metadata_Pre_v5_18_0 +{ + using atomic_var_t = AtomicVarType; + using guest_ptr_t = + typename std::make_unsigned_t; + unsigned int mask_bits; + guest_ptr_t metadata_ring_ptr; + guest_ptr_t info_ring_ptr; + atomic_var_t unused1; + atomic_var_t unused2; +}; + + +/** Metadata struct for Linux post-v5.18.0 + * + */ +template +struct Metadata_Post_v5_18_0 +{ + using atomic_var_t = AtomicVarType; + using guest_ptr_t = + typename std::make_unsigned_t; + unsigned int mask_bits; + guest_ptr_t metadata_ring_ptr; + guest_ptr_t info_ring_ptr; + atomic_var_t unused1; + atomic_var_t unused2; + atomic_var_t unused3; +}; + + /** Top-level ringbuffer record for the Linux dmesg ringbuffer, post-v5.10. * * Struct data members are compatible with the equivalent Linux data @@ -276,7 +311,7 @@ struct GEM5_PACKED DmesgInfoRecord * the gem5 world, and reading/generating appropriate masks. * */ -template +template struct GEM5_PACKED DmesgRingbuffer { static_assert( @@ -291,14 +326,9 @@ struct GEM5_PACKED DmesgRingbuffer using metadata_record_t = DmesgMetadataRecord; // Struct data members - struct - { - unsigned int mask_bits; - guest_ptr_t metadata_ring_ptr; - guest_ptr_t info_ring_ptr; - atomic_var_t unused1; - atomic_var_t unused2; - } metadata; + + // Metadata struct size depends on the Linux Kernel Version + MetadataStructType metadata; struct { unsigned int mask_bits; @@ -391,9 +421,16 @@ struct GEM5_PACKED DmesgRingbuffer } }; -// Aliases for the two types of Ringbuffer that could be used. -using Linux64_Ringbuffer = DmesgRingbuffer; -using Linux32_Ringbuffer = DmesgRingbuffer; +// Aliases for the types of Ringbuffer that could be used. +using Linux64_Ringbuffer_Pre_v5_18_0 = + DmesgRingbuffer>; +using Linux32_Ringbuffer_Pre_v5_18_0 = + DmesgRingbuffer>; + +using Linux64_Ringbuffer_Post_v5_18_0 = + DmesgRingbuffer>; +using Linux32_Ringbuffer_Post_v5_18_0 = + DmesgRingbuffer>; /** Print the record at the specified offset into the data ringbuffer, * and return the offset of the next entry in the data ringbuffer, @@ -635,7 +672,6 @@ extract_printable_strings(const std::vector buffer) * If no likely match is found, 0x0 is returned to indicate an error. * */ -[[maybe_unused]] uint32_t extract_kernel_version(ThreadContext* tc) { System *system = tc->getSystemPtr(); @@ -695,11 +731,26 @@ dumpDmesg(ThreadContext *tc, std::ostream &os) { System *system = tc->getSystemPtr(); const bool os_is_64_bit = loader::archIs64Bit(system->workload->getArch()); + const uint32_t kernel_version = extract_kernel_version(tc); + const uint32_t KERNEL_5_18_0 = 0x00051200; - if (os_is_64_bit) { - dumpDmesgImpl(tc, os); + if (kernel_version == 0x0) { + warn("Could not determine Linux Kernel version. " + "Assuming post-v5.18.0\n"); + } + + if (kernel_version == 0x0 || kernel_version >= KERNEL_5_18_0) { + if (os_is_64_bit) { + dumpDmesgImpl(tc, os); + } else { + dumpDmesgImpl(tc, os); + } } else { - dumpDmesgImpl(tc, os); + if (os_is_64_bit) { + dumpDmesgImpl(tc, os); + } else { + dumpDmesgImpl(tc, os); + } } }