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

Change: add a timestamp in name of crash files #9761

Merged
merged 1 commit into from Feb 2, 2022

Conversation

glx22
Copy link
Contributor

@glx22 glx22 commented Dec 22, 2021

Motivation / Problem

When a crash happens, it overwrites previous crash files.
But different crash may happen before a player decides to open an issue, and they can have very different causes.

Description

Add a timestamp to the filenames, so newer crashes don't overwrite maybe useful data from previous crashes.

Limitations

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@LordAro
Copy link
Member

LordAro commented Dec 22, 2021

I think there's a windows error message referencing crash.log somewhere as well

Also, the issue template :)

@glx22
Copy link
Contributor Author

glx22 commented Dec 22, 2021

Windows crash dialog says something about crash.dmp and lists generated files with their real fullpath.
I could add <timestamp> in all crash.xxx in issue template, but I don't think it's necessary.

src/crashlog.cpp Outdated
* @param filename The begin where to write at.
* @param filename_last The last position in the buffer to write to.
* @param ext The extension for the filename.
* @param with_dir Wether to append directory in front of filename.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/wether/whether/
Maybe something like "Whether to suffix the file name with the directory"? Not sure whether "append ... in front" or "suffix" would be the prefered way to write it in English though.

static std::string crashname;

if (crashname.empty()) {
UTCTime::Format(filename, filename_last, "crash%Y%m%d%H%M%S");
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder how likely it is for this to trigger an allocation that OOMs the crashlog when its being generated. I seem to remember we were fairly wary about memory allocation before at least the crash log and dump were written. Not sure how big a problem it is, because it's a very small amount of memory that gets allocated though.

Copy link
Contributor

Choose a reason for hiding this comment

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

On POSIX platforms at least, strictly speaking you should not call malloc or similar functions from a signal handler (including the crash log handler) because these are not re-entrant or signal-safe. OOM is not really an issue because if it happens it's more likely that the OOM killer will kill the whole process outright anyway. In the vast majority of cases you can get away with using malloc, and indeed the backtrace call implementations in the Unix crash log handler call malloc.
On Windows both writing the crash log and showing the dialog with the crash log contents in will allocate vastly more memory than this, so if an OOM event occurs any crash log will never make it to the user.

This string content here is short enough that it'll probably fit into the inline buffer anyway.

@LordAro LordAro added the backport requested This PR should be backport to current release (RC / stable) label Jan 5, 2022
@michicc michicc merged commit b6c5f49 into OpenTTD:master Feb 2, 2022
@glx22 glx22 deleted the timestamp_crash branch February 2, 2022 22:21
@TrueBrain TrueBrain added backported This PR is backported to a current release (RC / stable) and removed backport requested This PR should be backport to current release (RC / stable) labels Apr 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported This PR is backported to a current release (RC / stable)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants