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.

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

Conversation

dave-br
Copy link
Contributor

@dave-br dave-br commented May 10, 2025

This addresses issue #6655 (symlist command usability), plus adds a bit of plumbing for future extensibility.

Symlist command usability

The distinction between global and CPU symbols is unclear to most who don't read the source, so this makes symlist more natural:

  • symlist with no arguments displays all global and :maincpu symbols, with clear header text for each list. At the bottom, prints helper text to make user aware of the cpu form
  • symlist <cpu> works as it has before, only displays symbols for specified CPU

Symbol table extensibility

This pr includes some changes in anticipation of features that add new kinds of symbol tables. Example: the source-level debugging pr (#13444) adds symbol tables for local and global variables that appear in the debugged source code. The following were originally part of the source-level debugging PR, so merging this will shrink that PR somewhat.

  • Add an enum field to symbol table for its 'type', for prettier printing from symlist.
  • symlist now traverses symbol table chain completely. No noticeable difference from before, but this will make extensions to the chain work automatically
  • Eliminate hard-coded limit (1000) on number of symbols to print per symbol table

Sample session

A CoCo with Speech-Sound cartridge (thus, two valid CPUs).

>symlist

**** CPU ':maincpu' symbols ****
a = 0
b = B
cc = 90
curflags = 90
curpc = 10C
cycles = 24  (read-only)
d = B
dp = 0
lastinstructioncycles = 18  (read-only)
logunmap = 1
pc = 10C
s = 7F1E
totalcycles = 24E077  (read-only)
u = ABEE
x = 402
y = A00E

**** Global symbols ****
beamx = 85  (read-only)
beamy = D9  (read-only)
cpunum = 0  (read-only)
frame = A1  (read-only)
temp0 = 0
temp1 = 0
temp2 = 0
temp3 = 0
temp4 = 0
temp5 = 0
temp6 = 0
temp7 = 0
temp8 = 0
temp9 = 0
wpaddr = 0  (read-only)
wpdata = 0  (read-only)
wpsize = 0  (read-only)

To view the symbols for a particular CPU, try symlist <cpu>,
where <cpu> is the ID number or tag for a CPU.


>symlist :maincpu

**** CPU ':maincpu' symbols ****
a = 0
b = B
cc = 90
curflags = 90
curpc = 10C
cycles = 24  (read-only)
d = B
dp = 0
lastinstructioncycles = 18  (read-only)
logunmap = 1
pc = 10C
s = 7F1E
totalcycles = 24E077  (read-only)
u = ABEE
x = 402
y = A00E


>symlist 0

**** CPU ':maincpu' symbols ****
a = 0
b = B
cc = 90
curflags = 90
curpc = 10C
cycles = 24  (read-only)
d = B
dp = 0
lastinstructioncycles = 18  (read-only)
logunmap = 1
pc = 10C
s = 7F1E
totalcycles = 24E077  (read-only)
u = ABEE
x = 402
y = A00E


>symlist 1

**** CPU ':ext:multi:slot3:ssc:pic7040' symbols ****
a = FF
b = 0
curflags = D0
curpc = F2D1
cycles = 0  (read-only)
genpc = F2D1
lastinstructioncycles = 9  (read-only)
logunmap = 1
pc = F2D1
sp = 4C
st = D0
totalcycles = 24E062  (read-only)


>symlist 2
Invalid CPU index 2

Address issue mamedev#6655 (symlist command usability), add a bit of plumbing for future extensibility.

symlist with no arguments displays all global *and* :maincpu symbols, with clear header text for each list.  At the bottom, prints helper text to make user aware of the cpu form

To allow for adding new kinds of symbols in the future, this adds an enum field to symbol table for its 'type', for prettier printing from symlist.  Symlist now traverses symbol table chain completely.
@dave-br
Copy link
Contributor Author

dave-br commented May 19, 2025

Just a gentle bump... merging this fixes an issue (i.e., improves symlist usability), and helps to shed some more code off of my larger source-debugging PR. Happy to answer any questions.

@galibert galibert merged commit b39e684 into mamedev:master May 20, 2025
6 checks passed
@@ -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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants