[lldb] [MainLoop] Add kevent() EINTR handling

[mgorny]

Summary

Add missing EINTR handling for kevent() calls. If the call is
interrupted, return from Poll() as if zero events were returned and let
the polling resume on next iteration. This fixes test flakiness
on NetBSD.

Includes a test case suggested by Pavel Labath on D42206.

[labath]

We already had a patch for this, but it kind of got forgotten while we were figuring out how to test it. I had made a test for the behavior here https://reviews.llvm.org/D42206#980045. Could you please try it out and incorporate it into the patch (or write some other kind of test for this fix)?

[krytarowski]

For EINTR we shall use llvm::sys::RetryAfterSignal

[mgorny]

In D58230#1398020, @krytarowski wrote:

For EINTR we shall use llvm::sys::RetryAfterSignal

kevent() man page indicates:

All changes contained in the changelist are applied before any pending events are read from the queue.

Also:

[EINTR] A signal was delivered before the timeout expired and before any events were placed on the kqueue for return.

So while it's not stated explicitly, I think in_events is always consumed, even if EINTR is returned. In which case, llvm::sys::RetryAfterSignal would be wrong whenever in_events is not empty.

[mgorny]

Added the test case.

[krytarowski]

In D58230#1398529, @mgorny wrote:

In D58230#1398020, @krytarowski wrote:

For EINTR we shall use llvm::sys::RetryAfterSignal

kevent() man page indicates:

All changes contained in the changelist are applied before any pending events are read from the queue.

Also:

[EINTR] A signal was delivered before the timeout expired and before any events were placed on the kqueue for return.

So while it's not stated explicitly, I think in_events is always consumed, even if EINTR is returned. In which case, llvm::sys::RetryAfterSignal would be wrong whenever in_events is not empty.

Please bring it to tech-kern@ to clear this and improve documentation. I think that we need to use llvm::sys::RetryAfterSignal.

[labath]

The change looks fine to me. I don't understand all the intricacies of the RetryAfterSignal discussion (AFAICT, it does not matter what you do there, since RunImpl::Poll is called in a loop anyway), so I'll let you sort that out with Kamil.

[krytarowski]

@mgorny let's go through tech-kern@ and later checking FreeBSD/Darwin/OpenBSD. I think it's worth to clarify this in the documentation.

As an intermediate version we can land this patch.

[mgorny]

FTR, I've just verified this against NetBSD kernel sources and -- as I suspected and as I believe any reasonable implementation would do -- the only place where EINTR could be returned is when kevent() is sleeping, waiting for new events to appear in the event queue. The changelist is always consumed.

lldb/trunk/source/Host/common/MainLoop.cpp

Status MainLoop::RunImpl::Poll() {
   in_events.resize(loop.m_read_fds.size());
   unsigned i = 0;
   for (auto &fd : loop.m_read_fds)
     EV_SET(&in_events[i++], fd.first, EVFILT_READ, EV_ADD, 0, 0, 0);
 
   num_events = kevent(loop.m_kqueue, in_events.data(), in_events.size(),
                       out_events, llvm::array_lengthof(out_events), nullptr);
 
-  if (num_events < 0)
+  if (num_events < 0) {
+    if (errno == EINTR) {
+      // in case of EINTR, let the main loop run one iteration
+      // we need to zero num_events to avoid assertions failing
+      num_events = 0;
+    } else
       return Status(errno, eErrorTypePOSIX);
+  }
   return Status();
 }
 
 void MainLoop::RunImpl::ProcessEvents() {
   assert(num_events >= 0);
   for (int i = 0; i < num_events; ++i) {
     if (loop.m_terminate_request)
       return;

lldb/trunk/unittests/Host/MainLoopTest.cpp

TEST_F(MainLoopTest, Signal) {
   Status error;
 
   auto handle = loop.RegisterSignal(SIGUSR1, make_callback(), error);
   ASSERT_TRUE(error.Success());
   kill(getpid(), SIGUSR1);
   ASSERT_TRUE(loop.Run().Success());
   ASSERT_EQ(1u, callback_count);
 }
+
+// Test that a signal which is not monitored by the MainLoop does not
+// cause a premature exit.
+TEST_F(MainLoopTest, UnmonitoredSignal) {
+  MainLoop loop;
+  Status error;
+  struct sigaction sa;
+  sa.sa_sigaction = [](int, siginfo_t *, void *) { };
+  sa.sa_flags = SA_SIGINFO; // important: no SA_RESTART
+  sigemptyset(&sa.sa_mask);
+  ASSERT_EQ(0, sigaction(SIGUSR2, &sa, nullptr));
+
+  auto handle = loop.RegisterSignal(SIGUSR1, make_callback(), error);
+  ASSERT_TRUE(error.Success());
+  std::thread killer([]() {
+    sleep(1);
+    kill(getpid(), SIGUSR2);
+    sleep(1);
+    kill(getpid(), SIGUSR1);
+  });
+  ASSERT_TRUE(loop.Run().Success());
+  killer.join();
+  ASSERT_EQ(1u, callback_count);
+}
 #endif