tsan: fix PTRACE_ATTACH handling during stop-the-world

[dvyukov]

Summary

If the thread receives a signal concurrently with PTRACE_ATTACH,
we can get notification about the signal before notification about stop.
In such case we need to forward the signal to the thread, otherwise
the signal will be missed (as we do PTRACE_DETACH with arg=0) and
any logic relying on signals will break. After forwarding we need to
continue to wait for stopping, because the thread is not stopped yet.
We do ignore delivery of SIGSTOP, because we want to make stop-the-world
as invisible as possible.

[earthdok]

lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc

114

We do actually want to stop the threads, though.

I think you misunderstood this part:

The usual practice is to reinject these signals until SIGSTOP is seen, then suppress SIGSTOP injection.

This is "usual practice" for when you want the threads to keep running.

119

passing uptr* in place of int*

125

indent

130

How about this part:

Signal-delivery-stop is observed by the tracer as waitpid(2) returning with WIFSTOPPED(status) true, with the signal returned by WSTOPSIG(status). If the signal is SIGTRAP, this may be a different kind of ptrace-stop; see the "Syscall-stops" and "execve" sections below for details. If WSTOPSIG(status) returns a stopping signal, this may be a group-stop; see below.

and

The fact that signal injection requests may be ignored when restarting the tracee after ptrace stops that are not signal-delivery-stops is a cause of confusion among ptrace users. One typical scenario is that the tracer observes group-stop, mistakes it for signal-delivery-stop, restarts the tracee with
ptrace(PTRACE_restart, pid, 0, stopsig)
with the intention of injecting stopsig, but stopsig gets ignored and the tracee continues to run.

test/tsan/signal_segv_handler.cc

35–36

don't need this anymore

[dvyukov]

lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc

114

Why do you think that my code does not stop threads?
Or what do you suggest? I don't understand.

[dvyukov]

lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc

119

status has type int

125

it is 4 spaces aligned
in accordance with the Style

130

And what is about this? Please be more concrete.

test/tsan/signal_segv_handler.cc

35–36

done

[dvyukov]

removed done label

[earthdok]

lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc

114

nevermind, I was reading it wrong

119

oops

125

nope, that's only for those cases where even the first parameter is wrapped
otherwise you align to the first parameter

130

Nevermind

[dvyukov]

lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc

125

... and what if the wrapped argument does not fit onto line if you align it to the first arg?...
ok, as you wish, done
the style guide instruction that covers this seems to be "follow the style of existing code"

[dvyukov]

Committed revision 229832

[earthdok]

lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc

125

and what if the wrapped argument does not fit onto line if you align it to the first arg?

Then you wrap at the opening bracket and indent with 4 spaces. It's pretty clear:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Calls

Either write the call all on a single line, wrap the arguments at the parenthesis, or start the arguments on a new line indented by four spaces and continue at that 4 space indent. In the absence of other considerations, use the minimum number of lines, including placing multiple arguments on each line where appropriate.

Basically you can choose between

foobar(a,
       b)

and

foobar(
    a, b)

or

foobar(
    a,
    b)

But nowhere does it mention the middle ground of

foobar(a,
    b)

lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc

SuspendedThreadsList &suspended_threads_list() {
     return suspended_threads_list_;
   }
  private:
   SuspendedThreadsList suspended_threads_list_;
   pid_t pid_;
   bool SuspendThread(SuspendedThreadID thread_id);
 };
 
-bool ThreadSuspender::SuspendThread(SuspendedThreadID thread_id) {
+bool ThreadSuspender::SuspendThread(SuspendedThreadID tid) {
   // Are we already attached to this thread?
   // Currently this check takes linear time, however the number of threads is
   // usually small.
-  if (suspended_threads_list_.Contains(thread_id))
+  if (suspended_threads_list_.Contains(tid))
     return false;
   int pterrno;
-  if (internal_iserror(internal_ptrace(PTRACE_ATTACH, thread_id, NULL, NULL),
+  if (internal_iserror(internal_ptrace(PTRACE_ATTACH, tid, NULL, NULL),
                        &pterrno)) {
     // Either the thread is dead, or something prevented us from attaching.
     // Log this event and move on.
-    VReport(1, "Could not attach to thread %d (errno %d).\n", thread_id,
-            pterrno);
+    VReport(1, "Could not attach to thread %d (errno %d).\n", tid, pterrno);
     return false;
   } else {
-    VReport(1, "Attached to thread %d.\n", thread_id);
+    VReport(1, "Attached to thread %d.\n", tid);
     // The thread is not guaranteed to stop before ptrace returns, so we must
-    // wait on it.
+    // wait on it. Note: if the thread receives a signal concurrently,
+    // we can get notification about the signal before notification about stop.
+    // In such case we need to forward the signal to the thread, otherwise
+    // the signal will be missed (as we do PTRACE_DETACH with arg=0) and
+    // any logic relying on signals will break. After forwarding we need to
+    // continue to wait for stopping, because the thread is not stopped yet.
+    // We do ignore delivery of SIGSTOP, because we want to make stop-the-world
+    // as invisible as possible.
+    for (;;) {
+      int status;
       uptr waitpid_status;
-    HANDLE_EINTR(waitpid_status, internal_waitpid(thread_id, NULL, __WALL));
+      HANDLE_EINTR(waitpid_status, internal_waitpid(tid, &status, __WALL));
       int wperrno;
       if (internal_iserror(waitpid_status, &wperrno)) {
         // Got a ECHILD error. I don't think this situation is possible, but it
         // doesn't hurt to report it.
         VReport(1, "Waiting on thread %d failed, detaching (errno %d).\n",
-              thread_id, wperrno);
-      internal_ptrace(PTRACE_DETACH, thread_id, NULL, NULL);
+            tid, wperrno);
+        internal_ptrace(PTRACE_DETACH, tid, NULL, NULL);
         return false;
       }
-    suspended_threads_list_.Append(thread_id);
+      if (WIFSTOPPED(status) && WSTOPSIG(status) != SIGSTOP) {
+        internal_ptrace(PTRACE_CONT, tid, 0, (void*)(uptr)WSTOPSIG(status));
+        continue;
+      }
+      break;
+    }
+    suspended_threads_list_.Append(tid);
     return true;
   }
 }
 
 void ThreadSuspender::ResumeAllThreads() {
   for (uptr i = 0; i < suspended_threads_list_.thread_count(); i++) {
     pid_t tid = suspended_threads_list_.GetThreadID(i);
     int pterrno;

test/tsan/signal_segv_handler.cc

 
 void *guard;
 
 void handler(int signo, siginfo_t *info, void *uctx) {
   mprotect(guard, 4096, PROT_READ | PROT_WRITE);
 }
 
 int main() {
-  // Disabled for now, because there is another issue in ptrace.
-  // It will be addressed in a subsequent change.
-  goto done;
   struct sigaction a;
   a.sa_sigaction = handler;
   a.sa_flags = SA_SIGINFO;
   sigaction(SIGSEGV, &a, 0);
   guard = mmap(0, 4096, PROT_NONE, MAP_ANON | MAP_PRIVATE, -1, 0);
   for (int i = 0; i < 1000000; i++) {
     mprotect(guard, 4096, PROT_NONE);
     *(int*)guard = 1;
   }
- done:
   fprintf(stderr, "DONE\n");
 }
 
 // CHECK: DONE