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

Add: List recently executed commands in crashlog output. #7366

Closed

Conversation

JGRennison
Copy link
Contributor

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.

@nielsmh
Copy link
Contributor

nielsmh commented Mar 11, 2019

I don't see any way for the CLEF_CMD_FAILED or CLEF_GENERATING_WORLD flags to become set?

@Eddi-z
Copy link
Contributor

Eddi-z commented Mar 11, 2019

Could this leak passwords or other personal data?

@nielsmh
Copy link
Contributor

nielsmh commented Mar 11, 2019

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.

@JGRennison
Copy link
Contributor Author

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.

@nielsmh nielsmh added the backport requested This PR should be backport to current release (RC / stable) label Mar 11, 2019
CLEF_MY_CMD = 0x20, ///< locally generated command
};
DECLARE_ENUM_AS_BIT_SET(CommandLogEntryFlagEnum)
typedef SimpleTinyEnumT<CommandLogEntryFlagEnum, byte> CommandLogEntryFlag;
Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Contributor

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

@nielsmh
Copy link
Contributor

nielsmh commented Mar 15, 2019

@JGRennison How useful is this actually in resolving issues? Any examples where this has been the primary lead?

@LordAro LordAro removed the backport requested This PR should be backport to current release (RC / stable) label Mar 22, 2019
@TrueBrain
Copy link
Member

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.

@JGRennison
Copy link
Contributor Author

@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.
Conceptually it is similar to the recent news section, though I've been unable to find a use for that so far.

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.

@PeterN
Copy link
Member

PeterN commented Apr 2, 2019

Is there a way to make this log AI commands as well?

@glx22
Copy link
Contributor

glx22 commented Apr 2, 2019

AI commands are as human commands usually, so they are probably already included in the log.

src/command.cpp Outdated Show resolved Hide resolved
@PeterN
Copy link
Member

PeterN commented Apr 2, 2019

@glx I thought so but my crashlog only contained AI startup control commands

@PeterN
Copy link
Member

PeterN commented Apr 3, 2019

Ok, AI uses DoCommandPInternal() directly, so bypasses the logging in DoCommandP()

PeterN
PeterN previously requested changes Apr 3, 2019
src/command.cpp Outdated Show resolved Hide resolved
Maintain a circular buffer of info on recent commands.
@JGRennison
Copy link
Contributor Author

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.
As DoCommandPInternal has multiple return paths, the early error paths where the test command is never executed aren't logged any more, and the logging takes place after the test command is done, before any local command execution with DC_EXEC.
Is this reasonable, or should the logging entry be made/updated after any DC_EXEC execution (in the edge case where the results differ)?
DoCommandP's only_sending flags isn't saved any more, but that is probably no great loss.

@TrueBrain TrueBrain dismissed PeterN’s stale review April 13, 2019 06:31

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;
Copy link
Contributor

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

@LordAro
Copy link
Member

LordAro commented Aug 17, 2019

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

@nielsmh nielsmh closed this Sep 28, 2019
@JGRennison JGRennison deleted the crashlog-recent-commands branch January 9, 2024 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants