Skip to content
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

debugger: add nul-terminated string output to printf/logerror #12124

Merged
merged 1 commit into from
Mar 20, 2024

Conversation

pmackinlay
Copy link
Contributor

@pmackinlay pmackinlay commented Mar 11, 2024

  • support lower-case hexadecimal output
  • remove some format handling code obsoleted by strformat.h

This proposed change extends the debugger printf/logerror commands with a %s format specifier, allowing nul-terminated strings to be output from emulated system memory. I've found this incredibly handy for debugging Unix system calls like open() and execve(), and combined with exception points, allow this kind of tracing purely from the debugger instead of adding system dependent compile-time enabled debug code in the CPU.

As an example of usage, for my current ns32k system target, I'm using debugger commands like this: eps 5,w@(pc+1)==0x003b,{ logerror "0x%08x: execve(%s)\n", PC, :cpu, d@(r0); g; }. That is, when exception 5 ("supervisor call instruction") is triggered with system call number 0x3b, grab the first parameter from a dword in memory pointed at by register 0, and dump it as a nul-terminated string from the specified device address space.

Other than possibly adding a precision specifier to limit the maximum output length to minimize the impact of user error or unexpected data, I'm interested to hear any comments on this addition or its implementation.

@cuavas
Copy link
Member

cuavas commented Mar 11, 2024

Well, my first thought is that a format specifier that consumes two arguments isn’t particularly intuitive, and there’s the possibility that you mentioned of it running wild if it doesn’t find a NUL.

Changing %x to produce lowercase output is a functional change, but probably not a bad one to be honest. I know some people have complained about it not producing lowercase in the past.

Apart from that, the framework seems to make the implementation pretty straightforward at this point.

src/emu/debug/debugcmd.cpp Outdated Show resolved Hide resolved
@pmackinlay
Copy link
Contributor Author

Well, my first thought is that a format specifier that consumes two arguments isn’t particularly intuitive

Yeah, I agree, but couldn't think of a way to avoid it without making some assumptions that would no doubt prove to be a problem somewhere down the line. I considered an alternative of adding a string value and a new s@ expression to the expression system, but this seemed a lot more difficult.

and there’s the possibility that you mentioned of it running wild if it doesn’t find a NUL.

I have a local change to add a precision specifier (using C-style %width.precision format), but I'm considering an alternative of interpreting the width as the maximum string length (instead of minimum). In my opinion, being able to specify the maximum length is far more useful than the minimum, especially while there's currently no support for left-justification. Doing it this way violates "least surprise" (giving us two strikes against the intuitive approach), but on the other hand I don't see the precision specifier as useful for the integer formats. Comments?

logerror "%10s", ...
vs
logerror "%.10s", ...

Not having this option at all means it's a "programmer beware" issue, however I think there are ample opportunities to shoot yourself in the foot with the debugger in any case.

@pmackinlay
Copy link
Contributor Author

Well, my first thought is that a format specifier that consumes two arguments isn’t particularly intuitive

Yeah, I agree, but couldn't think of a way to avoid it ...

Of course there's a way and I didn't realise it was that simple. PR updated to use a single argument and to use the width as the maximum length of the string.

@cuavas
Copy link
Member

cuavas commented Mar 18, 2024

I think using the width as maximum length is just going to irritate people. The syntax is supposed to be printf-like to make it approachable. Making the width mean something completely different for this format specifier goes against the principle of least surprise.

Would it be easier to support left-justification now that the actual formatting is being outsourced?

@pmackinlay
Copy link
Contributor Author

I think using the width as maximum length is just going to irritate people. The syntax is supposed to be printf-like to make it approachable. Making the width mean something completely different for this format specifier goes against the principle of least surprise.

Yeah, I was also concerned about this. I'll take a stab at keeping it in line with printf and leveraging strformat.h more.

cuavas added a commit that referenced this pull request Mar 19, 2024
…error. [Patrick Mackinlay]

Also simplified implementation by better leveraging util/strformat.h.

This is from pull request #12124, to get some testing for the
fundamental change before freeze.
@cuavas
Copy link
Member

cuavas commented Mar 19, 2024

I cribbed the main implementation changes as well as lowercase hex support in 0524b10. I also updated the docs, and updated debughlp.cpp for other changes to printf/logerror that someone else didn’t bother to update it for. This should get us at least a little testing before freeze in case something broke.

@cuavas
Copy link
Member

cuavas commented Mar 19, 2024

I’ve resolved the conflicts in debughlp.cpp here – please either pull the PR branch again, or resolve the conflicts locally and force push.

@cuavas
Copy link
Member

cuavas commented Mar 19, 2024

Also, remember to update docs/source/debugger/general.rst when %s formatting is finalised.

* also added left-justification option for numeric and string formats
* removed duplication and made documentation more consistent
@pmackinlay
Copy link
Contributor Author

Updated the string format to allow both minimum and maximum field lengths (specified using the C-standard %x.ys syntax), and also support left justification for numeric and string formats with the - flag. Debugger and external documentation updated to include both changes.

@cuavas cuavas merged commit f93a040 into mamedev:master Mar 20, 2024
6 checks passed
@pmackinlay pmackinlay deleted the debugger branch March 21, 2024 02:30
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.

2 participants