[PATCH] D109506: [RFC] Print current request context along with the stack trance in clangd

[Emma Blink via Phabricator via llvm-commits]

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