| Message ID | 20231219120200.24003-1-martin@martin.st |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel] checkasm: Generalize crash handling | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Tue, Dec 19, 2023 at 1:02 PM Martin Storsjö <martin@martin.st> wrote: > This replaces the riscv specific handling from > 7212466e735aa187d82f51dadbce957fe3da77f0 (which essentially is > reverted, together with 286d6742218ba0235c32876b50bf593cb1986353) > with a different implementation of the same (plus a bit more), based > on the corresponding feature in dav1d's checkasm, supporting both Unix > and Windows. lgtm
Le 19 décembre 2023 14:02:00 GMT+02:00, "Martin Storsjö" <martin@martin.st> a écrit : >This replaces the riscv specific handling from >7212466e735aa187d82f51dadbce957fe3da77f0 (which essentially is >reverted, together with 286d6742218ba0235c32876b50bf593cb1986353) >with a different implementation of the same (plus a bit more), based >on the corresponding feature in dav1d's checkasm, supporting both Unix >and Windows. > >See in particular dav1d commits >0b6ee30eab2400e4f85b735ad29a68a842c34e21 and >0421f787ea592fd2cc74c887f20b8dc31393788b, authored by >Henrik Gramner. > >The overall approach is the same; set up a signal handler, >store the state with setjmp/sigsetjmp, jump out of the crashing >function with longjmp/siglongjmp. > >The main difference is in what happens when the signal handler >is invoked. In the previous implementation, it would resume from >right before calling the crashing function, and then skip that call >based on the setjmp return value. > >In the imported implementation from dav1d, we return to right before >the check_func() call, which will skip testing the current function >(as the pointer is the same as it was before). > >Other differences are: >- Support for other signal handling mechanisms (Windows > AddVectoredExceptionHandler) >- Using RtlCaptureContext/RtlRestoreContext instead of setjmp/longjmp > on Windows with SEH (which adds the design limitation that it doesn't > return a value like setjmp does) >- Only catching signals once per function - if more than one > signal is delivered before signal handling is reenabled, any > signal is handled as it would without our handler >- Not using an arch specific signal handler written in assembly >--- > tests/checkasm/checkasm.c | 100 ++++++++++++++++++++++++++------ > tests/checkasm/checkasm.h | 79 ++++++++++++++++++------- > tests/checkasm/riscv/checkasm.S | 12 ---- > 3 files changed, 140 insertions(+), 51 deletions(-) > >diff --git a/tests/checkasm/checkasm.c b/tests/checkasm/checkasm.c >index 6318d9296b..668034c67f 100644 >--- a/tests/checkasm/checkasm.c >+++ b/tests/checkasm/checkasm.c >@@ -23,8 +23,10 @@ > #include "config.h" > #include "config_components.h" > >-#ifndef _GNU_SOURCE >-# define _GNU_SOURCE // for syscall (performance monitoring API), strsignal() >+#if CONFIG_LINUX_PERF >+# ifndef _GNU_SOURCE >+# define _GNU_SOURCE // for syscall (performance monitoring API) >+# endif > #endif > > #include <signal.h> >@@ -326,6 +328,7 @@ static struct { > const char *cpu_flag_name; > const char *test_name; > int verbose; >+ int catch_signals; > } state; > > /* PRNG state */ >@@ -627,6 +630,64 @@ static CheckasmFunc *get_func(CheckasmFunc **root, const char *name) > return f; > } > >+checkasm_context checkasm_context_buf; >+ >+/* Crash handling: attempt to catch crashes and handle them >+ * gracefully instead of just aborting abruptly. */ >+#ifdef _WIN32 >+#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP) >+static LONG NTAPI signal_handler(EXCEPTION_POINTERS *const e) { >+ const char *err; >+ >+ if (!state.catch_signals) >+ return EXCEPTION_CONTINUE_SEARCH; >+ >+ switch (e->ExceptionRecord->ExceptionCode) { >+ case EXCEPTION_FLT_DIVIDE_BY_ZERO: >+ case EXCEPTION_INT_DIVIDE_BY_ZERO: >+ err = "fatal arithmetic error"; >+ break; >+ case EXCEPTION_ILLEGAL_INSTRUCTION: >+ case EXCEPTION_PRIV_INSTRUCTION: >+ err = "illegal instruction"; >+ break; >+ case EXCEPTION_ACCESS_VIOLATION: >+ case EXCEPTION_ARRAY_BOUNDS_EXCEEDED: >+ case EXCEPTION_DATATYPE_MISALIGNMENT: >+ case EXCEPTION_STACK_OVERFLOW: >+ err = "segmentation fault"; >+ break; >+ case EXCEPTION_IN_PAGE_ERROR: >+ err = "bus error"; >+ break; >+ default: >+ return EXCEPTION_CONTINUE_SEARCH; >+ } >+ state.catch_signals = 0; >+ checkasm_fail_func("%s", err); >+ checkasm_load_context(); >+ return EXCEPTION_CONTINUE_EXECUTION; /* never reached, but shuts up gcc */ >+} >+#endif >+#else >+static void signal_handler(const int s) { >+ if (state.catch_signals) { >+ state.catch_signals = 0; >+ checkasm_fail_func("%s", >+ s == SIGFPE ? "fatal arithmetic error" : >+ s == SIGILL ? "illegal instruction" : >+ s == SIGBUS ? "bus error" : >+ "segmentation fault"); >+ checkasm_load_context(); Use of format string is probably not async-signal-safe. I would also be surprised if the load_context() function was safe in signal context. That's why the current code does pretty much nothing other than a long jump. >+ } else { >+ /* fall back to the default signal handler */ >+ static const struct sigaction default_sa = { .sa_handler = SIG_DFL }; >+ sigaction(s, &default_sa, NULL); >+ raise(s); >+ } >+} >+#endif >+ > /* Perform tests and benchmarks for the specified cpu flag if supported by the host */ > static void check_cpu_flag(const char *name, int flag) > { >@@ -737,18 +798,24 @@ int main(int argc, char *argv[]) > unsigned int seed = av_get_random_seed(); > int i, ret = 0; > >+#ifdef _WIN32 >+#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP) >+ AddVectoredExceptionHandler(0, signal_handler); >+#endif >+#else >+ const struct sigaction sa = { >+ .sa_handler = signal_handler, >+ .sa_flags = SA_NODEFER, >+ }; >+ sigaction(SIGBUS, &sa, NULL); >+ sigaction(SIGFPE, &sa, NULL); >+ sigaction(SIGILL, &sa, NULL); >+ sigaction(SIGSEGV, &sa, NULL); >+#endif > #if ARCH_ARM && HAVE_ARMV5TE_EXTERNAL > if (have_vfp(av_get_cpu_flags()) || have_neon(av_get_cpu_flags())) > checkasm_checked_call = checkasm_checked_call_vfp; > #endif >-#if ARCH_RISCV && HAVE_RV >- struct sigaction act = { >- .sa_handler = checkasm_handle_signal, >- .sa_flags = 0, >- }; >- >- sigaction(SIGILL, &act, NULL); >-#endif > > if (!tests[0].func || !cpus[0].flag) { > fprintf(stderr, "checkasm: no tests to perform\n"); >@@ -876,15 +943,6 @@ void checkasm_fail_func(const char *msg, ...) > } > } > >-void checkasm_fail_signal(int signum) >-{ >-#ifdef __GLIBC__ >- checkasm_fail_func("fatal signal %d: %s", signum, strsignal(signum)); >-#else >- checkasm_fail_func("fatal signal %d", signum); >-#endif >-} >- > /* Get the benchmark context of the current function */ > CheckasmPerf *checkasm_get_perf_context(void) > { >@@ -932,6 +990,10 @@ void checkasm_report(const char *name, ...) > } > } > >+void checkasm_set_signal_handler_state(const int enabled) { >+ state.catch_signals = enabled; >+} >+ > #define DEF_CHECKASM_CHECK_FUNC(type, fmt) \ > int checkasm_check_##type(const char *const file, const int line, \ > const type *buf1, ptrdiff_t stride1, \ >diff --git a/tests/checkasm/checkasm.h b/tests/checkasm/checkasm.h >index e3c19510fa..3d1d16d5bb 100644 >--- a/tests/checkasm/checkasm.h >+++ b/tests/checkasm/checkasm.h >@@ -23,7 +23,6 @@ > #ifndef TESTS_CHECKASM_CHECKASM_H > #define TESTS_CHECKASM_CHECKASM_H > >-#include <setjmp.h> > #include <stdint.h> > #include "config.h" > >@@ -43,6 +42,27 @@ > #include "libavutil/lfg.h" > #include "libavutil/timer.h" > >+#if !ARCH_X86_32 && defined(_WIN32) >+/* setjmp/longjmp on Windows on architectures using SEH (all except x86_32) >+ * will try to use SEH to unwind the stack, which doesn't work for assembly >+ * functions without unwind information. */ >+#include <windows.h> >+#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP) >+#define checkasm_context CONTEXT >+#define checkasm_save_context() RtlCaptureContext(&checkasm_context_buf) >+#define checkasm_load_context() RtlRestoreContext(&checkasm_context_buf, NULL) >+#else >+#define checkasm_context void* >+#define checkasm_save_context() do {} while (0) >+#define checkasm_load_context() do {} while (0) >+#endif >+#else >+#include <setjmp.h> >+#define checkasm_context jmp_buf >+#define checkasm_save_context() setjmp(checkasm_context_buf) >+#define checkasm_load_context() longjmp(checkasm_context_buf, 1) >+#endif >+ > void checkasm_check_aacencdsp(void); > void checkasm_check_aacpsdsp(void); > void checkasm_check_ac3dsp(void); >@@ -105,9 +125,10 @@ struct CheckasmPerf; > void *checkasm_check_func(void *func, const char *name, ...) av_printf_format(2, 3); > int checkasm_bench_func(void); > void checkasm_fail_func(const char *msg, ...) av_printf_format(1, 2); >-void checkasm_fail_signal(int signum); > struct CheckasmPerf *checkasm_get_perf_context(void); > void checkasm_report(const char *name, ...) av_printf_format(1, 2); >+void checkasm_set_signal_handler_state(int enabled); >+extern checkasm_context checkasm_context_buf; > > /* float compare utilities */ > int float_near_ulp(float a, float b, unsigned max_ulp); >@@ -131,7 +152,7 @@ static av_unused void *func_ref, *func_new; > #define BENCH_RUNS 1000 /* Trade-off between accuracy and speed */ > > /* Decide whether or not the specified function needs to be tested */ >-#define check_func(func, ...) (func_ref = checkasm_check_func((func_new = func), __VA_ARGS__)) >+#define check_func(func, ...) (checkasm_save_context(), func_ref = checkasm_check_func((func_new = func), __VA_ARGS__)) > > /* Declare the function prototype. The first argument is the return value, the remaining > * arguments are the function parameters. Naming parameters is optional. */ >@@ -146,7 +167,10 @@ static av_unused void *func_ref, *func_new; > #define report checkasm_report > > /* Call the reference function */ >-#define call_ref(...) ((func_type *)func_ref)(__VA_ARGS__) >+#define call_ref(...)\ >+ (checkasm_set_signal_handler_state(1),\ >+ ((func_type *)func_ref)(__VA_ARGS__));\ >+ checkasm_set_signal_handler_state(0) > > #if ARCH_X86 && HAVE_X86ASM > /* Verifies that clobbered callee-saved registers are properly saved and restored >@@ -179,16 +203,22 @@ void checkasm_stack_clobber(uint64_t clobber, ...); > ((cpu_flags) & av_get_cpu_flags()) ? (void *)checkasm_checked_call_emms : \ > (void *)checkasm_checked_call; > #define CLOB (UINT64_C(0xdeadbeefdeadbeef)) >-#define call_new(...) (checkasm_stack_clobber(CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,\ >+#define call_new(...) (checkasm_set_signal_handler_state(1),\ >+ checkasm_stack_clobber(CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,\ > CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB),\ >- checked_call(func_new, 0, 0, 0, 0, 0, __VA_ARGS__)) >+ checked_call(func_new, 0, 0, 0, 0, 0, __VA_ARGS__));\ >+ checkasm_set_signal_handler_state(0) > #elif ARCH_X86_32 > #define declare_new(ret, ...) ret (*checked_call)(void *, __VA_ARGS__) = (void *)checkasm_checked_call; > #define declare_new_float(ret, ...) ret (*checked_call)(void *, __VA_ARGS__) = (void *)checkasm_checked_call_float; > #define declare_new_emms(cpu_flags, ret, ...) ret (*checked_call)(void *, __VA_ARGS__) = \ > ((cpu_flags) & av_get_cpu_flags()) ? (void *)checkasm_checked_call_emms : \ > (void *)checkasm_checked_call; >-#define call_new(...) checked_call(func_new, __VA_ARGS__) >+#define call_new(...)\ >+ (checkasm_set_signal_handler_state(1),\ >+ checked_call(func_new, __VA_ARGS__, 15, 14, 13, 12,\ >+ 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1));\ >+ checkasm_set_signal_handler_state(0) > #endif > #elif ARCH_ARM && HAVE_ARMV5TE_EXTERNAL > /* Use a dummy argument, to offset the real parameters by 2, not only 1. >@@ -200,7 +230,10 @@ extern void (*checkasm_checked_call)(void *func, int dummy, ...); > #define declare_new(ret, ...) ret (*checked_call)(void *, int dummy, __VA_ARGS__, \ > int, int, int, int, int, int, int, int, \ > int, int, int, int, int, int, int) = (void *)checkasm_checked_call; >-#define call_new(...) checked_call(func_new, 0, __VA_ARGS__, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0, 0, 0, 0) >+#define call_new(...) \ >+ (checkasm_set_signal_handler_state(1),\ >+ checked_call(func_new, 0, __VA_ARGS__, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0, 0, 0, 0));\ >+ checkasm_set_signal_handler_state(0) > #elif ARCH_AARCH64 && !defined(__APPLE__) > void checkasm_stack_clobber(uint64_t clobber, ...); > void checkasm_checked_call(void *func, ...); >@@ -209,35 +242,39 @@ void checkasm_checked_call(void *func, ...); > int, int, int, int, int, int, int)\ > = (void *)checkasm_checked_call; > #define CLOB (UINT64_C(0xdeadbeefdeadbeef)) >-#define call_new(...) (checkasm_stack_clobber(CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,\ >+#define call_new(...) (checkasm_set_signal_handler_state(1),\ >+ checkasm_stack_clobber(CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,\ > CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB),\ > checked_call(func_new, 0, 0, 0, 0, 0, 0, 0, __VA_ARGS__,\ >- 7, 6, 5, 4, 3, 2, 1, 0, 0, 0, 0, 0, 0, 0, 0)) >+ 7, 6, 5, 4, 3, 2, 1, 0, 0, 0, 0, 0, 0, 0, 0));\ >+ checkasm_set_signal_handler_state(0) > #elif ARCH_RISCV >-void checkasm_set_function(void *, sigjmp_buf); >+void checkasm_set_function(void *); > void *checkasm_get_wrapper(void); >-void checkasm_handle_signal(int signum); > > #if HAVE_RV && (__riscv_xlen == 64) && defined (__riscv_d) > #define declare_new(ret, ...) \ >- int checked_call_signum = 0; \ >- sigjmp_buf checked_call_jb; \ > ret (*checked_call)(__VA_ARGS__) = checkasm_get_wrapper(); > #define call_new(...) \ >- (checkasm_set_function(func_new, checked_call_jb), \ >- (checked_call_signum = sigsetjmp(checked_call_jb, 1)) == 0 \ >- ? checked_call(__VA_ARGS__) \ >- : (checkasm_fail_signal(checked_call_signum), 0)) >+ (checkasm_set_signal_handler_state(1),\ >+ checkasm_set_function(func_new), checked_call(__VA_ARGS__));\ >+ checkasm_set_signal_handler_state(0) > #else > #define declare_new(ret, ...) >-#define call_new(...) ((func_type *)func_new)(__VA_ARGS__) >+#define call_new(...)\ >+ (checkasm_set_signal_handler_state(1),\ >+ ((func_type *)func_new)(__VA_ARGS__));\ >+ checkasm_set_signal_handler_state(0) > #endif > #else > #define declare_new(ret, ...) > #define declare_new_float(ret, ...) > #define declare_new_emms(cpu_flags, ret, ...) > /* Call the function */ >-#define call_new(...) ((func_type *)func_new)(__VA_ARGS__) >+#define call_new(...)\ >+ (checkasm_set_signal_handler_state(1),\ >+ ((func_type *)func_new)(__VA_ARGS__));\ >+ checkasm_set_signal_handler_state(0) > #endif > > #ifndef declare_new_emms >@@ -284,6 +321,7 @@ typedef struct CheckasmPerf { > uint64_t tsum = 0;\ > int ti, tcount = 0;\ > uint64_t t = 0; \ >+ checkasm_set_signal_handler_state(1);\ > for (ti = 0; ti < BENCH_RUNS; ti++) {\ > PERF_START(t);\ > tfunc(__VA_ARGS__);\ >@@ -299,6 +337,7 @@ typedef struct CheckasmPerf { > emms_c();\ > perf->cycles += t;\ > perf->iterations++;\ >+ checkasm_set_signal_handler_state(0);\ > }\ > } while (0) > #else >diff --git a/tests/checkasm/riscv/checkasm.S b/tests/checkasm/riscv/checkasm.S >index 971d881157..73ca85f344 100644 >--- a/tests/checkasm/riscv/checkasm.S >+++ b/tests/checkasm/riscv/checkasm.S >@@ -41,7 +41,6 @@ endconst > > checked_func: > .quad 0 >- .quad 0 > > saved_regs: > /* Space to spill RA, SP, GP, TP, S0-S11 and FS0-FS11 */ >@@ -53,7 +52,6 @@ func checkasm_set_function > la.tls.ie t0, checked_func > add t0, tp, t0 > sd a0, (t0) >- sd a1, 8(t0) > ret > endfunc > >@@ -177,14 +175,4 @@ func checkasm_get_wrapper, v > call checkasm_fail_func > j 4b > endfunc >- >-func checkasm_handle_signal >- mv a1, a0 >- la.tls.ie a0, checked_func >- add a0, tp, a0 >- ld a0, 8(a0) >- beqz a0, 8f >- tail siglongjmp >-8: tail abort /* No jump buffer to go to */ >-endfunc > #endif
On Thu, 21 Dec 2023, Rémi Denis-Courmont wrote: >> diff --git a/tests/checkasm/checkasm.c b/tests/checkasm/checkasm.c >> index 6318d9296b..668034c67f 100644 >> --- a/tests/checkasm/checkasm.c >> +++ b/tests/checkasm/checkasm.c >> @@ -23,8 +23,10 @@ >> #include "config.h" >> #include "config_components.h" >> >> -#ifndef _GNU_SOURCE >> -# define _GNU_SOURCE // for syscall (performance monitoring API), strsignal() >> +#if CONFIG_LINUX_PERF >> +# ifndef _GNU_SOURCE >> +# define _GNU_SOURCE // for syscall (performance monitoring API) >> +# endif >> #endif >> >> #include <signal.h> >> @@ -326,6 +328,7 @@ static struct { >> const char *cpu_flag_name; >> const char *test_name; >> int verbose; >> + int catch_signals; >> } state; >> >> /* PRNG state */ >> @@ -627,6 +630,64 @@ static CheckasmFunc *get_func(CheckasmFunc **root, const char *name) >> return f; >> } >> >> +checkasm_context checkasm_context_buf; >> + >> +/* Crash handling: attempt to catch crashes and handle them >> + * gracefully instead of just aborting abruptly. */ >> +#ifdef _WIN32 >> +#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP) >> +static LONG NTAPI signal_handler(EXCEPTION_POINTERS *const e) { >> + const char *err; >> + >> + if (!state.catch_signals) >> + return EXCEPTION_CONTINUE_SEARCH; >> + >> + switch (e->ExceptionRecord->ExceptionCode) { >> + case EXCEPTION_FLT_DIVIDE_BY_ZERO: >> + case EXCEPTION_INT_DIVIDE_BY_ZERO: >> + err = "fatal arithmetic error"; >> + break; >> + case EXCEPTION_ILLEGAL_INSTRUCTION: >> + case EXCEPTION_PRIV_INSTRUCTION: >> + err = "illegal instruction"; >> + break; >> + case EXCEPTION_ACCESS_VIOLATION: >> + case EXCEPTION_ARRAY_BOUNDS_EXCEEDED: >> + case EXCEPTION_DATATYPE_MISALIGNMENT: >> + case EXCEPTION_STACK_OVERFLOW: >> + err = "segmentation fault"; >> + break; >> + case EXCEPTION_IN_PAGE_ERROR: >> + err = "bus error"; >> + break; >> + default: >> + return EXCEPTION_CONTINUE_SEARCH; >> + } >> + state.catch_signals = 0; >> + checkasm_fail_func("%s", err); >> + checkasm_load_context(); >> + return EXCEPTION_CONTINUE_EXECUTION; /* never reached, but shuts up gcc */ >> +} >> +#endif >> +#else >> +static void signal_handler(const int s) { >> + if (state.catch_signals) { >> + state.catch_signals = 0; >> + checkasm_fail_func("%s", >> + s == SIGFPE ? "fatal arithmetic error" : >> + s == SIGILL ? "illegal instruction" : >> + s == SIGBUS ? "bus error" : >> + "segmentation fault"); >> + checkasm_load_context(); > > Use of format string is probably not async-signal-safe. I'll have a look if it's possible to restructure this into doing the checkasm_fail_func call afterwards. > I would also be surprised if the load_context() function was safe in > signal context. That's why the current code does pretty much nothing > other than a long jump. The load_context() function here is either longjmp or RtlRestoreContext. // Martin
Le tiistaina 19. joulukuuta 2023, 14.02.00 EET Martin Storsjö a écrit : > This replaces the riscv specific handling from > 7212466e735aa187d82f51dadbce957fe3da77f0 (which essentially is > reverted, together with 286d6742218ba0235c32876b50bf593cb1986353) > with a different implementation of the same (plus a bit more), based > on the corresponding feature in dav1d's checkasm, supporting both Unix > and Windows. > > See in particular dav1d commits > 0b6ee30eab2400e4f85b735ad29a68a842c34e21 and > 0421f787ea592fd2cc74c887f20b8dc31393788b, authored by > Henrik Gramner. > > The overall approach is the same; set up a signal handler, > store the state with setjmp/sigsetjmp, jump out of the crashing > function with longjmp/siglongjmp. > > The main difference is in what happens when the signal handler > is invoked. In the previous implementation, it would resume from > right before calling the crashing function, and then skip that call > based on the setjmp return value. > > In the imported implementation from dav1d, we return to right before > the check_func() call, which will skip testing the current function > (as the pointer is the same as it was before). > > Other differences are: > - Support for other signal handling mechanisms (Windows > AddVectoredExceptionHandler) > - Using RtlCaptureContext/RtlRestoreContext instead of setjmp/longjmp > on Windows with SEH (which adds the design limitation that it doesn't > return a value like setjmp does) > - Only catching signals once per function - if more than one > signal is delivered before signal handling is reenabled, any > signal is handled as it would without our handler > - Not using an arch specific signal handler written in assembly > --- > tests/checkasm/checkasm.c | 100 ++++++++++++++++++++++++++------ > tests/checkasm/checkasm.h | 79 ++++++++++++++++++------- > tests/checkasm/riscv/checkasm.S | 12 ---- > 3 files changed, 140 insertions(+), 51 deletions(-) > > diff --git a/tests/checkasm/checkasm.c b/tests/checkasm/checkasm.c > index 6318d9296b..668034c67f 100644 > --- a/tests/checkasm/checkasm.c > +++ b/tests/checkasm/checkasm.c > @@ -23,8 +23,10 @@ > #include "config.h" > #include "config_components.h" > > -#ifndef _GNU_SOURCE > -# define _GNU_SOURCE // for syscall (performance monitoring API), > strsignal() +#if CONFIG_LINUX_PERF > +# ifndef _GNU_SOURCE > +# define _GNU_SOURCE // for syscall (performance monitoring API) > +# endif > #endif > > #include <signal.h> > @@ -326,6 +328,7 @@ static struct { > const char *cpu_flag_name; > const char *test_name; > int verbose; > + int catch_signals; AFAICT, this needs to be volatile sigatomic_t > } state; > > /* PRNG state */ > @@ -627,6 +630,64 @@ static CheckasmFunc *get_func(CheckasmFunc **root, > const char *name) return f; > } > > +checkasm_context checkasm_context_buf; > + > +/* Crash handling: attempt to catch crashes and handle them > + * gracefully instead of just aborting abruptly. */ > +#ifdef _WIN32 > +#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP) > +static LONG NTAPI signal_handler(EXCEPTION_POINTERS *const e) { > + const char *err; > + > + if (!state.catch_signals) > + return EXCEPTION_CONTINUE_SEARCH; > + > + switch (e->ExceptionRecord->ExceptionCode) { > + case EXCEPTION_FLT_DIVIDE_BY_ZERO: > + case EXCEPTION_INT_DIVIDE_BY_ZERO: > + err = "fatal arithmetic error"; > + break; > + case EXCEPTION_ILLEGAL_INSTRUCTION: > + case EXCEPTION_PRIV_INSTRUCTION: > + err = "illegal instruction"; > + break; > + case EXCEPTION_ACCESS_VIOLATION: > + case EXCEPTION_ARRAY_BOUNDS_EXCEEDED: > + case EXCEPTION_DATATYPE_MISALIGNMENT: > + case EXCEPTION_STACK_OVERFLOW: > + err = "segmentation fault"; > + break; > + case EXCEPTION_IN_PAGE_ERROR: > + err = "bus error"; > + break; > + default: > + return EXCEPTION_CONTINUE_SEARCH; > + } > + state.catch_signals = 0; > + checkasm_fail_func("%s", err); > + checkasm_load_context(); > + return EXCEPTION_CONTINUE_EXECUTION; /* never reached, but shuts up gcc > */ +} > +#endif > +#else > +static void signal_handler(const int s) { > + if (state.catch_signals) { > + state.catch_signals = 0; > + checkasm_fail_func("%s", > + s == SIGFPE ? "fatal arithmetic error" : > + s == SIGILL ? "illegal instruction" : > + s == SIGBUS ? "bus error" : > + "segmentation fault"); The current code for the error print-out is both simpler and more versatile, so I don't get this. > + checkasm_load_context(); > + } else { > + /* fall back to the default signal handler */ > + static const struct sigaction default_sa = { .sa_handler = SIG_DFL > }; + sigaction(s, &default_sa, NULL); > + raise(s); Why raise here? Returning from the handler will reevaluate the same code with the same thread state, and trigger the default signal handler anyway (since you don't modify the user context). > + } > +} > +#endif > + > /* Perform tests and benchmarks for the specified cpu flag if supported by > the host */ static void check_cpu_flag(const char *name, int flag) > { > @@ -737,18 +798,24 @@ int main(int argc, char *argv[]) > unsigned int seed = av_get_random_seed(); > int i, ret = 0; > > +#ifdef _WIN32 > +#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP) > + AddVectoredExceptionHandler(0, signal_handler); > +#endif > +#else > + const struct sigaction sa = { > + .sa_handler = signal_handler, > + .sa_flags = SA_NODEFER, That does not look very sane to me. If a recursive signal occurs, processing it recursively is NOT a good idea. This would cause an infinite loop, eventually a literal stack overflow after maxing out the processor for a while. I'd rather let the OS kernel deal with the problem, by killing the process or whatever the last resort is. > + }; > + sigaction(SIGBUS, &sa, NULL); > + sigaction(SIGFPE, &sa, NULL); > + sigaction(SIGILL, &sa, NULL); > + sigaction(SIGSEGV, &sa, NULL); > +#endif > #if ARCH_ARM && HAVE_ARMV5TE_EXTERNAL > if (have_vfp(av_get_cpu_flags()) || have_neon(av_get_cpu_flags())) > checkasm_checked_call = checkasm_checked_call_vfp; > #endif > -#if ARCH_RISCV && HAVE_RV > - struct sigaction act = { > - .sa_handler = checkasm_handle_signal, > - .sa_flags = 0, > - }; > - > - sigaction(SIGILL, &act, NULL); > -#endif > > if (!tests[0].func || !cpus[0].flag) { > fprintf(stderr, "checkasm: no tests to perform\n"); > @@ -876,15 +943,6 @@ void checkasm_fail_func(const char *msg, ...) > } > } > > -void checkasm_fail_signal(int signum) > -{ > -#ifdef __GLIBC__ > - checkasm_fail_func("fatal signal %d: %s", signum, strsignal(signum)); > -#else > - checkasm_fail_func("fatal signal %d", signum); > -#endif > -} > - > /* Get the benchmark context of the current function */ > CheckasmPerf *checkasm_get_perf_context(void) > { > @@ -932,6 +990,10 @@ void checkasm_report(const char *name, ...) > } > } > > +void checkasm_set_signal_handler_state(const int enabled) { > + state.catch_signals = enabled; > +} > + > #define DEF_CHECKASM_CHECK_FUNC(type, fmt) \ > int checkasm_check_##type(const char *const file, const int line, \ > const type *buf1, ptrdiff_t stride1, \ > diff --git a/tests/checkasm/checkasm.h b/tests/checkasm/checkasm.h > index e3c19510fa..3d1d16d5bb 100644 > --- a/tests/checkasm/checkasm.h > +++ b/tests/checkasm/checkasm.h > @@ -23,7 +23,6 @@ > #ifndef TESTS_CHECKASM_CHECKASM_H > #define TESTS_CHECKASM_CHECKASM_H > > -#include <setjmp.h> > #include <stdint.h> > #include "config.h" > > @@ -43,6 +42,27 @@ > #include "libavutil/lfg.h" > #include "libavutil/timer.h" > > +#if !ARCH_X86_32 && defined(_WIN32) > +/* setjmp/longjmp on Windows on architectures using SEH (all except x86_32) > + * will try to use SEH to unwind the stack, which doesn't work for > assembly + * functions without unwind information. */ > +#include <windows.h> > +#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP) > +#define checkasm_context CONTEXT > +#define checkasm_save_context() RtlCaptureContext(&checkasm_context_buf) > +#define checkasm_load_context() RtlRestoreContext(&checkasm_context_buf, > NULL) +#else > +#define checkasm_context void* > +#define checkasm_save_context() do {} while (0) > +#define checkasm_load_context() do {} while (0) > +#endif > +#else > +#include <setjmp.h> > +#define checkasm_context jmp_buf > +#define checkasm_save_context() setjmp(checkasm_context_buf) > +#define checkasm_load_context() longjmp(checkasm_context_buf, 1) > +#endif Been there done that and it did not end well. sigsetjmp() & co are necessary here. > + > void checkasm_check_aacencdsp(void); > void checkasm_check_aacpsdsp(void); > void checkasm_check_ac3dsp(void); > @@ -105,9 +125,10 @@ struct CheckasmPerf; > void *checkasm_check_func(void *func, const char *name, ...) > av_printf_format(2, 3); int checkasm_bench_func(void); > void checkasm_fail_func(const char *msg, ...) av_printf_format(1, 2); > -void checkasm_fail_signal(int signum); > struct CheckasmPerf *checkasm_get_perf_context(void); > void checkasm_report(const char *name, ...) av_printf_format(1, 2); > +void checkasm_set_signal_handler_state(int enabled); > +extern checkasm_context checkasm_context_buf; > > /* float compare utilities */ > int float_near_ulp(float a, float b, unsigned max_ulp); > @@ -131,7 +152,7 @@ static av_unused void *func_ref, *func_new; > #define BENCH_RUNS 1000 /* Trade-off between accuracy and speed */ > > /* Decide whether or not the specified function needs to be tested */ > -#define check_func(func, ...) (func_ref = checkasm_check_func((func_new = > func), __VA_ARGS__)) > +#define check_func(func, ...) > (checkasm_save_context(), func_ref = checkasm_check_func((func_new = func), > __VA_ARGS__)) > > /* Declare the function prototype. The first argument is the return value, > the remaining * arguments are the function parameters. Naming parameters is > optional. */ @@ -146,7 +167,10 @@ static av_unused void *func_ref, > *func_new; > #define report checkasm_report > > /* Call the reference function */ > -#define call_ref(...) ((func_type *)func_ref)(__VA_ARGS__) > +#define call_ref(...)\ > + (checkasm_set_signal_handler_state(1),\ > + ((func_type *)func_ref)(__VA_ARGS__));\ > + checkasm_set_signal_handler_state(0) > > #if ARCH_X86 && HAVE_X86ASM > /* Verifies that clobbered callee-saved registers are properly saved and > restored @@ -179,16 +203,22 @@ void checkasm_stack_clobber(uint64_t > clobber, ...); ((cpu_flags) & av_get_cpu_flags()) ? (void > *)checkasm_checked_call_emms : \ (void *)checkasm_checked_call; #define > CLOB (UINT64_C(0xdeadbeefdeadbeef)) > -#define call_new(...) > (checkasm_stack_clobber(CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,C > LOB,\ +#define call_new(...) (checkasm_set_signal_handler_state(1),\ > + > checkasm_stack_clobber(CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CL > OB,\ CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB),\ - > checked_call(func_new, 0, 0, 0, 0, 0, __VA_ARGS__)) + > checked_call(func_new, 0, 0, 0, 0, 0, __VA_ARGS__));\ + > checkasm_set_signal_handler_state(0) > #elif ARCH_X86_32 > #define declare_new(ret, ...) ret (*checked_call)(void *, __VA_ARGS__) = > (void *)checkasm_checked_call; #define declare_new_float(ret, ...) ret > (*checked_call)(void *, __VA_ARGS__) = (void *)checkasm_checked_call_float; > #define declare_new_emms(cpu_flags, ret, ...) ret (*checked_call)(void *, > __VA_ARGS__) = \ ((cpu_flags) & av_get_cpu_flags()) ? (void > *)checkasm_checked_call_emms : \ (void *)checkasm_checked_call; > -#define call_new(...) checked_call(func_new, __VA_ARGS__) > +#define call_new(...)\ > + (checkasm_set_signal_handler_state(1),\ > + checked_call(func_new, __VA_ARGS__, 15, 14, 13, 12,\ > + 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1));\ > + checkasm_set_signal_handler_state(0) > #endif > #elif ARCH_ARM && HAVE_ARMV5TE_EXTERNAL > /* Use a dummy argument, to offset the real parameters by 2, not only 1. > @@ -200,7 +230,10 @@ extern void (*checkasm_checked_call)(void *func, int > dummy, ...); #define declare_new(ret, ...) ret (*checked_call)(void *, int > dummy, __VA_ARGS__, \ int, int, int, int, int, int, int, int, \ int, int, > int, int, int, int, int) = (void *)checkasm_checked_call; -#define > call_new(...) checked_call(func_new, 0, __VA_ARGS__, 11, 10, 9, 8, 7, 6, 5, > 4, 3, 2, 1, 0, 0, 0, 0) +#define call_new(...) \ > + (checkasm_set_signal_handler_state(1),\ > + checked_call(func_new, 0, __VA_ARGS__, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, > 1, 0, 0, 0, 0));\ + checkasm_set_signal_handler_state(0) > #elif ARCH_AARCH64 && !defined(__APPLE__) > void checkasm_stack_clobber(uint64_t clobber, ...); > void checkasm_checked_call(void *func, ...); > @@ -209,35 +242,39 @@ void checkasm_checked_call(void *func, ...); > int, int, int, int, int, > int, int)\ = (void *)checkasm_checked_call; > #define CLOB (UINT64_C(0xdeadbeefdeadbeef)) > -#define call_new(...) > (checkasm_stack_clobber(CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,C > LOB,CLOB,\ +#define call_new(...) (checkasm_set_signal_handler_state(1),\ > + > checkasm_stack_clobber(CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CL > OB,CLOB,\ CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB),\ > checked_call(func_new, 0, 0, 0, 0, 0, 0, 0, __VA_ARGS__,\ - > 7, 6, 5, 4, 3, 2, 1, 0, 0, 0, 0, 0, 0, 0, 0)) + > 7, 6, 5, 4, 3, 2, 1, 0, 0, 0, 0, 0, 0, 0, 0));\ + > checkasm_set_signal_handler_state(0) > #elif ARCH_RISCV > -void checkasm_set_function(void *, sigjmp_buf); > +void checkasm_set_function(void *); > void *checkasm_get_wrapper(void); > -void checkasm_handle_signal(int signum); > > #if HAVE_RV && (__riscv_xlen == 64) && defined (__riscv_d) > #define declare_new(ret, ...) \ > - int checked_call_signum = 0; \ > - sigjmp_buf checked_call_jb; \ > ret (*checked_call)(__VA_ARGS__) = checkasm_get_wrapper(); > #define call_new(...) \ > - (checkasm_set_function(func_new, checked_call_jb), \ > - (checked_call_signum = sigsetjmp(checked_call_jb, 1)) == 0 \ > - ? checked_call(__VA_ARGS__) \ > - : (checkasm_fail_signal(checked_call_signum), 0)) > + (checkasm_set_signal_handler_state(1),\ > + checkasm_set_function(func_new), checked_call(__VA_ARGS__));\ > + checkasm_set_signal_handler_state(0) > #else > #define declare_new(ret, ...) > -#define call_new(...) ((func_type *)func_new)(__VA_ARGS__) > +#define call_new(...)\ > + (checkasm_set_signal_handler_state(1),\ > + ((func_type *)func_new)(__VA_ARGS__));\ > + checkasm_set_signal_handler_state(0) > #endif > #else > #define declare_new(ret, ...) > #define declare_new_float(ret, ...) > #define declare_new_emms(cpu_flags, ret, ...) > /* Call the function */ > -#define call_new(...) ((func_type *)func_new)(__VA_ARGS__) > +#define call_new(...)\ > + (checkasm_set_signal_handler_state(1),\ > + ((func_type *)func_new)(__VA_ARGS__));\ > + checkasm_set_signal_handler_state(0) > #endif > > #ifndef declare_new_emms > @@ -284,6 +321,7 @@ typedef struct CheckasmPerf { > uint64_t tsum = 0;\ > int ti, tcount = 0;\ > uint64_t t = 0; \ > + checkasm_set_signal_handler_state(1);\ > for (ti = 0; ti < BENCH_RUNS; ti++) {\ > PERF_START(t);\ > tfunc(__VA_ARGS__);\ > @@ -299,6 +337,7 @@ typedef struct CheckasmPerf { > emms_c();\ > perf->cycles += t;\ > perf->iterations++;\ > + checkasm_set_signal_handler_state(0);\ > }\ > } while (0) > #else > diff --git a/tests/checkasm/riscv/checkasm.S > b/tests/checkasm/riscv/checkasm.S index 971d881157..73ca85f344 100644 > --- a/tests/checkasm/riscv/checkasm.S > +++ b/tests/checkasm/riscv/checkasm.S > @@ -41,7 +41,6 @@ endconst > > checked_func: > .quad 0 > - .quad 0 > > saved_regs: > /* Space to spill RA, SP, GP, TP, S0-S11 and FS0-FS11 */ > @@ -53,7 +52,6 @@ func checkasm_set_function > la.tls.ie t0, checked_func > add t0, tp, t0 > sd a0, (t0) > - sd a1, 8(t0) > ret > endfunc > > @@ -177,14 +175,4 @@ func checkasm_get_wrapper, v > call checkasm_fail_func > j 4b > endfunc > - > -func checkasm_handle_signal > - mv a1, a0 > - la.tls.ie a0, checked_func > - add a0, tp, a0 > - ld a0, 8(a0) > - beqz a0, 8f > - tail siglongjmp > -8: tail abort /* No jump buffer to go to */ > -endfunc > #endif
On Thu, Dec 21, 2023 at 9:16 PM Rémi Denis-Courmont <remi@remlab.net> wrote: > > + checkasm_fail_func("%s", > > + s == SIGFPE ? "fatal arithmetic error" : > > + s == SIGILL ? "illegal instruction" : > > + s == SIGBUS ? "bus error" : > > + "segmentation fault"); > > The current code for the error print-out is both simpler and more versatile, > so I don't get this. IMO "illegal instruction" is a far better error message than "fatal signal 4" (with an implementation-defined number which nobody knows the meaning of without having to look it up). > > + /* fall back to the default signal handler */ > > + static const struct sigaction default_sa = { .sa_handler = SIG_DFL > > }; + sigaction(s, &default_sa, NULL); > > + raise(s); > > Why raise here? Returning from the handler will reevaluate the same code with > the same thread state, and trigger the default signal handler anyway (since > you don't modify the user context). No it wont, it'll get stuck in an infinite loop invoking the signal handler over and over. At least on my system. > > + const struct sigaction sa = { > > + .sa_handler = signal_handler, > > + .sa_flags = SA_NODEFER, > > That does not look very sane to me. If a recursive signal occurs, processing > it recursively is NOT a good idea. This would cause an infinite loop, > eventually a literal stack overflow after maxing out the processor for a while. > I'd rather let the OS kernel deal with the problem, by killing the process or > whatever the last resort is. > > > +#define checkasm_save_context() setjmp(checkasm_context_buf) > > +#define checkasm_load_context() longjmp(checkasm_context_buf, 1) > > +#endif > > Been there done that and it did not end well. > sigsetjmp() & co are necessary here. For all intents and purposes sigjmp()/longjmp() with SA_NODEFER does the same thing as sigsetjmp()/siglongjmp() without SA_NODEFER for this particular use case (no infinite recursion is possible the way the code is written). The change isn't necessary per se but it seems reasonable and I have no objections to it.
Le 22 décembre 2023 00:03:59 GMT+02:00, Henrik Gramner via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> a écrit : >On Thu, Dec 21, 2023 at 9:16 PM Rémi Denis-Courmont <remi@remlab.net> wrote: >> > + checkasm_fail_func("%s", >> > + s == SIGFPE ? "fatal arithmetic error" : >> > + s == SIGILL ? "illegal instruction" : >> > + s == SIGBUS ? "bus error" : >> > + "segmentation fault"); >> >> The current code for the error print-out is both simpler and more versatile, >> so I don't get this. > >IMO "illegal instruction" is a far better error message than "fatal >signal 4" (with an implementation-defined number which nobody knows >the meaning of without having to look it up). The current code prints the number and the name. > >> > + /* fall back to the default signal handler */ >> > + static const struct sigaction default_sa = { .sa_handler = SIG_DFL >> > }; + sigaction(s, &default_sa, NULL); >> > + raise(s); >> >> Why raise here? Returning from the handler will reevaluate the same code with >> the same thread state, and trigger the default signal handler anyway (since >> you don't modify the user context). > >No it wont, it'll get stuck in an infinite loop invoking the signal >handler over and over. At least on my system. No, it won't since the default signal handler was restored. And it's much less confusing to debug if the signal comes from where it was actually triggered than from explicit raise call. > >> > + const struct sigaction sa = { >> > + .sa_handler = signal_handler, >> > + .sa_flags = SA_NODEFER, >> >> That does not look very sane to me. If a recursive signal occurs, processing >> it recursively is NOT a good idea. This would cause an infinite loop, >> eventually a literal stack overflow after maxing out the processor for a while. >> I'd rather let the OS kernel deal with the problem, by killing the process or >> whatever the last resort is. >> >> > +#define checkasm_save_context() setjmp(checkasm_context_buf) >> > +#define checkasm_load_context() longjmp(checkasm_context_buf, 1) >> > +#endif >> >> Been there done that and it did not end well. >> sigsetjmp() & co are necessary here. > >For all intents and purposes sigjmp()/longjmp() with SA_NODEFER does >the same thing as sigsetjmp()/siglongjmp() without SA_NODEFER for this >particular use case (no infinite recursion is possible the way the >code is written). The change isn't necessary per se but it seems >reasonable and I have no objections to it. >_______________________________________________ >ffmpeg-devel mailing list >ffmpeg-devel@ffmpeg.org >https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >To unsubscribe, visit link above, or email >ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Le 21 décembre 2023 22:16:09 GMT+02:00, "Rémi Denis-Courmont" <remi@remlab.net> a écrit : >Le tiistaina 19. joulukuuta 2023, 14.02.00 EET Martin Storsjö a écrit : >> This replaces the riscv specific handling from >> 7212466e735aa187d82f51dadbce957fe3da77f0 (which essentially is >> reverted, together with 286d6742218ba0235c32876b50bf593cb1986353) >> with a different implementation of the same (plus a bit more), based >> on the corresponding feature in dav1d's checkasm, supporting both Unix >> and Windows. >> >> See in particular dav1d commits >> 0b6ee30eab2400e4f85b735ad29a68a842c34e21 and >> 0421f787ea592fd2cc74c887f20b8dc31393788b, authored by >> Henrik Gramner. >> >> The overall approach is the same; set up a signal handler, >> store the state with setjmp/sigsetjmp, jump out of the crashing >> function with longjmp/siglongjmp. >> >> The main difference is in what happens when the signal handler >> is invoked. In the previous implementation, it would resume from >> right before calling the crashing function, and then skip that call >> based on the setjmp return value. >> >> In the imported implementation from dav1d, we return to right before >> the check_func() call, which will skip testing the current function >> (as the pointer is the same as it was before). >> >> Other differences are: >> - Support for other signal handling mechanisms (Windows >> AddVectoredExceptionHandler) >> - Using RtlCaptureContext/RtlRestoreContext instead of setjmp/longjmp >> on Windows with SEH (which adds the design limitation that it doesn't >> return a value like setjmp does) >> - Only catching signals once per function - if more than one >> signal is delivered before signal handling is reenabled, any >> signal is handled as it would without our handler >> - Not using an arch specific signal handler written in assembly >> --- >> tests/checkasm/checkasm.c | 100 ++++++++++++++++++++++++++------ >> tests/checkasm/checkasm.h | 79 ++++++++++++++++++------- >> tests/checkasm/riscv/checkasm.S | 12 ---- >> 3 files changed, 140 insertions(+), 51 deletions(-) >> >> diff --git a/tests/checkasm/checkasm.c b/tests/checkasm/checkasm.c >> index 6318d9296b..668034c67f 100644 >> --- a/tests/checkasm/checkasm.c >> +++ b/tests/checkasm/checkasm.c >> @@ -23,8 +23,10 @@ >> #include "config.h" >> #include "config_components.h" >> >> -#ifndef _GNU_SOURCE >> -# define _GNU_SOURCE // for syscall (performance monitoring API), >> strsignal() +#if CONFIG_LINUX_PERF >> +# ifndef _GNU_SOURCE >> +# define _GNU_SOURCE // for syscall (performance monitoring API) >> +# endif >> #endif >> >> #include <signal.h> >> @@ -326,6 +328,7 @@ static struct { >> const char *cpu_flag_name; >> const char *test_name; >> int verbose; >> + int catch_signals; > >AFAICT, this needs to be volatile sigatomic_t > >> } state; >> >> /* PRNG state */ >> @@ -627,6 +630,64 @@ static CheckasmFunc *get_func(CheckasmFunc **root, >> const char *name) return f; >> } >> >> +checkasm_context checkasm_context_buf; >> + >> +/* Crash handling: attempt to catch crashes and handle them >> + * gracefully instead of just aborting abruptly. */ >> +#ifdef _WIN32 >> +#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP) >> +static LONG NTAPI signal_handler(EXCEPTION_POINTERS *const e) { >> + const char *err; >> + >> + if (!state.catch_signals) >> + return EXCEPTION_CONTINUE_SEARCH; >> + >> + switch (e->ExceptionRecord->ExceptionCode) { >> + case EXCEPTION_FLT_DIVIDE_BY_ZERO: >> + case EXCEPTION_INT_DIVIDE_BY_ZERO: >> + err = "fatal arithmetic error"; >> + break; >> + case EXCEPTION_ILLEGAL_INSTRUCTION: >> + case EXCEPTION_PRIV_INSTRUCTION: >> + err = "illegal instruction"; >> + break; >> + case EXCEPTION_ACCESS_VIOLATION: >> + case EXCEPTION_ARRAY_BOUNDS_EXCEEDED: >> + case EXCEPTION_DATATYPE_MISALIGNMENT: >> + case EXCEPTION_STACK_OVERFLOW: >> + err = "segmentation fault"; >> + break; >> + case EXCEPTION_IN_PAGE_ERROR: >> + err = "bus error"; >> + break; >> + default: >> + return EXCEPTION_CONTINUE_SEARCH; >> + } >> + state.catch_signals = 0; >> + checkasm_fail_func("%s", err); >> + checkasm_load_context(); >> + return EXCEPTION_CONTINUE_EXECUTION; /* never reached, but shuts up gcc >> */ +} >> +#endif >> +#else >> +static void signal_handler(const int s) { >> + if (state.catch_signals) { >> + state.catch_signals = 0; >> + checkasm_fail_func("%s", >> + s == SIGFPE ? "fatal arithmetic error" : >> + s == SIGILL ? "illegal instruction" : >> + s == SIGBUS ? "bus error" : >> + "segmentation fault"); > >The current code for the error print-out is both simpler and more versatile, >so I don't get this. > >> + checkasm_load_context(); >> + } else { >> + /* fall back to the default signal handler */ >> + static const struct sigaction default_sa = { .sa_handler = SIG_DFL >> }; + sigaction(s, &default_sa, NULL); >> + raise(s); > >Why raise here? Returning from the handler will reevaluate the same code with >the same thread state, and trigger the default signal handler anyway (since >you don't modify the user context). > >> + } >> +} >> +#endif >> + >> /* Perform tests and benchmarks for the specified cpu flag if supported by >> the host */ static void check_cpu_flag(const char *name, int flag) >> { >> @@ -737,18 +798,24 @@ int main(int argc, char *argv[]) >> unsigned int seed = av_get_random_seed(); >> int i, ret = 0; >> >> +#ifdef _WIN32 >> +#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP) >> + AddVectoredExceptionHandler(0, signal_handler); >> +#endif >> +#else >> + const struct sigaction sa = { >> + .sa_handler = signal_handler, >> + .sa_flags = SA_NODEFER, > >That does not look very sane to me. If a recursive signal occurs, processing >it recursively is NOT a good idea. Following that, it actually seems safer to automatically reset the handler, using `signal()` or equivalently passing the `SA_RESETHAND` flag. Then the handler can rearm its own self if and *only* if it was able to actually handle the signal by observing a long jump. Resetting to default explicitly is no longer useful then.
On Fri, Dec 22, 2023 at 7:20 AM Rémi Denis-Courmont <remi@remlab.net> wrote: > >> > + checkasm_fail_func("%s", > >> > + s == SIGFPE ? "fatal arithmetic error" : > >> > + s == SIGILL ? "illegal instruction" : > >> > + s == SIGBUS ? "bus error" : > >> > + "segmentation fault"); > >> > >> The current code for the error print-out is both simpler and more versatile, > >> so I don't get this. > > > >IMO "illegal instruction" is a far better error message than "fatal > >signal 4" (with an implementation-defined number which nobody knows > >the meaning of without having to look it up). > > The current code prints the number and the name. Not on all supported systems. And even when it does it's in an implementation-defined and locale-dependent form, which isn't great. > >> + const struct sigaction sa = { > >> + .sa_handler = signal_handler, > >> + .sa_flags = SA_NODEFER, > > > >That does not look very sane to me. If a recursive signal occurs, processing > >it recursively is NOT a good idea. > > Following that, it actually seems safer to automatically reset the handler, using `signal()` or equivalently passing the `SA_RESETHAND` flag. Then the handler can rearm its own self if and *only* if it was able to actually handle the signal by observing a long jump. Resetting to default explicitly is no longer useful then. Sure, that approach sounds fine.
diff --git a/tests/checkasm/checkasm.c b/tests/checkasm/checkasm.c index 6318d9296b..668034c67f 100644 --- a/tests/checkasm/checkasm.c +++ b/tests/checkasm/checkasm.c @@ -23,8 +23,10 @@ #include "config.h" #include "config_components.h" -#ifndef _GNU_SOURCE -# define _GNU_SOURCE // for syscall (performance monitoring API), strsignal() +#if CONFIG_LINUX_PERF +# ifndef _GNU_SOURCE +# define _GNU_SOURCE // for syscall (performance monitoring API) +# endif #endif #include <signal.h> @@ -326,6 +328,7 @@ static struct { const char *cpu_flag_name; const char *test_name; int verbose; + int catch_signals; } state; /* PRNG state */ @@ -627,6 +630,64 @@ static CheckasmFunc *get_func(CheckasmFunc **root, const char *name) return f; } +checkasm_context checkasm_context_buf; + +/* Crash handling: attempt to catch crashes and handle them + * gracefully instead of just aborting abruptly. */ +#ifdef _WIN32 +#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP) +static LONG NTAPI signal_handler(EXCEPTION_POINTERS *const e) { + const char *err; + + if (!state.catch_signals) + return EXCEPTION_CONTINUE_SEARCH; + + switch (e->ExceptionRecord->ExceptionCode) { + case EXCEPTION_FLT_DIVIDE_BY_ZERO: + case EXCEPTION_INT_DIVIDE_BY_ZERO: + err = "fatal arithmetic error"; + break; + case EXCEPTION_ILLEGAL_INSTRUCTION: + case EXCEPTION_PRIV_INSTRUCTION: + err = "illegal instruction"; + break; + case EXCEPTION_ACCESS_VIOLATION: + case EXCEPTION_ARRAY_BOUNDS_EXCEEDED: + case EXCEPTION_DATATYPE_MISALIGNMENT: + case EXCEPTION_STACK_OVERFLOW: + err = "segmentation fault"; + break; + case EXCEPTION_IN_PAGE_ERROR: + err = "bus error"; + break; + default: + return EXCEPTION_CONTINUE_SEARCH; + } + state.catch_signals = 0; + checkasm_fail_func("%s", err); + checkasm_load_context(); + return EXCEPTION_CONTINUE_EXECUTION; /* never reached, but shuts up gcc */ +} +#endif +#else +static void signal_handler(const int s) { + if (state.catch_signals) { + state.catch_signals = 0; + checkasm_fail_func("%s", + s == SIGFPE ? "fatal arithmetic error" : + s == SIGILL ? "illegal instruction" : + s == SIGBUS ? "bus error" : + "segmentation fault"); + checkasm_load_context(); + } else { + /* fall back to the default signal handler */ + static const struct sigaction default_sa = { .sa_handler = SIG_DFL }; + sigaction(s, &default_sa, NULL); + raise(s); + } +} +#endif + /* Perform tests and benchmarks for the specified cpu flag if supported by the host */ static void check_cpu_flag(const char *name, int flag) { @@ -737,18 +798,24 @@ int main(int argc, char *argv[]) unsigned int seed = av_get_random_seed(); int i, ret = 0; +#ifdef _WIN32 +#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP) + AddVectoredExceptionHandler(0, signal_handler); +#endif +#else + const struct sigaction sa = { + .sa_handler = signal_handler, + .sa_flags = SA_NODEFER, + }; + sigaction(SIGBUS, &sa, NULL); + sigaction(SIGFPE, &sa, NULL); + sigaction(SIGILL, &sa, NULL); + sigaction(SIGSEGV, &sa, NULL); +#endif #if ARCH_ARM && HAVE_ARMV5TE_EXTERNAL if (have_vfp(av_get_cpu_flags()) || have_neon(av_get_cpu_flags())) checkasm_checked_call = checkasm_checked_call_vfp; #endif -#if ARCH_RISCV && HAVE_RV - struct sigaction act = { - .sa_handler = checkasm_handle_signal, - .sa_flags = 0, - }; - - sigaction(SIGILL, &act, NULL); -#endif if (!tests[0].func || !cpus[0].flag) { fprintf(stderr, "checkasm: no tests to perform\n"); @@ -876,15 +943,6 @@ void checkasm_fail_func(const char *msg, ...) } } -void checkasm_fail_signal(int signum) -{ -#ifdef __GLIBC__ - checkasm_fail_func("fatal signal %d: %s", signum, strsignal(signum)); -#else - checkasm_fail_func("fatal signal %d", signum); -#endif -} - /* Get the benchmark context of the current function */ CheckasmPerf *checkasm_get_perf_context(void) { @@ -932,6 +990,10 @@ void checkasm_report(const char *name, ...) } } +void checkasm_set_signal_handler_state(const int enabled) { + state.catch_signals = enabled; +} + #define DEF_CHECKASM_CHECK_FUNC(type, fmt) \ int checkasm_check_##type(const char *const file, const int line, \ const type *buf1, ptrdiff_t stride1, \ diff --git a/tests/checkasm/checkasm.h b/tests/checkasm/checkasm.h index e3c19510fa..3d1d16d5bb 100644 --- a/tests/checkasm/checkasm.h +++ b/tests/checkasm/checkasm.h @@ -23,7 +23,6 @@ #ifndef TESTS_CHECKASM_CHECKASM_H #define TESTS_CHECKASM_CHECKASM_H -#include <setjmp.h> #include <stdint.h> #include "config.h" @@ -43,6 +42,27 @@ #include "libavutil/lfg.h" #include "libavutil/timer.h" +#if !ARCH_X86_32 && defined(_WIN32) +/* setjmp/longjmp on Windows on architectures using SEH (all except x86_32) + * will try to use SEH to unwind the stack, which doesn't work for assembly + * functions without unwind information. */ +#include <windows.h> +#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP) +#define checkasm_context CONTEXT +#define checkasm_save_context() RtlCaptureContext(&checkasm_context_buf) +#define checkasm_load_context() RtlRestoreContext(&checkasm_context_buf, NULL) +#else +#define checkasm_context void* +#define checkasm_save_context() do {} while (0) +#define checkasm_load_context() do {} while (0) +#endif +#else +#include <setjmp.h> +#define checkasm_context jmp_buf +#define checkasm_save_context() setjmp(checkasm_context_buf) +#define checkasm_load_context() longjmp(checkasm_context_buf, 1) +#endif + void checkasm_check_aacencdsp(void); void checkasm_check_aacpsdsp(void); void checkasm_check_ac3dsp(void); @@ -105,9 +125,10 @@ struct CheckasmPerf; void *checkasm_check_func(void *func, const char *name, ...) av_printf_format(2, 3); int checkasm_bench_func(void); void checkasm_fail_func(const char *msg, ...) av_printf_format(1, 2); -void checkasm_fail_signal(int signum); struct CheckasmPerf *checkasm_get_perf_context(void); void checkasm_report(const char *name, ...) av_printf_format(1, 2); +void checkasm_set_signal_handler_state(int enabled); +extern checkasm_context checkasm_context_buf; /* float compare utilities */ int float_near_ulp(float a, float b, unsigned max_ulp); @@ -131,7 +152,7 @@ static av_unused void *func_ref, *func_new; #define BENCH_RUNS 1000 /* Trade-off between accuracy and speed */ /* Decide whether or not the specified function needs to be tested */ -#define check_func(func, ...) (func_ref = checkasm_check_func((func_new = func), __VA_ARGS__)) +#define check_func(func, ...) (checkasm_save_context(), func_ref = checkasm_check_func((func_new = func), __VA_ARGS__)) /* Declare the function prototype. The first argument is the return value, the remaining * arguments are the function parameters. Naming parameters is optional. */ @@ -146,7 +167,10 @@ static av_unused void *func_ref, *func_new; #define report checkasm_report /* Call the reference function */ -#define call_ref(...) ((func_type *)func_ref)(__VA_ARGS__) +#define call_ref(...)\ + (checkasm_set_signal_handler_state(1),\ + ((func_type *)func_ref)(__VA_ARGS__));\ + checkasm_set_signal_handler_state(0) #if ARCH_X86 && HAVE_X86ASM /* Verifies that clobbered callee-saved registers are properly saved and restored @@ -179,16 +203,22 @@ void checkasm_stack_clobber(uint64_t clobber, ...); ((cpu_flags) & av_get_cpu_flags()) ? (void *)checkasm_checked_call_emms : \ (void *)checkasm_checked_call; #define CLOB (UINT64_C(0xdeadbeefdeadbeef)) -#define call_new(...) (checkasm_stack_clobber(CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,\ +#define call_new(...) (checkasm_set_signal_handler_state(1),\ + checkasm_stack_clobber(CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,\ CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB),\ - checked_call(func_new, 0, 0, 0, 0, 0, __VA_ARGS__)) + checked_call(func_new, 0, 0, 0, 0, 0, __VA_ARGS__));\ + checkasm_set_signal_handler_state(0) #elif ARCH_X86_32 #define declare_new(ret, ...) ret (*checked_call)(void *, __VA_ARGS__) = (void *)checkasm_checked_call; #define declare_new_float(ret, ...) ret (*checked_call)(void *, __VA_ARGS__) = (void *)checkasm_checked_call_float; #define declare_new_emms(cpu_flags, ret, ...) ret (*checked_call)(void *, __VA_ARGS__) = \ ((cpu_flags) & av_get_cpu_flags()) ? (void *)checkasm_checked_call_emms : \ (void *)checkasm_checked_call; -#define call_new(...) checked_call(func_new, __VA_ARGS__) +#define call_new(...)\ + (checkasm_set_signal_handler_state(1),\ + checked_call(func_new, __VA_ARGS__, 15, 14, 13, 12,\ + 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1));\ + checkasm_set_signal_handler_state(0) #endif #elif ARCH_ARM && HAVE_ARMV5TE_EXTERNAL /* Use a dummy argument, to offset the real parameters by 2, not only 1. @@ -200,7 +230,10 @@ extern void (*checkasm_checked_call)(void *func, int dummy, ...); #define declare_new(ret, ...) ret (*checked_call)(void *, int dummy, __VA_ARGS__, \ int, int, int, int, int, int, int, int, \ int, int, int, int, int, int, int) = (void *)checkasm_checked_call; -#define call_new(...) checked_call(func_new, 0, __VA_ARGS__, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0, 0, 0, 0) +#define call_new(...) \ + (checkasm_set_signal_handler_state(1),\ + checked_call(func_new, 0, __VA_ARGS__, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0, 0, 0, 0));\ + checkasm_set_signal_handler_state(0) #elif ARCH_AARCH64 && !defined(__APPLE__) void checkasm_stack_clobber(uint64_t clobber, ...); void checkasm_checked_call(void *func, ...); @@ -209,35 +242,39 @@ void checkasm_checked_call(void *func, ...); int, int, int, int, int, int, int)\ = (void *)checkasm_checked_call; #define CLOB (UINT64_C(0xdeadbeefdeadbeef)) -#define call_new(...) (checkasm_stack_clobber(CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,\ +#define call_new(...) (checkasm_set_signal_handler_state(1),\ + checkasm_stack_clobber(CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,\ CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB,CLOB),\ checked_call(func_new, 0, 0, 0, 0, 0, 0, 0, __VA_ARGS__,\ - 7, 6, 5, 4, 3, 2, 1, 0, 0, 0, 0, 0, 0, 0, 0)) + 7, 6, 5, 4, 3, 2, 1, 0, 0, 0, 0, 0, 0, 0, 0));\ + checkasm_set_signal_handler_state(0) #elif ARCH_RISCV -void checkasm_set_function(void *, sigjmp_buf); +void checkasm_set_function(void *); void *checkasm_get_wrapper(void); -void checkasm_handle_signal(int signum); #if HAVE_RV && (__riscv_xlen == 64) && defined (__riscv_d) #define declare_new(ret, ...) \ - int checked_call_signum = 0; \ - sigjmp_buf checked_call_jb; \ ret (*checked_call)(__VA_ARGS__) = checkasm_get_wrapper(); #define call_new(...) \ - (checkasm_set_function(func_new, checked_call_jb), \ - (checked_call_signum = sigsetjmp(checked_call_jb, 1)) == 0 \ - ? checked_call(__VA_ARGS__) \ - : (checkasm_fail_signal(checked_call_signum), 0)) + (checkasm_set_signal_handler_state(1),\ + checkasm_set_function(func_new), checked_call(__VA_ARGS__));\ + checkasm_set_signal_handler_state(0) #else #define declare_new(ret, ...) -#define call_new(...) ((func_type *)func_new)(__VA_ARGS__) +#define call_new(...)\ + (checkasm_set_signal_handler_state(1),\ + ((func_type *)func_new)(__VA_ARGS__));\ + checkasm_set_signal_handler_state(0) #endif #else #define declare_new(ret, ...) #define declare_new_float(ret, ...) #define declare_new_emms(cpu_flags, ret, ...) /* Call the function */ -#define call_new(...) ((func_type *)func_new)(__VA_ARGS__) +#define call_new(...)\ + (checkasm_set_signal_handler_state(1),\ + ((func_type *)func_new)(__VA_ARGS__));\ + checkasm_set_signal_handler_state(0) #endif #ifndef declare_new_emms @@ -284,6 +321,7 @@ typedef struct CheckasmPerf { uint64_t tsum = 0;\ int ti, tcount = 0;\ uint64_t t = 0; \ + checkasm_set_signal_handler_state(1);\ for (ti = 0; ti < BENCH_RUNS; ti++) {\ PERF_START(t);\ tfunc(__VA_ARGS__);\ @@ -299,6 +337,7 @@ typedef struct CheckasmPerf { emms_c();\ perf->cycles += t;\ perf->iterations++;\ + checkasm_set_signal_handler_state(0);\ }\ } while (0) #else diff --git a/tests/checkasm/riscv/checkasm.S b/tests/checkasm/riscv/checkasm.S index 971d881157..73ca85f344 100644 --- a/tests/checkasm/riscv/checkasm.S +++ b/tests/checkasm/riscv/checkasm.S @@ -41,7 +41,6 @@ endconst checked_func: .quad 0 - .quad 0 saved_regs: /* Space to spill RA, SP, GP, TP, S0-S11 and FS0-FS11 */ @@ -53,7 +52,6 @@ func checkasm_set_function la.tls.ie t0, checked_func add t0, tp, t0 sd a0, (t0) - sd a1, 8(t0) ret endfunc @@ -177,14 +175,4 @@ func checkasm_get_wrapper, v call checkasm_fail_func j 4b endfunc - -func checkasm_handle_signal - mv a1, a0 - la.tls.ie a0, checked_func - add a0, tp, a0 - ld a0, 8(a0) - beqz a0, 8f - tail siglongjmp -8: tail abort /* No jump buffer to go to */ -endfunc #endif