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

[Cyrill Gorcunov <gorcunov@gmail.com>]

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?