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

SCI: adds support for debugging/executing kernel calls #5333

Merged
merged 1 commit into from Sep 29, 2023

Conversation

deckarep
Copy link
Contributor

@deckarep deckarep commented Sep 17, 2023

Introduces a new kerncall kernel/sub command to the debugger console only to aid in debugging all related SCI games. Similar in vein to using the send command.

Example within the SCI debug console:

) kerncall Random 4 8
kernel call result is: 0000:0008
  • Additionally, adds a few annotations and clarifications when you are viewing decimal/hex segment ids.
  • Lastly, to be more clear renames command: functions to kernfunctions to list all kernel routines.

@deckarep
Copy link
Contributor Author

Hi ScummVM devs, there’s certainly no rush here but I’m hoping this is a small enough, welcome enhancement to the debugging tools for SCI.

Please let me know if I can provide more context on why I thought to contribute this change. Also I welcome any comments to improve this code.

@sluicebox
Copy link
Member

Wow! Congratulations, this is a great and worthy new debug command. I will use it all the time. I think I've wished for this before?

  • Zero-argument calls should work; some functions don't have any, or they're optional. For example, kerncall GameIsRestarting should work. (That one is safe to call from anywhere, it returns a flag)
  • This is so useful that we'll get sick of typing kerncall every time, let's also add a kc alias. You've earned it!
  • kernfunctions is an okay name for consistency, but let's also keep the functions alias for those of us that have built up muscle memory. =)

The command appears to work great, so once the little things are done I'll merge it. @bluegr mentioned to me earlier that this looked good and useful. He's right!

If this weren't debug code, I'd ask for the cmdKernelFunctions cleanup to be moved to a separate commit to keep things hyper organized for posterity, but the bar is a little lower in console.cpp, so no need for more work.

@deckarep
Copy link
Contributor Author

Alright!

Excellent comments and it’s super appreciated. I will get on this today!

@deckarep
Copy link
Contributor Author

@sluicebox,

Please see this updated review with your comments addressed:

  • Aliases added for kerncall -> kc.
  • Aliases added for kernfunctions -> functions.
  • 0 argument kernel calls now supported. Good catch!

Lastly, I made sure to rebase and squash with the latest ScummVM code. Feel free to let me know if you'd like more tweaks.

Cheers!

engines/sci/console.cpp Outdated Show resolved Hide resolved
engines/sci/console.cpp Outdated Show resolved Hide resolved
@sluicebox
Copy link
Member

Looks great!

Sorry, but I failed to click the shiny Review button earlier, so my inline comments were invisible to you. You should be able to see them now.

@deckarep
Copy link
Contributor Author

@sluicebox - ok, for the 2nd round of comments this PR has been rebased against master and squashed.

@sluicebox
Copy link
Member

Excellent, thank you for your hard work. Looking forward to kc'ing all over the place. Merging with conviction!

@sluicebox sluicebox merged commit d71a009 into scummvm:master Sep 29, 2023
7 of 8 checks passed
@deckarep deckarep deleted the sci-debug-kernel-call-support branch September 29, 2023 04:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants