reactor: default to oneline backtraces in signal handlers

[fruch]

align the handlers with the same logic that is used in the reactor stalls backtrace prints

Ref: scylladb/scylladb#16922

[fruch]

I gave up trying to pass the reactor_options into the signal handlers

I think of possible two direction

1. pass way is via global variables
2. duplicate the handlers - multiline handlers, oneline handlers

[nyh]

Some of the CI runs failed because of compiler warnings on unused functions and such. Please look into these failures.

[nyh]

align the handlers with the same logic that is used in the reactor stalls backtrace prints

Ref: scylladb/scylladb#16922

I am missing some context about this patch:

1. What's wrong with multi-line backtraces, especially in the case of a one-off abort. I see some comments in the ScyllaDB issue about some ScyllaDB-specific testing tools that don't like the multi-line format, but it's not a very convincing reason.
2. Why did we reach the current situation that we have both formats? The commit that introduced the one-line backtrace for reactor stalls, 4006d78, by @bhalevy doesn't explain, or why he also wanted to keep the old format. I want to understand that the current mishmash - stalls printed in one format and crashes in another format - was never intentional.
3. Do our tools, especially Seastar's scripts/addr2line.py and @bhalevy 's backtracing website, support the one-line format? (maybe they already do, but it should be tested, and mentioned).

[fruch]

align the handlers with the same logic that is used in the reactor stalls backtrace prints

Ref: scylladb/scylladb#16922

I am missing some context about this patch:

1. What's wrong with multi-line backtraces, especially in the case of a one-off abort. I see some comments in the ScyllaDB issue about some ScyllaDB-specific testing tools that don't like the multi-line format, but it's not a very convincing reason.

It was agreed 4 years ago, you can see the rest of the discussion here:
scylladb/scylladb#5464

There was agreement almost from side to side that one line backtraces are better

And evens idea and suggestions on how to compress it even further

It's not that the tool doesn't like it, it adds complexity which is slowing those tools down, and they need to handle cases of interlaced backtraces as one example (yes it was relevert to stalls more the signal handlers), but again it is something that was agreed almost 4 years ago

2. Why did we reach the current situation that we have both formats? The commit that introduced the one-line backtrace for reactor stalls, 4006d78, by @bhalevy doesn't explain, or why he also wanted to keep the old format. I want to understand that the current mishmash - stalls printed in one format and crashes in another format - was never intentional.

I don't know if it was intentional or not, I think @bhalevy cared more about reactor stalls in the context of performance testing and analysis

3. Do our tools, especially Seastar's scripts/addr2line.py and @bhalevy 's backtracing website, support the one-line format? (maybe they already do, but it should be tested, and mentioned).

Again @bhalevy probably know the answer to this one, I assume the answer is yes they do.

[nyh]

The name of this option, "blocked_reactor_report_format_oneline", is no longer a very good name after this patch :-(

[fruch]

you think we should change the name ? or create a new flag ?

[nyh]

The common way to get rid of such duplicate functions in modern C++ is to use lambdas. I.e., have a single function

static void sigsegv_action(bool oneline) noexcept {
    print_with_backtrace("Segmentation fault", oneline);
    reraise_signal(SIGSEGV);
}

and then install it as:

install_oneshot_signal_handler<SIGSEGV, [oneline]() { sigsegv_action(oneline); }>();

(I didn't test this, I don't know why this install function is a template, by the way)

[fruch]

I went in few circle to figure how to do that, I'll give this option a try.

[fruch]

I've tried using this,

but the temple part isn't happy with it:

/home/fruch/projects/scylla/seastar/src/core/reactor.cc:4266:5: error: no matching function for call to 'install_oneshot_signal_handler'
    install_oneshot_signal_handler<SIGSEGV, [oneline]() { sigsegv_action(oneline); }>();
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/fruch/projects/scylla/seastar/src/core/reactor.cc:4029:6: note: candidate template ignored: invalid explicitly-specified argument for template parameter 'Func'
void install_oneshot_signal_handler() {
     ^
/home/fruch/projects/scylla/seastar/src/core/reactor.cc:4270:5: error: no matching function for call to 'install_oneshot_signal_handler'
    install_oneshot_signal_handler<SIGABRT, [oneline]() { sigabrt_action(oneline); }>();
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/fruch/projects/scylla/seastar/src/core/reactor.cc:4029:6: note: candidate template ignored: invalid explicitly-specified argument for template parameter 'Func'
void install_oneshot_signal_handler() {
     ^
2 errors generated.

[avikivity]

The lambda needs to go inside the parentheses.

[fruch]

like that ?

install_oneshot_signal_handler<SIGABRT, >([oneline]() { sigabrt_action(oneline); });

[avikivity]

Without random commas,

[fruch]

regardless of the comma, this is not how the function is defined, nor how the template is defined.

so basically to get rid of the template all together ?

[avikivity]

I don't understand the question.

[fruch]

this is the function signature:

template<int Signal, void(*Func)()>
void install_oneshot_signal_handler() {

it receives the signal number and callback function via template, and not via the functions arguments.

I don't have a clue why it defined like that

[fruch]

at least now the compilers are happy, but still didn't found a way to get the template working, to avoid the duplication.

[fruch]

@scylladb/seastar-maint

can someone give a hand with this one ? the only approach I've been able to conjure here is to duplicate the handlers.