From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f66.google.com (mail-lf1-f66.google.com [209.85.167.66]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 1D2C045C304 for ; Tue, 15 Dec 2020 11:16:49 +0300 (MSK) Received: by mail-lf1-f66.google.com with SMTP id s26so2393832lfc.8 for ; Tue, 15 Dec 2020 00:16:49 -0800 (PST) Date: Tue, 15 Dec 2020 11:16:45 +0300 From: Cyrill Gorcunov Message-ID: <20201215081645.GB983198@grain> References: <20201210161832.729439-1-gorcunov@gmail.com> <20201210161832.729439-4-gorcunov@gmail.com> <2a91ad22-6cd3-0bdd-78ef-203bd4b48a8d@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2a91ad22-6cd3-0bdd-78ef-203bd4b48a8d@tarantool.org> 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: Vladislav Shpilevoy Cc: Mons Anderson , tml On Mon, Dec 14, 2020 at 11:54:23PM +0100, Vladislav Shpilevoy wrote: > > + > > +#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'? G(eneral). > > +#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. I thought about it, but you know on other OSes there might be different names for these registers (greg name comes from inside of the kernel) so I stick to OS specifics to be more clear. > > +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. The other members are exported to report while these two are printed in local console only. To be honest I don't see any reason in these two members but I kept them to not break backward compatibility. > > +#ifdef ENABLE_BACKTRACE > > + /* > > 4. We usually use /** in out-of-function comment's > first line. This comment is not a part of doxygen, I left it this way intentionally. This comment for internal use. > > > + * 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? Because the buffer is used between the calls, iow it filled once and then passed to plain report to the console and then to json encoding. And keeping data in static buffer between the calls is very bad idea, it bounds calls to the context. I'm ready to spend 4K per instance for this. We can shrink the value down to 1K if you prefer but keeping static buffer between the calls definitely is not an option. > 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. Vlad, I remember this. The problem is that if I would do interdiff the result will be simply unreadable (believe me, I tried). This is why I sent the whole new patch instead. I reworked the patch too much. > Also print_backtrace() becomes unused after your patch. Not really [cyrill@grain tarantool.git] git grep -n print_backtrace src/lib/core/backtrace.cc:436:print_backtrace(void) src/lib/core/backtrace.cc:449: print_backtrace(); src/lib/core/backtrace.h:46:void print_backtrace(void); src/lua/init.c:367: print_backtrace(); It is still suitable own handler, no?