SCI: Additional crash context when ScummVM errors

[deckarep]

I looked at all the open SCI tickets and a common theme when a crash is reported is there is some back and forth on identifying the current room/script involved. Additionally knowing the exact platform and gameid is useful.

Would it be nice to have additional context logged out on any crash such as the room number, script# and backtrace along with gameid and OS/platform info?

Maybe having a text blob above it saying: please copy and paste the following information when reporting bugs to the ScummVM team to expedite any troubleshooting.

Also, this could be useful for other engines that crash.

[sluicebox]

I didn't realize we had a mechanism for engines to append text to error output: Engine::errorString. SCUMM and NANCY use this. SCI should too! Great idea! lol I would have done this already if I knew.

I'm going to have strong opinions on this one. I think it needs to be short and dense, because we don't want to clutter error output or ruin screenshots with text that scrolls away the real error. The more text, the less likely it's going to get communicated back to us. We also hit ERRORs all the time when developing, so the output shouldn't annoy us. And this code needs to be careful because it runs in a failure mode where any state could be corrupt; erroring when printing errors could be worse than doing nothing.

Here's what I'm thinking:

ERROR: currentErrorMessage (the line we have now)
Game: gameId [, Version: versionString]
Room: ###, Script: ###, Function: scriptOrKernelFunction(params if possible) @ pc

By "gameId" I mean the scummvm string, for example "kq5-fr". That string is short and dense with clues like localized language, CD vs floppy, and non-DOS platform. Also, some people typo the game name or forget to mention it.

Back traces are super useful, but they're long and unpredicable. I don't usually need an entire trace, it's just an efficient way to ask someone for a lot of information at once if I don't have anything else. I usually just need to know what room someone is in (harder than you'd think in some games) and the currently executing function.

I don't think we should include instructions; that takes up space, would annoy us (me), and users don't read things like that anyway. The others already provide everything up front anyway. This is about improving the quality of reports without requiring anyone to change their behavior; because they won't. If these two lines appeared by default in most tickets that would be a quality of life improvement, and we can escalate to asking for back traces (and almost certainly other questions) on the harder ones.

[deckarep]

Excellent! I'll spend some time trying to come up with something that is outlined here and is simple, PR hopefully in a day or two.

[deckarep]

@sluicebox - i have a rough cut of this code now working except for the (params if possible) part. My question is how are you envisioning this data formatted when its present?

Something like: methodFoo(0023:1123, 0034:3030, ...)?

[deckarep]

Hey @sluicebox, just thought I'd follow up again if you wouldn't mind giving some clarification around the arguments.

@bluegr was kind enough to merge what I had so far but there's still a TODO to handle args.

Hopefully I can button this up soon!

[sluicebox]

This was a great start, thanks! I'm excited for the higher quality bug reports it will generate.

The PR was merged into master too quickly, as errorString had many crashes, so I've been playing catch-up so that the daily builds no longer eat SCI error messages. We should be in good shape now, and in the process I've learned a bunch and found unrelated things to cleanup.

I added a comment about this but I'll repeat it here: it's critical that this function not crash or call error (infinite recursion / stack-overflow) or else we lose the original message and the debugger. This feature is inherently difficult because it has to be perfect, or else it's better to do nothing, and error can be called at any time. Complicating things is that a lot of this core engine code is 1990s-style C from FreeSCI, so there are uninitialized variables, out of date comments, all our favorites.

I played around with formatting options, and landed on one where all the info is condensed into a single-line header. That's what the error/errorString structure expects, and that's working well for the SCUMM engine. By limiting the context to the same line as the error message, this gives us the greatest chance of the info reaching us in a bug report. Put another way: the target audience for this text isn't the user who gets the error, it's us when it gets reported back. The formatting needs be terse, not pretty, so that it survives the trip. We can of course play with the formatting further as we gain experience.

I am now convinced that we don't need or want function parameters in the string: they take up too much space, their length is unpredictable, object addresses are meaningless out of context, and ultimately it's just not that important when you start with room,script,function,pc. If we still can't reproduce, we're already in "serious" territory, and will probably have follow-up questions anyway and can ask for a backtrace.

I think we can close this ticket, but I'll leave it open for now in case there's more someone wants to discuss.

Thanks for making this happen! It's a solid feature, and I can think of many tickets this little text string would have sped up.

[deckarep]

@sluicebox - please close the ticket. Thank you for all this additional information and circling back. Even though the initial commit sounds like it wasn't quite ready I'm glad that with some follow-up code it's more robust and paying dividends on capturing higher quality bug reports. Of course, that's the ultimate goal so I hope future bug reports can come about with better details and context.

I do agree that this function must be robust enough to capture faulted states in the engine but I blame it on my lack of C++ skills but I'm also happy I was able to contribute and learn some things along the way.

Thank you and @bluegr for helping to shepherd such changes and helping a n00b contribute to ScummVM. I really appreciate it.

I hope to help with ScummVM in the future as well.

[sluicebox]

Please do!

I can't believe that only two other engines use errorString! (SCUMM and NANCY.) It's been there since at least 2006!! I have begun a campaign to raise awareness.

This would not have gotten done anytime soon if it was just "someone please implement errorString", so thank you for getting the ball rolling and showing how it's done. This was good timing too, because things are kind of quiet in SCI at the moment, and I've been sniping code cleanups when I can, so I'm growing more intolerant of unreadable C-isms and bad comments. Looking at the core VM/engine code again to figure out the queries has added targets to my hit list...

Feature Request Complete! Closing with gusto!

[sluicebox]

"Weeee've Goooot One!!"

A speedrunner crashed Phantasmagoria 1 with crazy-fast clicks (that happens a lot!) and posted a screenshot to a Discord. Version, script/room#, object method, and pc. Wow.

[deckarep]

Super badass! Even having this little amount of debugger output should help to provide a direction. Excellent news and thanks for pinging this thread.