[NativeProcessLinux] Fix handling of SIGSTOP

[labath]

Summary

Previously, NPL tried to reinject SIGSTOP into the inferior in an attempt to get the process to
start in the group-stop state. This was:
a) wrong (reinjection should be controlled by "process handle" lldb setting)
b) racy (it should use Resume for transparent resuming instead of RequestResume)
c) broken (llgs crashed on inferior SIGSTOP)

With this change, SIGSTOP is handled just like any other signal delivered to the inferior: we
stop all threads and report signal reception to lldb. SIGSTOP reinjection does not behave the
same way as it would outside the debugger, but simulating this is a hard problem and is not
normally necessary.

[ovyalov]

Please see my comments.

test/functionalities/signal/raise/TestRaise.py

1 ↗

(On Diff #26058)

It will be great to make this test to verify different modes that "process handle" allows - e.g., with enabled/disabled debugger stop, ...

3 ↗

(On Diff #26058)

It seems time module isn't used.

10 ↗

(On Diff #26058)

Please verify the test remote invocation.

27 ↗

(On Diff #26058)

It seems no need to have this method just for delegation purposes.

47 ↗

(On Diff #26058)

Could you use process.GetUnixSignals().GetSignalNumberFromName('SIGSTOP') instead of signal.SIGSTOP - it matters when running test remotely on different platforms (for example, OSX <-> Linux) ?

[labath]

• Make the test more robust and extensive.

[labath]

Fixed the test suite, please take another look.

test/functionalities/signal/raise/TestRaise.py

1 ↗

(On Diff #26058)

Added tests for no-stop, notify and no-stop, no-notify modes.

3 ↗

(On Diff #26058)

Removed.

10 ↗

(On Diff #26058)

test passes linux->android

27 ↗

(On Diff #26058)

Removed.

47 ↗

(On Diff #26058)

Done, thanks.

[ovyalov]

Looks good.

lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp

if (err.Success())
         else
             MonitorSignal(&info, pid, exited);
     }
     else
     {
         if (err.GetError() == EINVAL)
         {
             // This is a group stop reception for this tid.
+            // We can reach here if we reinject SIGSTOP, SIGSTP, SIGTTIN or SIGTTOU into the
+            // tracee, triggering the group-stop mechanism. Normally receiving these would stop
+            // the process, pending a SIGCONT. Simulating this state in a debugger is hard and is
+            // generally not needed (one use case is debugging background task being managed by a
+            // shell). For general use, it is sufficient to stop the process in a signal-delivery
+            // stop which happens before the group stop. This done by MonitorSignal and works
+            // correctly for all signals.
             if (log)
-                log->Printf ("NativeProcessLinux::%s received a group stop for pid %" PRIu64 " tid %" PRIu64, __FUNCTION__, GetID (), pid);
-            ThreadDidStop(pid, false);
+                log->Printf("NativeProcessLinux::%s received a group stop for pid %" PRIu64 " tid %" PRIu64 ". Transparent handling of group stops not supported, resuming the thread.", __FUNCTION__, GetID (), pid);
+            Resume(pid, signal);
         }
         else
         {
             // ptrace(GETSIGINFO) failed (but not due to group-stop).
 
             // A return value of ESRCH means the thread/process is no longer on the system,
             // so it was killed somehow outside of our control.  Either way, we can't do anything
             // with it anymore.
NativeProcessLinux::MonitorSignal(const siginfo_t *info, lldb::pid_t pid, bool exited)
     if (log)
         log->Printf ("NativeProcessLinux::%s() received signal %s", __FUNCTION__, GetUnixSignals ().GetSignalAsCString (signo));
 
     // This thread is stopped.
     ThreadDidStop (pid, false);
 
     switch (signo)
     {
-    case SIGSTOP:
-        {
-            std::static_pointer_cast<NativeThreadLinux> (thread_sp)->SetStoppedBySignal (signo);
-            if (log)
-            {
-                if (is_from_llgs)
-                    log->Printf ("NativeProcessLinux::%s pid = %" PRIu64 " tid %" PRIu64 " received SIGSTOP from llgs, most likely an interrupt", __FUNCTION__, GetID (), pid);
-                else
-                    log->Printf ("NativeProcessLinux::%s pid = %" PRIu64 " tid %" PRIu64 " received SIGSTOP from outside of debugger", __FUNCTION__, GetID (), pid);
-            }
-
-            // Resume this thread to get the group-stop mechanism to fire off the true group stops.
-            // This thread will get stopped again as part of the group-stop completion.
-            ResumeThread(pid,
-                    [=](lldb::tid_t tid_to_resume, bool supress_signal)
-                    {
-                        std::static_pointer_cast<NativeThreadLinux> (thread_sp)->SetRunning ();
-                        // Pass this signal number on to the inferior to handle.
-                        return Resume (tid_to_resume, (supress_signal) ? LLDB_INVALID_SIGNAL_NUMBER : signo);
-                    },
-                    true);
-        }
-        break;
     case SIGSEGV:
     case SIGILL:
     case SIGFPE:
     case SIGBUS:
         if (thread_sp)
             std::static_pointer_cast<NativeThreadLinux> (thread_sp)->SetCrashedWithException (*info);
         break;
     default:

lldb/trunk/test/functionalities/signal/raise/Makefile

+LEVEL = ../../../make
+
+C_SOURCES := main.c
+
+include $(LEVEL)/Makefile.rules

lldb/trunk/test/functionalities/signal/raise/TestRaise.py

+"""Test that we handle inferiors that send signals to themselves"""
+
+import os
+import unittest2
+import lldb
+from lldbtest import *
+import lldbutil
+
+
+class RaiseTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @skipIfWindows # signals do not exist on Windows
+    @skipUnlessDarwin
+    @dsym_test
+    def test_sigstop_with_dsym(self):
+        self.buildDsym()
+        self.sigstop()
+
+    @skipIfWindows # signals do not exist on Windows
+    @dwarf_test
+    def test_sigstop_with_dwarf(self):
+        self.buildDwarf()
+        self.sigstop()
+
+    def launch(self, target):
+        # launch the process, do not stop at entry point.
+        process = target.LaunchSimple(['SIGSTOP'], None, self.get_process_working_directory())
+        self.assertTrue(process, PROCESS_IS_VALID)
+        self.assertEqual(process.GetState(), lldb.eStateStopped)
+        thread = lldbutil.get_stopped_thread(process, lldb.eStopReasonBreakpoint)
+        self.assertTrue(thread.IsValid(), "Thread should be stopped due to a breakpoint")
+        return process
+
+    def set_handle(self, signal, stop_at_signal, pass_signal, notify_signal):
+        return_obj = lldb.SBCommandReturnObject()
+        self.dbg.GetCommandInterpreter().HandleCommand(
+                "process handle %s -s %d -p %d -n %d" % (signal, stop_at_signal, pass_signal, notify_signal),
+                return_obj)
+        self.assertTrue (return_obj.Succeeded() == True, "Setting signal handling failed")
+
+
+    def sigstop(self):
+        """Test that we handle inferior raising SIGSTOP"""
+        exe = os.path.join(os.getcwd(), "a.out")
+
+        # Create a target by the debugger.
+        target = self.dbg.CreateTarget(exe)
+        self.assertTrue(target, VALID_TARGET)
+        lldbutil.run_break_set_by_symbol(self, "main")
+
+        # launch
+        process = self.launch(target)
+
+        # Make sure we stop at the signal
+        self.set_handle("SIGSTOP", 1, 0, 1)
+        process.Continue()
+        thread = lldbutil.get_stopped_thread(process, lldb.eStopReasonSignal)
+        self.assertTrue(thread.IsValid(), "Thread should be stopped due to a signal")
+        self.assertTrue(thread.GetStopReasonDataCount() >= 1, "There was data in the event.")
+        self.assertEqual(thread.GetStopReasonDataAtIndex(0),
+                process.GetUnixSignals().GetSignalNumberFromName('SIGSTOP'),
+                "The stop signal was SIGSTOP")
+
+        # Continue until we exit.
+        process.Continue()
+        self.assertEqual(process.GetState(), lldb.eStateExited)
+
+        # launch again
+        process = self.launch(target)
+
+        # Make sure we do not stop at the signal. We should still get the notification.
+        self.set_handle("SIGSTOP", 0, 0, 1)
+        self.expect("process continue", substrs=["stopped and restarted", "SIGSTOP"])
+        self.assertEqual(process.GetState(), lldb.eStateExited)
+
+        # launch again
+        process = self.launch(target)
+
+        # Make sure we do not stop at the signal, and we do not get the notification.
+        self.set_handle("SIGSTOP", 0, 0, 0)
+        self.expect("process continue", substrs=["stopped and restarted"], matching=False)
+        self.assertEqual(process.GetState(), lldb.eStateExited)
+
+        # passing of SIGSTOP is not correctly handled, so not testing that scenario: https://llvm.org/bugs/show_bug.cgi?id=23574
+
+        # reset signal handling to default
+        self.set_handle("SIGSTOP", 1, 0, 1)
+
+if __name__ == '__main__':
+    import atexit
+    lldb.SBDebugger.Initialize()
+    atexit.register(lambda: lldb.SBDebugger.Terminate())
+    unittest2.main()

lldb/trunk/test/functionalities/signal/raise/main.c

+#include <signal.h>
+#include <stdio.h>
+#include <string.h>
+
+int main (int argc, char *argv[])
+{
+    if (argc < 2)
+    {
+        puts("Please specify a signal to raise");
+        return 1;
+    }
+
+    if (strcmp(argv[1], "SIGSTOP") == 0)
+        raise(SIGSTOP);
+    else
+    {
+        printf("Unknown signal: %s\n", argv[1]);
+        return 2;
+    }
+
+    return 0;
+}
+