labath added a comment.
Yes, you're right that the current implementation of Editline::Interrupt is not
correct. It's kind of my fault since I added that locking code (in my defense,
the code was pretty messy to begin with). The implementation could indeed
deadlock if the signal is delivered on the thread which does the editline
reading. The reason why this does not happen in practice is probably because
the signal is indeed not delievered on the editline thread. Posix leaves this
behavior unspecified, but implementations have to actually make a choice, and I
believe most do it in such a way that they first try the main thread, and only
if that has the signal blocked, they try searching for other victims. The
problem could also be reproduced with an well-timed manual SIGINT, if the os
supports sending signals to a specific thread (linux does via non-standard
tgkill(2), I don't know about the rest).
Right now, I don't really remember whether that locking was solving any
specific problem or if it was a case of "well, this code is touching variables
that may be accessed concurrently, so I better lock it...". If I had to guess,
I would say it's the latter. The `fprintf` operation, and messing with
`m_editor_status` are indeed not safe to perform without additional
synchronization. But as we've established, a mutex is not a good method of
synchronization with a signal handler. There are several ways to fix this. The
fix will probably won't require touching a lot of code, but it will need to be
done with a steady hand. It will likely involve moving things out of the signal
handler (`fprintf("^C")` could easily be done in the main thread), making
things std::atomic/std::sig_atomic or blocking signals in some critical
sections. Most likely a combination of approaches would be needed.
So, anyway, I think it's fine to either keep the current form of the patch, do
it similarly to how SIGINT is handled (knowing that it's not fully posix
compliant, and hoping that someone will eventually make both things compliant),
or try to make everything posix compliant right now.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79654/new/
https://reviews.llvm.org/D79654
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
