Author: Pavel Labath Date: 2022-03-09T14:31:17+01:00 New Revision: 4bcadd66864bf4af929ac8753a51d7ad408cdef0
URL: https://github.com/llvm/llvm-project/commit/4bcadd66864bf4af929ac8753a51d7ad408cdef0 DIFF: https://github.com/llvm/llvm-project/commit/4bcadd66864bf4af929ac8753a51d7ad408cdef0.diff LOG: [lldb/driver] Fix SIGTSTP handling Our SIGTSTP handler was working, but that was mostly accidental. The reason it worked is because lldb is multithreaded for most of its lifetime and the OS is reasonably fast at responding to signals. So, what happened was that the kill(SIGTSTP) which we sent from inside the handler was delivered to another thread while the handler was still set to SIG_DFL (which then correctly put the entire process to sleep). Sometimes it happened that the other thread got the second signal after the first thread had already restored the handler, in which case the signal handler would run again, and it would again attempt to send the SIGTSTP signal back to itself. Normally it didn't take many iterations for the signal to be delivered quickly enough. However, if you were unlucky (or were playing around with pexpect) you could get SIGTSTP while lldb was single-threaded, and in that case, lldb would go into an endless loop because the second SIGTSTP could only be handled on the main thread, and only after the handler for the first signal returned (and re-installed itself). In that situation the handler would keep re-sending the signal to itself. This patch fixes the issue by implementing the handler the way it supposed to be done: - before sending the second SIGTSTP, we unblock the signal (it gets automatically blocked upon entering the handler) - we use raise to send the signal, which makes sure it gets delivered to the thread which is running the handler This also means we don't need the SIGCONT handler, as our TSTP handler resumes right after the entire process is continued, and we can do the required work there. I also include a test case for the SIGTSTP flow. It uses pexpect, but it includes a couple of extra twists. Specifically, I needed to create an extra process on top of lldb, which will run lldb in a separate process group and simulate the role of the shell. This is needed because SIGTSTP is not effective on a session leader (the signal gets delivered, but it does not cause a stop) -- normally there isn't anyone to notice the stop. Differential Revision: https://reviews.llvm.org/D120320 Added: lldb/test/API/driver/job_control/TestJobControl.py lldb/test/API/driver/job_control/shell.py Modified: lldb/packages/Python/lldbsuite/test/lldbpexpect.py lldb/tools/driver/Driver.cpp Removed: ################################################################################ diff --git a/lldb/packages/Python/lldbsuite/test/lldbpexpect.py b/lldb/packages/Python/lldbsuite/test/lldbpexpect.py index d3e3e8dde5d84..84a0d61d8bddb 100644 --- a/lldb/packages/Python/lldbsuite/test/lldbpexpect.py +++ b/lldb/packages/Python/lldbsuite/test/lldbpexpect.py @@ -23,11 +23,15 @@ class PExpectTest(TestBase): def expect_prompt(self): self.child.expect_exact(self.PROMPT) - def launch(self, executable=None, extra_args=None, timeout=60, dimensions=None): + def launch(self, executable=None, extra_args=None, timeout=60, + dimensions=None, run_under=None, post_spawn=None): logfile = getattr(sys.stdout, 'buffer', sys.stdout) if self.TraceOn() else None - args = ['--no-lldbinit', '--no-use-colors'] + args = [] + if run_under is not None: + args += run_under + args += [lldbtest_config.lldbExec, '--no-lldbinit', '--no-use-colors'] for cmd in self.setUpCommands(): args += ['-O', cmd] if executable is not None: @@ -41,8 +45,11 @@ def launch(self, executable=None, extra_args=None, timeout=60, dimensions=None): import pexpect self.child = pexpect.spawn( - lldbtest_config.lldbExec, args=args, logfile=logfile, + args[0], args=args[1:], logfile=logfile, timeout=timeout, dimensions=dimensions, env=env) + + if post_spawn is not None: + post_spawn() self.expect_prompt() for cmd in self.setUpCommands(): self.child.expect_exact(cmd) diff --git a/lldb/test/API/driver/job_control/TestJobControl.py b/lldb/test/API/driver/job_control/TestJobControl.py new file mode 100644 index 0000000000000..4faaaf0a9edca --- /dev/null +++ b/lldb/test/API/driver/job_control/TestJobControl.py @@ -0,0 +1,32 @@ +""" +Test lldb's handling of job control signals (SIGTSTP, SIGCONT). +""" + + +from lldbsuite.test.lldbtest import * +from lldbsuite.test.lldbpexpect import PExpectTest + + +class JobControlTest(PExpectTest): + + mydir = TestBase.compute_mydir(__file__) + + def test_job_control(self): + def post_spawn(): + self.child.expect("PID=([0-9]+)") + self.lldb_pid = int(self.child.match[1]) + + run_under = [sys.executable, self.getSourcePath('shell.py')] + self.launch(run_under=run_under, post_spawn=post_spawn) + + os.kill(self.lldb_pid, signal.SIGTSTP) + self.child.expect("STATUS=([0-9]+)") + status = int(self.child.match[1]) + + self.assertTrue(os.WIFSTOPPED(status)) + self.assertEquals(os.WSTOPSIG(status), signal.SIGTSTP) + + os.kill(self.lldb_pid, signal.SIGCONT) + + self.child.sendline("quit") + self.child.expect("RETURNCODE=0") diff --git a/lldb/test/API/driver/job_control/shell.py b/lldb/test/API/driver/job_control/shell.py new file mode 100644 index 0000000000000..678457e119c31 --- /dev/null +++ b/lldb/test/API/driver/job_control/shell.py @@ -0,0 +1,34 @@ +""" +Launch a process (given through argv) similar to how a shell would do it. +""" + +import signal +import subprocess +import sys +import os + + +def preexec_fn(): + # Block SIGTTOU generated by the tcsetpgrp call + orig_mask = signal.pthread_sigmask(signal.SIG_BLOCK, [signal.SIGTTOU]) + + # Put us in a new process group. + os.setpgid(0, 0) + + # And put it in the foreground. + fd = os.open("/dev/tty", os.O_RDONLY) + os.tcsetpgrp(fd, os.getpgid(0)) + os.close(fd) + + signal.pthread_sigmask(signal.SIG_SETMASK, orig_mask) + + +if __name__ == "__main__": + child = subprocess.Popen(sys.argv[1:], preexec_fn=preexec_fn) + print("PID=%d" % child.pid) + + _, status = os.waitpid(child.pid, os.WUNTRACED) + print("STATUS=%d" % status) + + returncode = child.wait() + print("RETURNCODE=%d" % returncode) diff --git a/lldb/tools/driver/Driver.cpp b/lldb/tools/driver/Driver.cpp index 80c2ec67314b9..cf396c37fd3a9 100644 --- a/lldb/tools/driver/Driver.cpp +++ b/lldb/tools/driver/Driver.cpp @@ -671,23 +671,30 @@ void sigint_handler(int signo) { _exit(signo); } -void sigtstp_handler(int signo) { +#ifndef _WIN32 +static void sigtstp_handler(int signo) { if (g_driver != nullptr) g_driver->GetDebugger().SaveInputTerminalState(); + // Unblock the signal and remove our handler. + sigset_t set; + sigemptyset(&set); + sigaddset(&set, signo); + pthread_sigmask(SIG_UNBLOCK, &set, nullptr); signal(signo, SIG_DFL); - kill(getpid(), signo); + + // Now re-raise the signal. We will immediately suspend... + raise(signo); + // ... and resume after a SIGCONT. + + // Now undo the modifications. + pthread_sigmask(SIG_BLOCK, &set, nullptr); signal(signo, sigtstp_handler); -} -void sigcont_handler(int signo) { if (g_driver != nullptr) g_driver->GetDebugger().RestoreInputTerminalState(); - - signal(signo, SIG_DFL); - kill(getpid(), signo); - signal(signo, sigcont_handler); } +#endif static void printHelp(LLDBOptTable &table, llvm::StringRef tool_name) { std::string usage_str = tool_name.str() + " [options]"; @@ -826,7 +833,6 @@ int main(int argc, char const *argv[]) { signal(SIGPIPE, SIG_IGN); signal(SIGWINCH, sigwinch_handler); signal(SIGTSTP, sigtstp_handler); - signal(SIGCONT, sigcont_handler); #endif int exit_code = 0; _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits