-
-
Notifications
You must be signed in to change notification settings - Fork 957
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
Add: List recently executed commands in crashlog output. #7366
Conversation
I don't see any way for the CLEF_CMD_FAILED or CLEF_GENERATING_WORLD flags to become set? |
Could this leak passwords or other personal data? |
Company passwords aren't game commands, but part of the network protocol itself. Server connection passwords aren't game commands either. The command log could contain any string entered for things like vehicle names, group names, signs, etc., i.e. nothing that wouldn't also be stored in the savegame. |
CLEF_CMD_FAILED and CLEF_GENERATING_WORLD are handled at src/command.cpp:398-399. This does not store text/strings, as these are rarely of any use for debugging. It only stores a flag indicating the presence of a non-empty text parameter. Passwords and (private) personal data should not go via DoCommandP calls as these are replicated to all clients in multiplayer. |
CLEF_MY_CMD = 0x20, ///< locally generated command | ||
}; | ||
DECLARE_ENUM_AS_BIT_SET(CommandLogEntryFlagEnum) | ||
typedef SimpleTinyEnumT<CommandLogEntryFlagEnum, byte> CommandLogEntryFlag; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think C++11 enum base type declarations are acceptable to use now, i.e. declaring enum: CommandLogEntryFlag : byte { CLEF_NONE ... };
so separate enum and storage types are not required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also wonder if we should start using enum classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely change this to enum with underlying type ahead of #7538
@JGRennison How useful is this actually in resolving issues? Any examples where this has been the primary lead? |
Removed "backport requested" as I think we should not add new code to RCs. This could potentially do more harm than good. Lets run this in 1.10, and bring it in from there. |
@nielsmh It's been quite useful in debugging desyncs and crashes/assertion failures involving map or data structure corruption. What commands were recently executed usually provides a big clue as to the cause of the issue. Generally when I receive a crash log I look at: the crash reason lines, scope logging lines, which thread it is, the decoded stack traces, which OS/compiler, AI/company config, the command log and if relevant the game date and lang, blitter, font config. The rest of it is rarely of any use. |
Is there a way to make this log AI commands as well? |
AI commands are as human commands usually, so they are probably already included in the log. |
@glx I thought so but my crashlog only contained AI startup control commands |
Ok, AI uses |
Maintain a circular buffer of info on recent commands.
1f92324
to
6925569
Compare
The flags are now all set in one place. I've moved the logging point to be within DoCommandPInternal, this does change the semantics a bit. |
No clue why GitHub didn't dismiss the review .. all is fixed as far as I can tell
CLEF_MY_CMD = 0x20, ///< locally generated command | ||
}; | ||
DECLARE_ENUM_AS_BIT_SET(CommandLogEntryFlagEnum) | ||
typedef SimpleTinyEnumT<CommandLogEntryFlagEnum, byte> CommandLogEntryFlag; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely change this to enum with underlying type ahead of #7538
Hi, there's been no activity on this for nearly 4 months. If this PR is not updated in the next week or so, it will be closed |
This maintains a circular buffer of info on recent commands, and adds a section to the crash log.
This is useful for providing context and further information of what was occurring shortly prior to a crash.