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

JGRennison commented Mar 11, 2019

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

Eddi-z commented Mar 11, 2019

Could this leak passwords or other personal data?

@nielsmh

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

JGRennison commented Mar 11, 2019

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;

This comment has been minimized.

Copy link
@nielsmh

nielsmh Mar 12, 2019

Contributor

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.

This comment has been minimized.

Copy link
@PeterN

PeterN Mar 12, 2019

Member

I also wonder if we should start using enum classes.

This comment has been minimized.

Copy link
@nielsmh

nielsmh Apr 24, 2019

Contributor

Definitely change this to enum with underlying type ahead of #7538

@nielsmh

This comment has been minimized.

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?

@TrueBrain

This comment has been minimized.

Copy link
Member

TrueBrain commented Mar 22, 2019

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

This comment has been minimized.

Copy link
Contributor Author

JGRennison commented Mar 27, 2019

@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

This comment has been minimized.

Copy link
Member

PeterN commented Apr 2, 2019

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

@glx22

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

PeterN commented Apr 2, 2019

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

@PeterN

This comment has been minimized.

Copy link
Member

PeterN commented Apr 3, 2019

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

src/command.cpp Outdated Show resolved Hide resolved
Maintain a circular buffer of info on recent commands.
@JGRennison JGRennison force-pushed the JGRennison:crashlog-recent-commands branch from 1f92324 to 6925569 Apr 10, 2019
@JGRennison

This comment has been minimized.

Copy link
Contributor Author

JGRennison commented Apr 10, 2019

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 Apr 13, 2019

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;

This comment has been minimized.

Copy link
@nielsmh

nielsmh Apr 24, 2019

Contributor

Definitely change this to enum with underlying type ahead of #7538

@LordAro

This comment has been minimized.

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.