From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 6C70F45C306 for ; Tue, 15 Dec 2020 01:54:25 +0300 (MSK) References: <20201210161832.729439-1-gorcunov@gmail.com> <20201210161832.729439-4-gorcunov@gmail.com> From: Vladislav Shpilevoy Message-ID: <2a91ad22-6cd3-0bdd-78ef-203bd4b48a8d@tarantool.org> Date: Mon, 14 Dec 2020 23:54:23 +0100 MIME-Version: 1.0 In-Reply-To: <20201210161832.729439-4-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v4 3/4] crash: move fatal signal handling in List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov , tml Cc: Mons Anderson 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 > --- > 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 > +#include > +#include > + > +#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.