Reset continue_after_async only if neither SIGINIT nor SIGSTOP received

[ovyalov]

Summary

Reset continue_after_async only if neither SIGINIT nor SIGSTOP received - otherwise it leads to stopped inferior when setting breakpoint (when m_interrupt_sent == true and signal is SIGSTOP).

[labath]

Does this fix an existing test or is a new issue? If it's new (it sounds like it is, as I don't see any test failures), could you also add a test for this. It shouldn't be too difficult to write one... Would something like this:

dbg.SetAsync(true)
process.Continue()
lldbutil.expect_state_changes(self,dbg.GetListener(), [eStateRunning])
target.BreakpointCreateBySourceRegex("// some code which will not get executed by the inferior")
do_some_assertions_on_the_breakpoint()
while dbg.GetListener().WaitForEvent(2, event):
  if SBProcess.GetStateFromEvent(event) == eStateStopped and SBProcess.GetRestartedFromEvent(state):
    continue
  if SBProcess.GetStateFromEvent(event) == eStateRunning:
    continue
  self.fail("Setting a breakpoint generated an unexpected event: %s"%SBDebugger.StateAsCString(SBProcess.GetStateFromEvent(event)));

do the trick?

[clayborg]

Looks good.

[ovyalov]

Added new TestBreakpointSetRestart test to cover the addressed issue.

[ovyalov]

In D18886#395216, @labath wrote:

Does this fix an existing test or is a new issue? If it's new (it sounds like it is, as I don't see any test failures), could you also add a test for this. It shouldn't be too difficult to write one... Would something like this:

dbg.SetAsync(true)
process.Continue()
lldbutil.expect_state_changes(self,dbg.GetListener(), [eStateRunning])
target.BreakpointCreateBySourceRegex("// some code which will not get executed by the inferior")
do_some_assertions_on_the_breakpoint()
while dbg.GetListener().WaitForEvent(2, event):
  if SBProcess.GetStateFromEvent(event) == eStateStopped and SBProcess.GetRestartedFromEvent(state):
    continue
  if SBProcess.GetStateFromEvent(event) == eStateRunning:
    continue
  self.fail("Setting a breakpoint generated an unexpected event: %s"%SBDebugger.StateAsCString(SBProcess.GetStateFromEvent(event)));

do the trick?

Thanks for suggested test, Pavel.

[ovyalov]

Submitted as http://reviews.llvm.org/rL265843

[amccarth]

FYI: According to git bisect, this patch seems to have introduced a new test failure on Windows.

[ovyalov]

In D18886#397637, @amccarth wrote:

FYI: According to git bisect, this patch seems to have introduced a new test failure on Windows.

Thanks for the report - will fix today.

[amccarth]

It's weird in that, if you run the test independently, it passes. But if you run it with the multiprocess test runner (ninja check-lldb), then it fails on this line:

self.fail("Setting a breakpoint generated an unexpected event: %s" % lldb.SBDebugger.StateAsCString(lldb.SBProcess.GetStateFromEvent(event)))

The state in this case is stopped.

[labath]

In D18886#397843, @amccarth wrote:

It's weird in that, if you run the test independently, it passes. But if you run it with the multiprocess test runner (ninja check-lldb), then it fails on this line:

self.fail("Setting a breakpoint generated an unexpected event: %s" % lldb.SBDebugger.StateAsCString(lldb.SBProcess.GetStateFromEvent(event)))

The state in this case is stopped.

I suspect this test has uncovered a problem in the ProcessWindows implementation. The same problem we were solving here for ProcessGdbRemote.

Namely, the question here is what happens if we try to set a breakpoint while the process is running. ProcessGdbRemote (and I expect ProcessWindows as well) needs to stop the inferior to be able to set the breakpoint. However, since this stop wasn't requested by the user, it should not generate any externally visible stops. The process should be silently resumed after you are done with it (I have no idea how is this supposed to be done with the non-gdb-remote processes).

I suspect the test is flaky for you because of different timings when running under heavy system load, but after the issue is fixed the test should pass 100% of time. Unfortunately, I think this task will be up to you. :)

[labath]

A random thought: will getchar() block the inferior on windows (because of missing stdio forwarding, et al.). If it wont then this could be the cause of the flakyness. If that's the case, then we can replace that call with something that will surely halt progress, like an infinite loop with sleep or something.

packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_set_restart/Makefile

+LEVEL = ../../../make
+
+CXX_SOURCES := main.cpp
+
+include $(LEVEL)/Makefile.rules

packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_set_restart/TestBreakpointSetRestart.py

+"""
+Test inferior restart when breakpoint is set on running target.
+"""
+
+import os
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class BreakpointSetRestart(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+    BREAKPOINT_TEXT = 'Set a breakpoint here'
+
+    def test_breakpoint_set_restart(self):
+        self.build()
+
+        cwd = self.get_process_working_directory()
+        exe = os.path.join(cwd, "a.out")
+        target = self.dbg.CreateTarget(exe)
+
+        self.dbg.SetAsync(True)
+        process = target.LaunchSimple(None, None, cwd)
+
+        lldbutil.expect_state_changes(self, self.dbg.GetListener(), [lldb.eStateRunning])
+        bp = target.BreakpointCreateBySourceRegex(self.BREAKPOINT_TEXT,
+                                                  lldb.SBFileSpec(os.path.join(cwd, 'main.cpp')))
+        self.assertTrue(bp.IsValid() and bp.GetNumLocations() == 1, VALID_BREAKPOINT)
+
+        event = lldb.SBEvent()
+        while self.dbg.GetListener().WaitForEvent(2, event):
+            if lldb.SBProcess.GetStateFromEvent(event) == lldb.eStateStopped and lldb.SBProcess.GetRestartedFromEvent(event):
+                continue
+            if lldb.SBProcess.GetStateFromEvent(event) == lldb.eStateRunning:
+                continue
+            self.fail("Setting a breakpoint generated an unexpected event: %s" % lldb.SBDebugger.StateAsCString(lldb.SBProcess.GetStateFromEvent(event)))
+

packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_set_restart/main.cpp

+//===-- main.cpp ------------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include <iostream>
+#include <stdio.h>
+
+int main(int argc, char const *argv[])
+{
+    getchar();
+    printf("Set a breakpoint here.\n");
+    return 0;
+}
+

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

                             // binaries that would send two stop replies anytime the process
                             // was interrupted, so we need to also check for an extra
                             // stop reply packet if we interrupted the process
-                            if (m_interrupt_sent || (signo != sigint_signo && signo != sigstop_signo))
+                            const bool received_nonstop_signal = signo != sigint_signo && signo != sigstop_signo;
+                            if (m_interrupt_sent || received_nonstop_signal)
                             {
-                                continue_after_async = false;
+                                if (received_nonstop_signal)
+                                    continue_after_async = false;
 
-                                // We didn't get a SIGINT or SIGSTOP, so try for a
-                                // very brief time (0.1s) to get another stop reply
+                                // Try for a very brief time (0.1s) to get another stop reply
                                 // packet to make sure it doesn't get in the way
                                 StringExtractorGDBRemote extra_stop_reply_packet;
                                 uint32_t timeout_usec = 100000;