Skip to content

debugger: symlist usability + symbol table extensibility #13694

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 20, 2025
Merged

debugger: symlist usability + symbol table extensibility #13694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 59 additions & 37 deletions src/emu/debug/debugcmd.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3909,58 +3909,80 @@ void debugger_commands::execute_memdump(const std::vector<std::string_view> &par

void debugger_commands::execute_symlist(const std::vector<std::string_view> &params)
{
const char *namelist[1000];
// Default to CPU "0" if none specified
const char * cpuname = (params.empty()) ? "0" : params[0].cbegin();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes no sense at all. Why wouldn’t you default to the CPU that’s currently focused in the debugger rather than the first CPU? That’s what all the other commands do (bpset, wpset, etc.). Always defaulting to the first CPU a very poor choice.

Also, as I’ve said elsewhere, the purpose a symbol table is being used for is not a characteristic of the symbol table, and shouldn’t be a property of the symbol table. Requiring a model object to know all the situations in which it will be used is broken design.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cuavas: Sorry about that CPU thing, and thanks for catching it. I opened a PR to fix (#13886).

Regarding the symbol table type enum, I mentioned a follow-up suggestion on #13733, but hadn't heard back yet. The gist was to allow the symlist command to provide helpful category headers in its output as future symbol tables get added to the parent chain, such as local and global variables from the source code for source-level debugging. The goal is to categorize what the symbol table contains, but not exactly where or how it's used.

This allows symlist to be more data-driven, which leads to simple code even when there are 4 symbol tables chained together...

	// Traverse symbol_table parent chain, printing each table's symbols in its own block
	for (; symtable != nullptr; symtable = symtable->parent())
	{
		switch (symtable->type())
		{
			// Print symbol table heading like "**** CPU '%s' symbols ****"
			// or "**** Source-level local variables ****", etc.
		}

		// Now print contents of symbol table
	}

Alternatively, if symbol tables cannot know anything about their contents, then rather than looping through the symbol table parent chain, symlist would need to be hard-coded to obtain each symbol table from a known accessor / owner object so it could reliably print the descriptive header before it. This would require ongoing maintenance to the symlist code if new symbol tables are added to the parent chain in the future. I was hoping to avoid this, but if you think it's cleaner this way, I can do it.

What do you think?

device_t *cpu = nullptr;
symbol_table *symtable;
int count = 0;

if (!params.empty())
if (!m_console.validate_cpu_parameter(cpuname, cpu))
{
// validate parameters
device_t *cpu;
if (!m_console.validate_cpu_parameter(params[0], cpu))
return;
symtable = &cpu->debug()->symtable();
m_console.printf("CPU '%s' symbols:\n", cpu->tag());
if (!params.empty())
return; // Explicitly specified cpu is invalid

// Somehow cpu "0" is invalid, so just stick with global symbol table
symtable = &m_machine.debugger().cpu().global_symtable();
}
else
{
symtable = &m_machine.debugger().cpu().global_symtable();
m_console.printf("Global symbols:\n");
symtable = &cpu->debug()->symtable();
}

// gather names for all symbols
for (auto &entry : symtable->entries())
// Traverse symbol_table parent chain, printing each table's symbols in its own block
for (; symtable != nullptr; symtable = symtable->parent())
{
// only display "register" type symbols
if (!entry.second->is_function())
// Skip globals if user explicitly requested CPU
if (symtable->type() == symbol_table::BUILTIN_GLOBALS && !params.empty())
continue;

if (symtable->entries().size() == 0)
continue;

std::vector<const char *> namelist;

// Print heading for table
switch (symtable->type())
{
namelist[count++] = entry.second->name();
if (count >= std::size(namelist))
break;
case symbol_table::CPU_STATE:
m_console.printf("\n**** CPU '%s' symbols ****\n", cpu->tag());
break;
case symbol_table::BUILTIN_GLOBALS:
m_console.printf("\n**** Global symbols ****\n");
break;
default:
assert (!"Unrecognized symbol table type");
}
}

// sort the symbols
if (count > 1)
{
// gather names for all relevant symbols
for (auto &entry : symtable->entries())
{
// ignore built-in function symbols
if (!entry.second->is_function())
{
namelist.push_back(entry.second->name());
}
}

// sort the symbols
std::sort(
&namelist[0],
&namelist[count],
namelist.begin(),
namelist.end(),
[] (const char *item1, const char *item2) { return strcmp(item1, item2) < 0; });
}

// iterate over symbols and print out relevant ones
for (int symnum = 0; symnum < count; symnum++)
// iterate over symbols and print them
for (const char * symname : namelist)
{
symbol_entry const *const entry = symtable->find(symname);
assert(entry != nullptr);
m_console.printf("%s = %X", symname, entry->value());
if (!entry->is_lval())
m_console.printf(" (read-only)");
m_console.printf("\n");
}
}
if (params.empty())
{
symbol_entry const *const entry = symtable->find(namelist[symnum]);
assert(entry != nullptr);
u64 value = entry->value();

// only display "register" type symbols
m_console.printf("%s = %X", namelist[symnum], value);
if (!entry->is_lval())
m_console.printf(" (read-only)");
m_console.printf("\n");
m_console.printf(
"\nTo view the symbols for a particular CPU, try symlist <cpu>,\n"
"where <cpu> is the ID number or tag for a CPU.\n");
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/emu/debug/debugcpu.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ debugger_cpu::debugger_cpu(running_machine &machine)
m_tempvar = make_unique_clear<u64[]>(NUM_TEMP_VARIABLES);

/* create a global symbol table */
m_symtable = std::make_unique<symbol_table>(machine);
m_symtable = std::make_unique<symbol_table>(machine, symbol_table::BUILTIN_GLOBALS);
m_symtable->set_memory_modified_func([this]() { set_memory_modified(true); });

/* add "wpaddr", "wpdata", "wpsize" to the global symbol table */
Expand Down Expand Up @@ -488,7 +488,7 @@ device_debug::device_debug(device_t &device)
, m_state(nullptr)
, m_disasm(nullptr)
, m_flags(0)
, m_symtable(std::make_unique<symbol_table>(device.machine(), &device.machine().debugger().cpu().global_symtable(), &device))
, m_symtable(std::make_unique<symbol_table>(device.machine(), symbol_table::CPU_STATE, &device.machine().debugger().cpu().global_symtable(), &device))
, m_stepaddr(0)
, m_stepsleft(0)
, m_delay_steps(0)
Expand Down
3 changes: 2 additions & 1 deletion src/emu/debug/express.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,8 +363,9 @@ symbol_entry::~symbol_entry()
// symbol_table - constructor
//-------------------------------------------------

symbol_table::symbol_table(running_machine &machine, symbol_table *parent, device_t *device)
symbol_table::symbol_table(running_machine &machine, table_type type, symbol_table *parent, device_t *device)
: m_machine(machine)
, m_type(type)
, m_parent(parent)
, m_memintf(dynamic_cast<device_memory_interface *>(device))
, m_memory_modified(nullptr)
Expand Down
13 changes: 12 additions & 1 deletion src/emu/debug/express.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,11 +171,21 @@ class symbol_table
READ_WRITE
};

// Identifies the type of symbols stored in this table. These help symlist create
// useful output
enum table_type
{
CPU_STATE, // CPU registers, etc.
BUILTIN_GLOBALS, // Built-in MAME global symbols (e.g., beamx, beamy, frame, etc.)
// (also used for tables outside debugger: lua scripts, cheat engine)
};

// construction/destruction
symbol_table(running_machine &machine, symbol_table *parent = nullptr, device_t *device = nullptr);
symbol_table(running_machine &machine, table_type type, symbol_table *parent = nullptr, device_t *device = nullptr);

// getters
const std::unordered_map<std::string, std::unique_ptr<symbol_entry>> &entries() const { return m_symlist; }
table_type type() const { return m_type; }
symbol_table *parent() const { return m_parent; }
running_machine &machine() { return m_machine; }

Expand Down Expand Up @@ -212,6 +222,7 @@ class symbol_table

// internal state
running_machine & m_machine; // reference to the machine
table_type m_type; // kind of symbols stored in this table
symbol_table * m_parent; // pointer to the parent symbol table
std::unordered_map<std::string,std::unique_ptr<symbol_entry>> m_symlist; // list of symbols
device_memory_interface *const m_memintf; // pointer to the local memory interface (if any)
Expand Down
4 changes: 2 additions & 2 deletions src/frontend/mame/cheat.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -683,7 +683,7 @@ void cheat_script::script_entry::output_argument::save(util::core_file &cheatfil

cheat_entry::cheat_entry(cheat_manager &manager, symbol_table &globaltable, std::string const &filename, util::xml::data_node const &cheatnode)
: m_manager(manager)
, m_symbols(manager.machine(), &globaltable)
, m_symbols(manager.machine(), symbol_table::BUILTIN_GLOBALS, &globaltable)
, m_state(SCRIPT_STATE_OFF)
, m_numtemp(DEFAULT_TEMP_VARIABLES)
, m_argindex(0)
Expand Down Expand Up @@ -1065,7 +1065,7 @@ cheat_manager::cheat_manager(running_machine &machine)
, m_numlines(0)
, m_lastline(0)
, m_disabled(true)
, m_symtable(machine)
, m_symtable(machine, symbol_table::BUILTIN_GLOBALS)
{
// if the cheat engine is disabled, we're done
if (!machine.options().cheat())
Expand Down
2 changes: 1 addition & 1 deletion src/frontend/mame/luaengine_debug.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ class lua_engine::symbol_table_wrapper

symbol_table_wrapper(lua_engine &host, running_machine &machine, std::shared_ptr<symbol_table_wrapper> const &parent, device_t *device)
: m_host(host)
, m_table(machine, parent ? &parent->table() : nullptr, device)
, m_table(machine, symbol_table::BUILTIN_GLOBALS, parent ? &parent->table() : nullptr, device)
, m_parent(parent)
{
}
Expand Down
Loading