[PATCH] remove dead "ifndef CLONE_PTRACE" branch, make setbpt easier to read

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

Hi,

process.c #defines CLONE_PTRACE for Linux, so it can't be undefined.
Therefore #ifndef CLONE_PTRACE code is dead (since at least 2004).
This patch removes it.
While at it, it adds a comment about setbpt on Linux
and untangles a particularly badly obfuscated bit of code
in setbpt:

-				 (tcp->u_arg[arg0_index] | CLONE_PTRACE)
-				 & ~(tcp->u_arg[arg0_index] & CLONE_VFORK
-				     ? CLONE_VFORK | CLONE_VM : 0)

becomes:

+		long new_arg0 = (tcp->u_arg[arg0_index] | CLONE_PTRACE);
+		if (new_arg0 & CLONE_VFORK)
+			new_arg0 &= ~(unsigned long)(CLONE_VFORK | CLONE_VM);

Please review.
-- 
vda


diff -d -urpN strace.6/defs.h strace.7/defs.h
--- strace.6/defs.h	2011-06-22 13:31:45.323884488 +0200
+++ strace.7/defs.h	2011-06-22 00:38:57.646897421 +0200
@@ -566,6 +566,12 @@ extern void print_sock_optmgmt(struct tc
 extern void printrusage(struct tcb *, long);
 extern void printuid(const char *, unsigned long);
 extern int clearbpt(struct tcb *);
+/*
+ * On Linux, "setbpt" is a misnomer: we don't set a breakpoint
+ * (IOW: no poking in user's text segment),
+ * instead we change fork/vfork/clone into clone(CLONE_PTRACE).
+ * On newer kernels, we use PTRACE_O_TRACECLONE/TRACE[V]FORK instead.
+ */
 extern int setbpt(struct tcb *);
 extern int sigishandled(struct tcb *, int);
 extern void printcall(struct tcb *);
diff -d -urpN strace.6/process.c strace.7/process.c
--- strace.6/process.c	2011-06-22 13:37:06.210864935 +0200
+++ strace.7/process.c	2011-06-22 13:36:57.602929960 +0200
@@ -780,7 +780,6 @@ handle_new_child(struct tcb *tcp, int pi
 {
 	struct tcb *tcpchild;
 
-#ifdef CLONE_PTRACE		/* See new setbpt code.  */
 	tcpchild = pid2tcb(pid);
 	if (tcpchild != NULL) {
 		/* The child already reported its startup trap
@@ -792,30 +791,15 @@ handle_new_child(struct tcb *tcp, int pi
 [preattached child %d of %d in weird state!]\n",
 				pid, tcp->pid);
 	}
-	else
-#endif /* CLONE_PTRACE */
-	{
+	else {
 		tcpchild = alloctcb(pid);
 	}
 
-#ifndef CLONE_PTRACE
-	/* Attach to the new child */
-	if (ptrace(PTRACE_ATTACH, pid, (char *) 1, 0) < 0) {
-		if (bpt)
-			clearbpt(tcp);
-		perror("PTRACE_ATTACH");
-		fprintf(stderr, "Too late?\n");
-		droptcb(tcpchild);
-		return 0;
-	}
-#endif /* !CLONE_PTRACE */
-
-	if (bpt)
-		clearbpt(tcp);
-
 	tcpchild->flags |= TCB_ATTACHED;
-	/* Child has BPT too, must be removed on first occasion.  */
+
 	if (bpt) {
+		clearbpt(tcp);
+		/* Child has BPT too, must be removed on first occasion.  */
 		tcpchild->flags |= TCB_BPTSET;
 		tcpchild->baddr = tcp->baddr;
 		memcpy(tcpchild->inst, tcp->inst,
diff -d -urpN strace.6/util.c strace.7/util.c
--- strace.6/util.c	2011-06-22 00:19:30.268714125 +0200
+++ strace.7/util.c	2011-06-22 13:32:30.653923439 +0200
@@ -1494,9 +1494,9 @@ setbpt(struct tcb *tcp)
 		return 0;
 #  endif
 
-	case SYS_clone:
+	case SYS_clone: ;
 #  ifdef SYS_clone2
-	case SYS_clone2:
+	case SYS_clone2: ;
 #  endif
 		/* ia64 calls directly `clone (CLONE_VFORK | CLONE_VM)'
 		   contrary to x86 SYS_vfork above.  Even on x86 we turn the
@@ -1506,12 +1506,12 @@ setbpt(struct tcb *tcp)
 		   clear also CLONE_VM but only in the CLONE_VFORK case as
 		   otherwise we would break pthread_create.  */
 
-		if ((arg_setup(tcp, &state) < 0
-		    || set_arg0(tcp, &state,
-				 (tcp->u_arg[arg0_index] | CLONE_PTRACE)
-				 & ~(tcp->u_arg[arg0_index] & CLONE_VFORK
-				     ? CLONE_VFORK | CLONE_VM : 0)) < 0
-		    || arg_finish_change(tcp, &state) < 0))
+		long new_arg0 = (tcp->u_arg[arg0_index] | CLONE_PTRACE);
+		if (new_arg0 & CLONE_VFORK)
+			new_arg0 &= ~(unsigned long)(CLONE_VFORK | CLONE_VM);
+		if (arg_setup(tcp, &state) < 0
+		 || set_arg0(tcp, &state, new_arg0) < 0
+		 || arg_finish_change(tcp, &state) < 0)
 			return -1;
 		tcp->flags |= TCB_BPTSET;
 		tcp->inst[0] = tcp->u_arg[arg0_index];
@@ -1535,7 +1535,8 @@ clearbpt(struct tcb *tcp)
 	    || restore_arg0(tcp, &state, tcp->inst[0]) < 0
 	    || restore_arg1(tcp, &state, tcp->inst[1]) < 0
 	    || arg_finish_change(tcp, &state))
-		if (errno != ESRCH) return -1;
+		if (errno != ESRCH)
+			return -1;
 	tcp->flags &= ~TCB_BPTSET;
 	return 0;
 }