0x1eaf added inline comments.
================
Comment at: clang-tools-extra/clangd/support/ThreadSignalHandler.cpp:22
+ ThreadSignalHandlerCallback *Callback = CurrentCallback.get();
+ // TODO: ignore non thread local signals like SIGTERM
+ if (Callback) {
----------------
sammccall wrote:
> I think we should do this already, probably just starting with an allowlist
> of `SIGBUS, SIGFPE, SIGILL, SIGSEGV` (these are signals that are delivered to
> the thread that triggered them per the linux docs).
>
> I'm also wondering how portable this assumption/mechanism is, e.g. whether we
> should enable it for certain platforms only. I guess it's probably true for
> unixy things, and apparently mostly true for windows too[1]? I'm a little
> concerned *something* may go wrong on windows, though.
>
> [1]
> https://stackoverflow.com/questions/15879342/which-thread-are-signal-handlers-e-g-signalsigint-crtl-c-called-on
I've just looked into adding filtering by signal number and there doesn't seem
to be a way to do this portably in LLVM. But it looks like it might be safe
enough to skip the filtering at all:
* From a cursory look at Windows `Signals.inc` it looks like the “signal”
handling is implemented via
[SetUnhandledExceptionFilter()](https://docs.microsoft.com/en-us/windows/win32/api/errhandlingapi/nf-errhandlingapi-setunhandledexceptionfilter)
which is thread-directed, and the `SetConsoleCtrlHandler()` callback [does not
call](https://reviews.llvm.org/source/llvm-github/browse/main/llvm/lib/Support/Windows/Signals.inc$843-846)
the registered handlers.
* And on Unix platforms the `AddSignalHandler()` callbacks are called
[only](https://reviews.llvm.org/source/llvm-github/browse/main/llvm/include/llvm/Support/Signals.h$62-64)
for
[KillSigs](https://reviews.llvm.org/source/llvm-github/browse/main/llvm/lib/Support/Unix/Signals.inc$217)
out of which only `SIGQUIT` seem to be process-directed and would be delivered
to [any thread](https://man7.org/linux/man-pages/man7/signal.7.html) that is
not blocking it, but if the thread gets delivered to has a
`ThreadCrashReporter` installed during the interrupt — it seems to make sense
to run the handler and let it print the thread-local context information (at
least there seems to be no harm in doing so).
What do you think?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109506/new/
https://reviews.llvm.org/D109506
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
