Re: [Tarantool-patches] [PATCH v4 3/4] crash: move fatal signal handling in

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

See 5 comments below.

On 10.12.2020 17:18, Cyrill Gorcunov wrote:
> When SIGSEGV or SIGFPE reaches the tarantool we try to gather
> all information related to the crash and print it out to the
> console (well, stderr actually). Still there is a request
> to not just show this info locally but send it out to the
> feedback server.
> 
> Thus to keep gathering crash related information in one module,
> we move fatal signal handling into the separate crash.c file.
> This allows us to collect the data we need in one place and
> reuse it when we need to send reports to stderr (and to the
> feedback server, which will be implemented in next patch).
> 
> Part-of #5261
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  src/lib/core/CMakeLists.txt |   1 +
>  src/lib/core/crash.c        | 291 ++++++++++++++++++++++++++++++++++++
>  src/lib/core/crash.h        |  32 ++++
>  src/main.cc                 | 138 +----------------
>  4 files changed, 329 insertions(+), 133 deletions(-)
>  create mode 100644 src/lib/core/crash.c
>  create mode 100644 src/lib/core/crash.h
> 
> diff --git a/src/lib/core/crash.c b/src/lib/core/crash.c
> new file mode 100644
> index 000000000..9572a023c
> --- /dev/null
> +++ b/src/lib/core/crash.c
> @@ -0,0 +1,291 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +
> +#include <string.h>
> +#include <unistd.h>
> +#include <time.h>
> +
> +#include "trivia/util.h"
> +#include "backtrace.h"
> +#include "crash.h"
> +#include "say.h"
> +
> +#define pr_fmt(fmt)		"crash: " fmt
> +#define pr_syserr(fmt, ...)	say_syserror(pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_panic(fmt, ...)	panic(pr_fmt(fmt), ##__VA_ARGS__)
> +
> +#ifdef TARGET_OS_LINUX
> +#ifndef __x86_64__
> +# error "Non x86-64 architectures are not supported"
> +#endif
> +struct crash_greg {

1. What is 'g' in 'greg'?

> +	uint64_t	r8;
> +	uint64_t	r9;
> +	uint64_t	r10;
> +	uint64_t	r11;
> +	uint64_t	r12;
> +	uint64_t	r13;
> +	uint64_t	r14;
> +	uint64_t	r15;
> +	uint64_t	di;
> +	uint64_t	si;
> +	uint64_t	bp;
> +	uint64_t	bx;
> +	uint64_t	dx;
> +	uint64_t	ax;
> +	uint64_t	cx;
> +	uint64_t	sp;
> +	uint64_t	ip;
> +	uint64_t	flags;
> +	uint16_t	cs;
> +	uint16_t	gs;
> +	uint16_t	fs;
> +	uint16_t	ss;
> +	uint64_t	err;
> +	uint64_t	trapno;
> +	uint64_t	oldmask;
> +	uint64_t	cr2;
> +	uint64_t	fpstate;
> +	uint64_t	reserved1[8];
> +};
> +#endif /* TARGET_OS_LINUX */

2. Perhaps you could reduce number of #ifdef-#endif
if you would define struct crash_greg as an empty
struct for all the other platforms. Then you wouldn't
need the TARGET_OS_LINUX check inside of crash_info.
But up to you.

> +
> +static struct crash_info {
> +	/**
> +	 * These two are mostly useless as being
> +	 * plain addresses but keep for backward
> +	 * compatibility.

3. Why don't you say the same about 'siaddr'? It is also
just a plain address.

> +	 */
> +	void *context_addr;
> +	void *siginfo_addr;
> +#ifdef TARGET_OS_LINUX
> +	/**
> +	 * Registers contents.
> +	 */
> +	struct crash_greg greg;
> +#endif
> +	/**
> +	 * Faulting address.
> +	 */
> +	void *siaddr;
> +	/**
> +	 * Crash signal number.
> +	 */
> +	int signo;
> +	/**
> +	 * Crash signal code.
> +	 */
> +	int sicode;
> +#ifdef ENABLE_BACKTRACE
> +	/*

4. We usually use /** in out-of-function comment's
first line.

> +	 * 4K of memory should be enough to keep the backtrace.
> +	 * In worst case it gonna be simply trimmed.
> +	 */
> +	char backtrace_buf[4096];

5. This is a functional change. Previously for the backtrace
we used the static buffer.

1) Why did you change it?

2) Why isn't it in a separate commit? As I told you, it is really
hard to extract what did you change in a ~460 lines patch titled as
'move' to check if it does not break anything or is even needed.
Please, don't make it harder.

Also print_backtrace() becomes unused after your patch.