[Lldb-commits] [lldb] [lldb] Handle SIGINT via the MainLoop signal thread (on POSIX) (PR #195959)

[Pavel Labath via lldb-commits]

labath wrote:

> > I'm wondering if we could avoid grabbing another signal for this (SIGURG could in theory be used by some other part of the application). I think the same effect could be achieved by sending another SIGINT to the main thread (obviously, after we've restored the handler) -- and then setting a flag to ignore the next callback invocation. WDYT?
> 
> Wouldn't that be race-y with a real external SIGINT, which would look identical to the self-sent one? I guess we could check `siginfo_t.si_pid` in the MainLoop to ignore it if it's our own PID? Let me know if you prefer this over the `SIGURG` approach.

That's kind of true, but the current MainLoop implementation already does not guarantee you'll get a separate notification for each signal. Neither does the kernel (linux kernel, at least), for that matter. So like, if the user presses ^C twice really quickly (before we manage to run the signal callback), the second signal will not have any effect. That's why (pthread_)sigqueue exists, which we could theoretically use, but that would be pretty messy, so I wouldn't recommend it.

I guess my question is, why do you think the race is a problem. If the second ^C arrives while we're inside `DispatchInputInterrupt`, the process will get killed -- same as with this implementation. And if it arrives later (after we've restored the handler), then it will just cause us to run `DispatchInputInterrupt`, which shouldn't do anything (right?). 



> My experience with a library installing signal handlers has been nothing but disastrous, so I'd be okay with preserving the current behavior and limitations to avoid that.

SGTM

> The crash I'm looking at is a bit more involved than what I described in the PR description. I don't think what I'm seeing is the problem you're describing. This is what the crashlog looks like:

Oh I see. The signal arrives between fork and execve. That makes sense, although I'm surprised that you can reproduce this reliably, as I would have expected this window to be very small. But then again, libSystem also appears do be doing a lot more post-fork work than I would have expected. This patch fixes that by making the signal handler innocuous in that context.

BTW, does this mean that `DebugSymbols` is not using posix_spawn to launch the new process?

https://github.com/llvm/llvm-project/pull/195959