i#38 attach: Make -skip_syscall the default

Makes interrupting a blocking syscall on attach the default behavior.
Removes the -skip_syscall drrun/drinject parameter.
Adds a test by adding an optional blocking syscall to infloop.

Issue: #38

api/docs/release.dox

@@ -126,7 +126,8 @@ clients.
 
 The changes between version \DR_VERSION and 9.0.1 include the following compatibility
 changes:
- - Nothing yet (placeholder).
+ - Eliminated the -skip_syscall option to drrun and drinject, which is now always
+   on by default.
 
 Further non-compatibility-affecting changes include:
  - Added AArchXX support for attaching to a running process.

suite/tests/CMakeLists.txt

@@ -1317,6 +1317,9 @@ function(torun test key source native standalone_dr dr_ops exe_ops added_out pas
         # is failing).
         set(exe_ops "${exe_ops};-v;-attach")
       endif ()
+      if ("${runall}" MATCHES "<block>")
+        set(exe_ops "${exe_ops};-block")
+      endif ()
     else ()
       if ("${runall}" MATCHES "<attach>")
         # For attach we use attachee, which is identical to infloop, aside for writing
@@ -2386,10 +2389,9 @@ if (ANNOTATIONS AND NOT ARM)
 endif (ANNOTATIONS AND NOT ARM)
 
 if (NOT ANDROID) # TODO i#38: Port test to Android.
-  # TODO i#38: Add targeted Linux tests of attaching during a blocking syscall.
-  # That seems to happen with the infloop test on ARM already (we have to add
-  # -skip_syscall in runall.cmake).
   tobuild_ci(client.attach_test client-interface/attach_test.runall "" "" "")
+  torunonly_ci(client.attach_blocking linux.infloop client.attach_test.dll
+    client-interface/attach_blocking.runall "" "" "")
 endif ()
 
 if (UNIX)

suite/tests/client-interface/attach_blocking.expect

@@ -0,0 +1,6 @@
+starting
+thank you for testing attach
+thread init
+select error: Interrupted system call
+blocking syscall returned -1 errno=4
+done

suite/tests/client-interface/attach_blocking.runall

@@ -0,0 +1 @@
+<attach><block>

suite/tests/linux/infloop.c

@@ -1,5 +1,5 @@
 /* **********************************************************
- * Copyright (c) 2011-2012 Google, Inc.  All rights reserved.
+ * Copyright (c) 2011-2022 Google, Inc.  All rights reserved.
  * Copyright (c) 2009-2010 VMware, Inc.  All rights reserved.
  * **********************************************************/
 
@@ -39,6 +39,11 @@
 #include <signal.h>
 #include <assert.h>
 #include <string.h>
+#include <errno.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <sys/select.h>
 
 static void
 signal_handler(int sig)
@@ -54,7 +59,7 @@ main(int argc, const char *argv[])
 {
     int arg_offs = 1;
     long long counter = 0;
-    bool for_attach = false;
+    bool for_attach = false, block = false;
     while (arg_offs < argc && argv[arg_offs][0] == '-') {
         if (strcmp(argv[arg_offs], "-v") == 0) {
             /* enough verbosity to satisfy runall.cmake: needs an initial and a
@@ -66,6 +71,9 @@ main(int argc, const char *argv[])
         } else if (strcmp(argv[arg_offs], "-attach") == 0) {
             for_attach = true;
             arg_offs++;
+        } else if (strcmp(argv[arg_offs], "-block") == 0) {
+            block = true;
+            arg_offs++;
         } else
             return 1;
     }
@@ -80,14 +88,32 @@ main(int argc, const char *argv[])
              */
             protect_mem((void *)signal_handler, 1, ALLOW_READ | ALLOW_EXEC);
         }
-        /* don't spin forever to avoid hosing machines if test harness somehow
+        /* Don't spin forever to avoid hosing machines if test harness somehow
          * fails to kill.  15 billion syscalls takes ~ 1 minute.
          */
         counter++;
         if (counter > 15 * 1024 * 1024 * 1024LL) {
             print("hit max iters\n");
             break;
         }
+        if (block) {
+            /* Make a blocking syscall, but not forever to again guard against
+             * a runaway test.
+             */
+            int res;
+            struct timeval timeout;
+            timeout.tv_sec = 60;
+            timeout.tv_usec = 0;
+            fd_set set;
+            FD_ZERO(&set);
+            FD_SET(0, &set);
+            res = select(0 + 1, &set, NULL, NULL, &timeout);
+            if (res == -1)
+                perror("select error");
+            else if (res == 0)
+                print("select timeout\n");
+            print("blocking syscall returned %d errno=%d\n", res, errno);
+        }
     }
     return 0;
 }

suite/tests/runall.cmake

@@ -197,22 +197,8 @@ if ("${nudge}" MATCHES "<use-persisted>")
   endif ()
 elseif ("${nudge}" MATCHES "<attach>")
   set(nudge_cmd run_in_bg)
-
-  set(extra_ops "")
-  if (UNIX)
-    # We're not yet explicitly testing -skip_syscall b/c it's considered experimental,
-    # but we seem to need it on ARM where infloop is in a blocking syscall.
-    # Not sure why it's different vs other arches.
-    # XXX i#38: Add additional explicit tests of blocking syscalls, and then turn
-    # -skip_syscall on by default all the time.
-    execute_process(COMMAND uname -m OUTPUT_VARIABLE HOST_ARCH)
-    if ("${HOST_ARCH}" MATCHES "^arm")
-      set(extra_ops "@-skip_syscall")
-    endif ()
-  endif ()
-
   string(REGEX REPLACE "<attach>"
-    "${toolbindir}/drrun@-attach@${pid}${extra_ops}@-takeover_sleep@-takeovers@1000"
+    "${toolbindir}/drrun@-attach@${pid}@-takeover_sleep@-takeovers@1000"
     nudge "${nudge}")
   string(REGEX REPLACE "@" ";" nudge "${nudge}")
   execute_process(COMMAND "${toolbindir}/${nudge_cmd}" ${nudge}

tools/drdeploy.c

@@ -302,13 +302,6 @@ const char *options_list_str =
     "       -attach <pid>      Attach to the process with the given pid.\n"
     "                          Attaching is an experimental feature and is not yet\n"
     "                          as well-supported as launching a new process.\n"
-    "                          When attaching to a process in the middle of a blocking\n"
-    "                          system call, DynamoRIO will wait until it returns.\n"
-    "                          Use -skip_syscall to force interruption.\n"
-    "       -skip_syscall      (Experimental)\n"
-    "                          Only works with -attach.\n"
-    "                          Attaching to a process will force blocking system calls\n"
-    "                          to fail with EINTR.\n"
 #    endif
 #    ifdef WINDOWS
     "       -attach <pid>      Attach to the process with the given pid.\n"
@@ -1192,7 +1185,6 @@ _tmain(int argc, TCHAR *targv[])
     time_t start_time, end_time;
 #    else
     bool use_ptrace = false;
-    bool wait_syscall = true;
     bool kill_group = false;
 #    endif
     process_id_t attach_pid = 0;
@@ -1379,12 +1371,6 @@ _tmain(int argc, TCHAR *targv[])
                              "-sleep_between_takeovers");
             continue;
         }
-#    ifdef UNIX
-        else if (strcmp(argv[i], "-skip_syscall") == 0) {
-            wait_syscall = false;
-            continue;
-        }
-#    endif
 #    ifdef UNIX
         else if (strcmp(argv[i], "-use_ptrace") == 0) {
             /* Undocumented option for using ptrace on a fresh process. */
@@ -1886,8 +1872,9 @@ _tmain(int argc, TCHAR *targv[])
         info("will exec %s", app_name);
         errcode = dr_inject_prepare_to_exec(app_name, app_argv, &inject_data);
     } else if (attach_pid != 0) {
-        errcode =
-            dr_inject_prepare_to_attach(attach_pid, app_name, wait_syscall, &inject_data);
+        /* We always try to avoid hanging on a blocked syscall. */
+        errcode = dr_inject_prepare_to_attach(attach_pid, app_name,
+                                              /*wait_syscall=*/false, &inject_data);
     } else
 #    elif defined(WINDOWS)
     if (attach_pid != 0) {
@@ -1978,8 +1965,6 @@ _tmain(int argc, TCHAR *targv[])
             goto error;
         } else {
             info("using ptrace to inject");
-            if (wait_syscall)
-                warn("using experimental attach feature; if it hangs, try -skip_syscall");
         }
     }
     if (kill_group) {