[PATCH 2/2] remove tcp->parent and TCB_CLONE_THREAD

[Denys V. <dvl...@re...>]

On Sat, 2011-06-25 at 00:56 +0200, Denys Vlasenko wrote:
> But after "do not detach when we think tracee is going to die" patch,
> we DO NOT NEED parent<->child link for anything. Therefore
> we do not need suspending too. Bingo!
> 
> This patch removes suspending code. Now new tracees will be continued
> right away. Next patch will remove tcp->parent member.

Here is it.

tcp->parent is used for only two things:
(1) to send signal on detach via tgkill (need to know tgid). Solution: use tkill,
it needs only tid.
(2) to optimize out ptrace options setting for new tracees.
Not a big deal if we drop this optimization: "set options" op is fast,
doing it just one extra time once per each tracee is hardly measurable.

TCB_CLONE_THREAD is a misnomer. It used only to flag sibling we attached to
in startup_attach. This is used to prevent infinite recursive rescanning
of /proc/PID/task.
Despite the name, there is no guarantee it is set only on non-leader:
if one would run "strace -f -p THREAD_ID" and THREAD_ID is _not_
a thread leader, strace will happily attach to it and all siblings
and will think that THREAD_ID is the leader! Which is a bug, but
if "do not detach when we think tracee is going to die" patch will be applied,
it no longer matters, because we do not use the knowledge about thread
group leaders for anything. (We used it to delay leader's exit,
and to send signals with tgkill (see above)).

This patch still uses a flag bit to prevent /proc/PID/task recursion.
But since this bit is used ONLY in startup_attach, now code simply
hijacks another bit for that (only in that function),
and doesn't pollute defs.h namespace with a globally-defined flag bit.
If this is deemed to be ugly, we can still define the bit globally,
but we will still need to rename it from TCB_CLONE_THREAD to, say,
TCB_ATTACH_DONE.



IOW: after this patch strace has no need to know about threads, parents
and children, and so on. Therefore it does not track that information.
It treats all tracees as independent entities. Overall,
this simplifies code a lot.

Please review.

-- 
vda
 

diff -d -urpN strace.8/defs.h strace.9/defs.h
--- strace.8/defs.h	2011-06-24 22:45:15.040890245 +0200
+++ strace.9/defs.h	2011-06-24 22:45:25.529858623 +0200
@@ -366,7 +366,6 @@ struct tcb {
 	struct timeval dtime;	/* Delta for system time usage */
 	struct timeval etime;	/* Syscall entry time */
 				/* Support for tracing forked processes */
-	struct tcb *parent;	/* Parent of this process */
 	long baddr;		/* `Breakpoint' address */
 	long inst[2];		/* Instructions on above */
 	int pfd;		/* proc file descriptor */
@@ -407,7 +406,6 @@ struct tcb {
   || defined(ARM) || defined(MIPS) || defined(BFIN) || defined(TILE)
 #  define TCB_WAITEXECVE 04000	/* ignore SIGTRAP after exceve */
 # endif
-# define TCB_CLONE_THREAD  010000 /* CLONE_THREAD set in creating syscall */
 # include <sys/syscall.h>
 # ifndef __NR_exit_group
 # /* Hack: Most headers around are too old to have __NR_exit_group.  */
diff -d -urpN strace.8/process.c strace.9/process.c
--- strace.8/process.c	2011-06-24 02:16:01.492073781 +0200
+++ strace.9/process.c	2011-06-24 22:18:28.392514721 +0200
@@ -866,7 +866,6 @@ internal_fork(struct tcb *tcp)
 			memcpy(tcpchild->inst, tcp->inst,
 				sizeof tcpchild->inst);
 		}
-		tcpchild->parent = tcp;
 		if (!qflag)
 			fprintf(stderr, "Process %d attached\n", pid);
 	}
diff -d -urpN strace.8/strace.c strace.9/strace.c
--- strace.8/strace.c	2011-06-24 22:45:15.041890160 +0200
+++ strace.9/strace.c	2011-06-24 22:45:25.530858613 +0200
@@ -49,16 +49,14 @@
 
 #ifdef LINUX
 # include <asm/unistd.h>
-# if defined __NR_tgkill
-#  define my_tgkill(pid, tid, sig) syscall(__NR_tgkill, (pid), (tid), (sig))
-# elif defined __NR_tkill
-#  define my_tgkill(pid, tid, sig) syscall(__NR_tkill, (tid), (sig))
+# if defined __NR_tkill
+#  define my_tkill(tid, sig) syscall(__NR_tkill, (tid), (sig))
 # else
    /* kill() may choose arbitrarily the target task of the process group
       while we later wait on a that specific TID.  PID process waits become
       TID task specific waits for a process under ptrace(2).  */
 #  warning "Neither tkill(2) nor tgkill(2) available, risk of strace hangs!"
-#  define my_tgkill(pid, tid, sig) kill((tid), (sig))
+#  define my_tkill(tid, sig) kill((tid), (sig))
 # endif
 #endif
 
@@ -434,14 +432,20 @@ startup_attach(void)
 		strace_tracer_pid = getpid();
 	}
 
+#ifdef LINUX
+/* Temporarily hijack this bit to mark tcb's we already attached */
+# define TCB_ATTACH_DONE TCB_INSYSCALL
+#else
+# define TCB_ATTACH_DONE 0
+#endif
 	for (tcbi = 0; tcbi < tcbtabsize; tcbi++) {
 		tcp = tcbtab[tcbi];
 		if (!(tcp->flags & TCB_INUSE) || !(tcp->flags & TCB_ATTACHED))
 			continue;
-#ifdef LINUX
-		if (tcp->flags & TCB_CLONE_THREAD)
+
+		if (tcp->flags & TCB_ATTACH_DONE)
 			continue;
-#endif
+
 		/* Reinitialize the output since it may have changed. */
 		tcp->outf = outf;
 		newoutf(tcp);
@@ -481,14 +485,13 @@ startup_attach(void)
 							fprintf(stderr, "attach to pid %d succeeded\n", tid);
 						if (tid != tcbtab[tcbi]->pid) {
 							tcp = alloctcb(tid);
-							tcp->flags |= TCB_ATTACHED|TCB_CLONE_THREAD;
-							tcp->parent = tcbtab[tcbi];
+							tcp->flags |= TCB_ATTACHED|TCB_ATTACH_DONE;
 						}
 					}
 					if (interactive) {
 						sigprocmask(SIG_SETMASK, &empty_set, NULL);
 						if (interrupted)
-							return;
+							goto ret;
 						sigprocmask(SIG_BLOCK, &blocked_set, NULL);
 					}
 				}
@@ -508,7 +511,7 @@ startup_attach(void)
 				continue;
 			} /* if (opendir worked) */
 		} /* if (-f) */
-# endif
+# endif /* LINUX */
 		if (ptrace(PTRACE_ATTACH, tcp->pid, (char *) 1, 0) < 0) {
 			perror("attach: ptrace(PTRACE_ATTACH, ...)");
 			droptcb(tcp);
@@ -537,6 +540,15 @@ startup_attach(void)
 				tcp->pid);
 	} /* for each tcbtab[] */
 
+ ret:
+	if (TCB_ATTACH_DONE) {
+		for (tcbi = 0; tcbi < tcbtabsize; tcbi++) {
+			tcp = tcbtab[tcbi];
+			tcp->flags &= ~TCB_ATTACH_DONE;
+		}
+	}
+#undef TCB_ATTACH_DONE
+
 	if (interactive)
 		sigprocmask(SIG_SETMASK, &empty_set, NULL);
 }
@@ -1622,15 +1634,11 @@ detach(struct tcb *tcp, int sig)
 		/* Shouldn't happen. */
 		perror("detach: ptrace(PTRACE_DETACH, ...)");
 	}
-	else if (my_tgkill((tcp->flags & TCB_CLONE_THREAD ? tcp->parent->pid
-							  : tcp->pid),
-			   tcp->pid, 0) < 0) {
+	else if (my_tkill(tcp->pid, 0) < 0) {
 		if (errno != ESRCH)
 			perror("detach: checking sanity");
 	}
-	else if (!catch_sigstop && my_tgkill((tcp->flags & TCB_CLONE_THREAD
-					      ? tcp->parent->pid : tcp->pid),
-					     tcp->pid, SIGSTOP) < 0) {
+	else if (!catch_sigstop && my_tkill(tcp->pid, SIGSTOP) < 0) {
 		if (errno != ESRCH)
 			perror("detach: stopping child");
 	}
@@ -2433,16 +2441,13 @@ trace()
 				}
 			}
 #ifdef LINUX
-			/* If options were not set for this tracee yet */
-			if (tcp->parent == NULL) {
-				if (ptrace_setoptions) {
-					if (debug)
-						fprintf(stderr, "setting opts %x on pid %d\n", ptrace_setoptions, tcp->pid);
-					if (ptrace(PTRACE_SETOPTIONS, tcp->pid, NULL, ptrace_setoptions) < 0) {
-						if (errno != ESRCH) {
-							/* Should never happen, really */
-							perror_msg_and_die("PTRACE_SETOPTIONS");
-						}
+			if (ptrace_setoptions) {
+				if (debug)
+					fprintf(stderr, "setting opts %x on pid %d\n", ptrace_setoptions, tcp->pid);
+				if (ptrace(PTRACE_SETOPTIONS, tcp->pid, NULL, ptrace_setoptions) < 0) {
+					if (errno != ESRCH) {
+						/* Should never happen, really */
+						perror_msg_and_die("PTRACE_SETOPTIONS");
 					}
 				}
 			}