[PATCH 2009-02-11] further simplification of new tcb allocation

Denys Vlasenko dvlasenk at redhat.com
Thu Feb 12 12:42:27 UTC 2009


On Wed, 2009-02-11 at 20:14 +0100, Denys Vlasenko wrote:
> > then it needs review before we decide whether to let it lie as it is.
> 
> I think it makes sense if I will produce patches for changes
> I committed, and post them to strace-devel. You will comment on them,
> identifying which patches, or parts of them, you want reverted.
> 
> I will either revert them verbatim if it's easy
> (say, if the entire patch is to be reverted)
> or produce reversion patches and post them on strace-devel
> for your review.
> 
> Patches will be posted in reverse-chronological order.

2009-02-10  Denys Vlasenko  <dvlasenk at redhat.com>

	Cleanup after tcb table expansion simplification.
	There was code which was trying to continue tracing
	even if table expansion fails. Now we treat it as fatal
	failure, so this code is removed by this change.
	* defs.h: Delete TCB_FOLLOWFORK constant.
	* process.c: Delete fork_tcb() and all calls of it.
	* strace.c (startup_attach): Remove usage of TCB_FOLLOWFORK.
	* syscall.c: Indent preprocessor directives.

In hindsight, "so this code is removed by this change"
is a confusing phrase.

I meant to say "the code which is trying to detect the case
when TCB table expansion fails is removed by this patch (because
now table expansion never fails by returning NULL, it aborts)"

Roland, please ACK/NAK.
--
vda



diff -x CVS -urpN 2009-02-10/defs.h 2009-02-11/defs.h
--- 2009-02-10/defs.h	2009-02-09 19:55:59.000000000 +0100
+++ 2009-02-11/defs.h	2009-02-10 17:03:20.000000000 +0100
@@ -356,7 +356,6 @@ struct tcb {
 #define TCB_SUSPENDED	00040	/* Process can not be allowed to resume just now */
 #define TCB_BPTSET	00100	/* "Breakpoint" set after fork(2) */
 #define TCB_SIGTRAPPED	00200	/* Process wanted to block SIGTRAP */
-#define TCB_FOLLOWFORK	00400	/* Process should have forks followed */
 #define TCB_REPRINT	01000	/* We should reprint this syscall on exit */
 #ifdef LINUX
 /* TCB_WAITEXECVE bit means "ignore next SIGTRAP, it's execve exit stop".
diff -x CVS -urpN 2009-02-10/process.c 2009-02-11/process.c
--- 2009-02-10/process.c	2009-01-28 20:00:54.000000000 +0100
+++ 2009-02-11/process.c	2009-02-10 17:03:20.000000000 +0100
@@ -484,18 +484,6 @@ struct tcb *tcp;
 	return 0;
 }
 
-/* TCP is creating a child we want to follow.
-   If there will be space in tcbtab for it, set TCB_FOLLOWFORK and return 0.
-   If not, clear TCB_FOLLOWFORK, print an error, and return 1.  */
-static void
-fork_tcb(struct tcb *tcp)
-{
-	if (nprocs == tcbtabsize)
-		expand_tcbtab();
-
-	tcp->flags |= TCB_FOLLOWFORK;
-}
-
 #ifdef USE_PROCFS
 
 int
@@ -545,7 +533,6 @@ struct tcb *tcp;
 			return 0;
 		if (!followfork)
 			return 0;
-		fork_tcb(tcp);
 		if (syserror(tcp))
 			return 0;
 		tcpchild = alloctcb(tcp->u_rval);
@@ -882,18 +869,14 @@ internal_clone(struct tcb *tcp)
 	struct tcb *tcpchild;
 	int pid, bpt;
 
+	if (!followfork)
+		return 0;
 	if (entering(tcp)) {
-		if (!followfork)
-			return 0;
-		fork_tcb(tcp);
 		setbpt(tcp);
 		return 0;
 	} else {
 		bpt = tcp->flags & TCB_BPTSET;
 
-		if (!(tcp->flags & TCB_FOLLOWFORK))
-			return 0;
-
 		if (syserror(tcp)) {
 			if (bpt)
 				clearbpt(tcp);
@@ -901,7 +884,7 @@ internal_clone(struct tcb *tcp)
 		}
 
 		pid = tcp->u_rval;
-		/* Should not happen, but bugs often cause bogus value here */
+		/* Should not happen, but bugs often cause bogus value here. */
 		if (pid <= 1
 		 || (sizeof(pid) != sizeof(tcp->u_rval) && pid != tcp->u_rval)
 		) {
@@ -926,7 +909,6 @@ internal_clone(struct tcb *tcp)
 		else
 #endif
 		{
-			fork_tcb(tcp);
 			tcpchild = alloctcb(pid);
 		}
 
@@ -1028,27 +1010,25 @@ struct tcb *tcp;
 
 	struct tcb *tcpchild;
 	int pid;
-	int dont_follow = 0;
+	int follow = 1;
 
 #ifdef SYS_vfork
 	if (known_scno(tcp) == SYS_vfork) {
 		/* Attempt to make vfork into fork, which we can follow. */
 		if (change_syscall(tcp, SYS_fork) < 0)
-			dont_follow = 1;
+			follow = 0;
 	}
 #endif
+	if (!followfork || !follow)
+		return 0;
+
 	if (entering(tcp)) {
-		if (!followfork || dont_follow)
-			return 0;
-		fork_tcb(tcp);
 		if (setbpt(tcp) < 0)
 			return 0;
   	}
 	else {
 		int bpt = tcp->flags & TCB_BPTSET;
 
-		if (!(tcp->flags & TCB_FOLLOWFORK))
-			return 0;
 		if (bpt)
 			clearbpt(tcp);
 
@@ -1056,7 +1036,6 @@ struct tcb *tcp;
 			return 0;
 
 		pid = tcp->u_rval;
-		fork_tcb(tcp);
 		tcpchild = alloctcb(pid);
 #ifdef LINUX
 #ifdef HPPA
diff -x CVS -urpN 2009-02-10/strace.c 2009-02-11/strace.c
--- 2009-02-10/strace.c	2009-02-09 19:55:59.000000000 +0100
+++ 2009-02-11/strace.c	2009-02-10 17:03:20.000000000 +0100
@@ -440,7 +440,7 @@ startup_attach(void)
 						++nerr;
 					else if (tid != tcbtab[tcbi]->pid) {
 						tcp = alloctcb(tid);
-						tcp->flags |= TCB_ATTACHED|TCB_CLONE_THREAD|TCB_CLONE_DETACHED|TCB_FOLLOWFORK;
+						tcp->flags |= TCB_ATTACHED|TCB_CLONE_THREAD|TCB_CLONE_DETACHED;
 						tcbtab[tcbi]->nchildren++;
 						tcbtab[tcbi]->nclone_threads++;
 						tcbtab[tcbi]->nclone_detached++;
@@ -2392,12 +2392,12 @@ Process %d attached (waiting for parent)
 			}
 		}
 
-		if (cflag) {
 #ifdef LINUX
+		if (cflag) {
 			tv_sub(&tcp->dtime, &ru.ru_stime, &tcp->stime);
 			tcp->stime = ru.ru_stime;
-#endif
 		}
+#endif
 		if (tcp->flags & TCB_SUSPENDED) {
 			/*
 			 * Apparently, doing any ptrace() call on a stopped
@@ -2533,7 +2533,7 @@ handle_stopped_tcbs(struct tcb *tcp)
 		/*
 		 * Interestingly, the process may stop
 		 * with STOPSIG equal to some other signal
-		 * than SIGSTOP if we happend to attach
+		 * than SIGSTOP if we happen to attach
 		 * just before the process takes a signal.
 		 */
 		if ((tcp->flags & TCB_STARTUP) && WSTOPSIG(status) == SIGSTOP) {
diff -x CVS -urpN 2009-02-10/syscall.c 2009-02-11/syscall.c
--- 2009-02-10/syscall.c	2009-01-23 17:30:26.000000000 +0100
+++ 2009-02-11/syscall.c	2009-02-10 17:03:20.000000000 +0100
@@ -38,38 +38,39 @@
 #include <signal.h>
 #include <time.h>
 #include <errno.h>
+#include <sched.h>
 #include <sys/user.h>
 #include <sys/syscall.h>
 #include <sys/param.h>
 
 #if HAVE_ASM_REG_H
-#if defined (SPARC) || defined (SPARC64)
+# if defined (SPARC) || defined (SPARC64)
 #  define fpq kernel_fpq
 #  define fq kernel_fq
 #  define fpu kernel_fpu
-#endif
-#include <asm/reg.h>
-#if defined (SPARC) || defined (SPARC64)
+# endif
+# include <asm/reg.h>
+# if defined (SPARC) || defined (SPARC64)
 #  undef fpq
 #  undef fq
 #  undef fpu
-#endif
+# endif
 #endif
 
 #ifdef HAVE_SYS_REG_H
-#include <sys/reg.h>
-#ifndef PTRACE_PEEKUSR
-# define PTRACE_PEEKUSR PTRACE_PEEKUSER
-#endif
+# include <sys/reg.h>
+# ifndef PTRACE_PEEKUSR
+#  define PTRACE_PEEKUSR PTRACE_PEEKUSER
+# endif
 #elif defined(HAVE_LINUX_PTRACE_H)
-#undef PTRACE_SYSCALL
+# undef PTRACE_SYSCALL
 # ifdef HAVE_STRUCT_IA64_FPREG
 #  define ia64_fpreg XXX_ia64_fpreg
 # endif
 # ifdef HAVE_STRUCT_PT_ALL_USER_REGS
 #  define pt_all_user_regs XXX_pt_all_user_regs
 # endif
-#include <linux/ptrace.h>
+# include <linux/ptrace.h>
 # undef ia64_fpreg
 # undef pt_all_user_regs
 #endif






More information about the Strace-devel mailing list