[Lldb-commits] [PATCH] D79654: [lldb/Driver] Support terminal resizing

[Pavel Labath via Phabricator via lldb-commits]

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