[PATCH] stop using -1 in syscallent tables

Dmitry V. Levin ldv at altlinux.org
Tue Aug 23 11:00:32 UTC 2011


On Tue, Aug 23, 2011 at 12:34:14PM +0200, Denys Vlasenko wrote:
> Usage -1 as argument count in syscallent tables
> necessitates the check for it, a-la:
> 
>         if (tcp->scno >= 0 && tcp->scno < nsyscalls && sysent[tcp->scno].nargs != -1)
>                 tcp->u_nargs = sysent[tcp->scno].nargs;
>         else
>                 tcp->u_nargs = MAX_ARGS;
> 
> which is stupid: we waste cycles checking something which
> is constant and known at compile time.
> 
> I propose to fix it with these changes:
> 
> * Replace all -1 by MA in all tables.
> * Redefine struct sysent::nargs as unsigned
> (sadly, this is mostly cosmetic, as gcc won't warn
> about -1 static initializer for unsigned field, but still...)
> * Add #define MA MAX_ARGS / #undef MA around #include "syscallent[N].h"
> * Remove sysent[tcp->scno].nargs != -1 checks.
> 
> This seems a good plan to me, so I'm going to commit these changes,

I agree, but first I'd like to reduce redundancy in syscall_enter().
I have a not quite tested patch, please have a look:

diff --git a/syscall.c b/syscall.c
index 2fd5bb4..7beb0cb 100644
--- a/syscall.c
+++ b/syscall.c
@@ -1949,22 +1949,20 @@ static int
 syscall_enter(struct tcb *tcp)
 {
 #ifdef LINUX
-# if defined(S390) || defined(S390X)
 	int i;
-	if (tcp->scno >= 0 && tcp->scno < nsyscalls && sysent[tcp->scno].nargs != -1)
+
+	if (tcp->scno >= 0 && tcp->scno < nsyscalls &&
+	    sysent[tcp->scno].nargs >= 0 && sysent[tcp->scno].nargs <= MAX_ARGS)
 		tcp->u_nargs = sysent[tcp->scno].nargs;
 	else
 		tcp->u_nargs = MAX_ARGS;
+
+# if defined(S390) || defined(S390X)
 	for (i = 0; i < tcp->u_nargs; i++) {
 		if (upeek(tcp, i==0 ? PT_ORIGGPR2 : PT_GPR2 + i*sizeof(long), &tcp->u_arg[i]) < 0)
 			return -1;
 	}
 # elif defined(ALPHA)
-	int i;
-	if (tcp->scno >= 0 && tcp->scno < nsyscalls && sysent[tcp->scno].nargs != -1)
-		tcp->u_nargs = sysent[tcp->scno].nargs;
-	else
-		tcp->u_nargs = MAX_ARGS;
 	for (i = 0; i < tcp->u_nargs; i++) {
 		/* WTA: if scno is out-of-bounds this will bomb. Add range-check
 		 * for scno somewhere above here!
@@ -1974,7 +1972,7 @@ syscall_enter(struct tcb *tcp)
 	}
 # elif defined(IA64)
 	if (!ia32) {
-		unsigned long *out0, cfm, sof, sol, i;
+		unsigned long *out0, cfm, sof, sol;
 		long rbs_end;
 		/* be backwards compatible with kernel < 2.4.4... */
 #		ifndef PT_RBS_END
@@ -1990,80 +1988,53 @@ syscall_enter(struct tcb *tcp)
 		sol = (cfm >> 7) & 0x7f;
 		out0 = ia64_rse_skip_regs((unsigned long *) rbs_end, -sof + sol);
 
-		if (tcp->scno >= 0 && tcp->scno < nsyscalls
-		    && sysent[tcp->scno].nargs != -1)
-			tcp->u_nargs = sysent[tcp->scno].nargs;
-		else
-			tcp->u_nargs = MAX_ARGS;
 		for (i = 0; i < tcp->u_nargs; ++i) {
 			if (umoven(tcp, (unsigned long) ia64_rse_skip_regs(out0, i),
 				   sizeof(long), (char *) &tcp->u_arg[i]) < 0)
 				return -1;
 		}
 	} else {
-		int i;
+		static const int argreg[MAX_ARGS] = { PT_R11 /* EBX = out0 */,
+						      PT_R9  /* ECX = out1 */,
+						      PT_R10 /* EDX = out2 */,
+						      PT_R14 /* ESI = out3 */,
+						      PT_R15 /* EDI = out4 */,
+						      PT_R13 /* EBP = out5 */ };
 
-		if (/* EBX = out0 */
-		    upeek(tcp, PT_R11, (long *) &tcp->u_arg[0]) < 0
-		    /* ECX = out1 */
-		    || upeek(tcp, PT_R9,  (long *) &tcp->u_arg[1]) < 0
-		    /* EDX = out2 */
-		    || upeek(tcp, PT_R10, (long *) &tcp->u_arg[2]) < 0
-		    /* ESI = out3 */
-		    || upeek(tcp, PT_R14, (long *) &tcp->u_arg[3]) < 0
-		    /* EDI = out4 */
-		    || upeek(tcp, PT_R15, (long *) &tcp->u_arg[4]) < 0
-		    /* EBP = out5 */
-		    || upeek(tcp, PT_R13, (long *) &tcp->u_arg[5]) < 0)
-			return -1;
-
-		for (i = 0; i < 6; ++i)
+		for (i = 0; i < tcp->u_nargs; ++i) {
+			if (upeek(tcp, argreg[i], &tcp->u_arg[i]) < 0)
+				return -1;
 			/* truncate away IVE sign-extension */
 			tcp->u_arg[i] &= 0xffffffff;
-
-		if (tcp->scno >= 0 && tcp->scno < nsyscalls
-		    && sysent[tcp->scno].nargs != -1)
-			tcp->u_nargs = sysent[tcp->scno].nargs;
-		else
-			tcp->u_nargs = 5;
+		}
 	}
 # elif defined(LINUX_MIPSN32) || defined(LINUX_MIPSN64)
 	/* N32 and N64 both use up to six registers.  */
 	unsigned long long regs[38];
-	int i, nargs;
-	if (tcp->scno >= 0 && tcp->scno < nsyscalls && sysent[tcp->scno].nargs != -1)
-		nargs = tcp->u_nargs = sysent[tcp->scno].nargs;
-	else
-		nargs = tcp->u_nargs = MAX_ARGS;
 
 	if (ptrace(PTRACE_GETREGS, tcp->pid, NULL, (long) &regs) < 0)
 		return -1;
 
-	for (i = 0; i < nargs; i++) {
+	for (i = 0; i < tcp->u_nargs; i++) {
 		tcp->u_arg[i] = regs[REG_A0 + i];
 #  if defined(LINUX_MIPSN32)
 		tcp->ext_arg[i] = regs[REG_A0 + i];
 #  endif
 	}
 # elif defined(MIPS)
-	long sp;
-	int i, nargs;
+	if (tcp->u_nargs > 4) {
+		long sp;
 
-	if (tcp->scno >= 0 && tcp->scno < nsyscalls && sysent[tcp->scno].nargs != -1)
-		nargs = tcp->u_nargs = sysent[tcp->scno].nargs;
-	else
-		nargs = tcp->u_nargs = MAX_ARGS;
-	if (nargs > 4) {
 		if (upeek(tcp, REG_SP, &sp) < 0)
 			return -1;
 		for (i = 0; i < 4; i++) {
 			if (upeek(tcp, REG_A0 + i, &tcp->u_arg[i]) < 0)
 				return -1;
 		}
-		umoven(tcp, sp+16, (nargs-4) * sizeof(tcp->u_arg[0]),
+		umoven(tcp, sp+16, (tcp->u_nargs - 4) * sizeof(tcp->u_arg[0]),
 		       (char *)(tcp->u_arg + 4));
 	} else {
-		for (i = 0; i < nargs; i++) {
+		for (i = 0; i < tcp->u_nargs; i++) {
 			if (upeek(tcp, REG_A0 + i, &tcp->u_arg[i]) < 0)
 				return -1;
 		}
@@ -2072,11 +2043,6 @@ syscall_enter(struct tcb *tcp)
 #  ifndef PT_ORIG_R3
 #   define PT_ORIG_R3 34
 #  endif
-	int i;
-	if (tcp->scno >= 0 && tcp->scno < nsyscalls && sysent[tcp->scno].nargs != -1)
-		tcp->u_nargs = sysent[tcp->scno].nargs;
-	else
-		tcp->u_nargs = MAX_ARGS;
 	for (i = 0; i < tcp->u_nargs; i++) {
 		if (upeek(tcp, (i==0) ?
 			(sizeof(unsigned long) * PT_ORIG_R3) :
@@ -2085,33 +2051,17 @@ syscall_enter(struct tcb *tcp)
 			return -1;
 	}
 # elif defined(SPARC) || defined(SPARC64)
-	int i;
-	if (tcp->scno >= 0 && tcp->scno < nsyscalls && sysent[tcp->scno].nargs != -1)
-		tcp->u_nargs = sysent[tcp->scno].nargs;
-	else
-		tcp->u_nargs = MAX_ARGS;
 	for (i = 0; i < tcp->u_nargs; i++)
 		tcp->u_arg[i] = regs.u_regs[U_REG_O0 + i];
 # elif defined(HPPA)
-	int i;
-	if (tcp->scno >= 0 && tcp->scno < nsyscalls && sysent[tcp->scno].nargs != -1)
-		tcp->u_nargs = sysent[tcp->scno].nargs;
-	else
-		tcp->u_nargs = MAX_ARGS;
 	for (i = 0; i < tcp->u_nargs; i++) {
 		if (upeek(tcp, PT_GR26-4*i, &tcp->u_arg[i]) < 0)
 			return -1;
 	}
 # elif defined(ARM)
-	int i;
-	if (tcp->scno >= 0 && tcp->scno < nsyscalls && sysent[tcp->scno].nargs != -1)
-		tcp->u_nargs = sysent[tcp->scno].nargs;
-	else
-		tcp->u_nargs = MAX_ARGS;
 	for (i = 0; i < tcp->u_nargs; i++)
 		tcp->u_arg[i] = regs.uregs[i];
 # elif defined(AVR32)
-	tcp->u_nargs = sysent[tcp->scno].nargs;
 	tcp->u_arg[0] = regs.r12;
 	tcp->u_arg[1] = regs.r11;
 	tcp->u_arg[2] = regs.r10;
@@ -2119,25 +2069,17 @@ syscall_enter(struct tcb *tcp)
 	tcp->u_arg[4] = regs.r5;
 	tcp->u_arg[5] = regs.r3;
 # elif defined(BFIN)
-	int i;
-	static const int argreg[] = { PT_R0, PT_R1, PT_R2, PT_R3, PT_R4, PT_R5 };
-
-	if (tcp->scno >= 0 && tcp->scno < nsyscalls && sysent[tcp->scno].nargs != -1)
-		tcp->u_nargs = sysent[tcp->scno].nargs;
-	else
-		tcp->u_nargs = ARRAY_SIZE(argreg);
+	static const int argreg[MAX_ARGS] = { PT_R0, PT_R1, PT_R2, PT_R3, PT_R4, PT_R5 };
 
 	for (i = 0; i < tcp->u_nargs; ++i)
 		if (upeek(tcp, argreg[i], &tcp->u_arg[i]) < 0)
 			return -1;
 # elif defined(SH)
-	int i;
-	static const int syscall_regs[] = {
-		4 * (REG_REG0+4), 4 * (REG_REG0+5), 4 * (REG_REG0+6), 4 * (REG_REG0+7),
-		4 * (REG_REG0  ), 4 * (REG_REG0+1), 4 * (REG_REG0+2)
+	static const int syscall_regs[MAX_ARGS] = {
+		4 * (REG_REG0+4), 4 * (REG_REG0+5), 4 * (REG_REG0+6),
+		4 * (REG_REG0+7), 4 * (REG_REG0+0), 4 * (REG_REG0+1)
 	};
 
-	tcp->u_nargs = sysent[tcp->scno].nargs;
 	for (i = 0; i < tcp->u_nargs; i++) {
 		if (upeek(tcp, syscall_regs[i], &tcp->u_arg[i]) < 0)
 			return -1;
@@ -2145,87 +2087,48 @@ syscall_enter(struct tcb *tcp)
 # elif defined(SH64)
 	int i;
 	/* Registers used by SH5 Linux system calls for parameters */
-	static const int syscall_regs[] = { 2, 3, 4, 5, 6, 7 };
-
-	/*
-	 * TODO: should also check that the number of arguments encoded
-	 *       in the trap number matches the number strace expects.
-	 */
-	/*
-	assert(sysent[tcp->scno].nargs < ARRAY_SIZE(syscall_regs));
-	 */
+	static const int syscall_regs[MAX_ARGS] = { 2, 3, 4, 5, 6, 7 };
 
-	tcp->u_nargs = sysent[tcp->scno].nargs;
 	for (i = 0; i < tcp->u_nargs; i++) {
 		if (upeek(tcp, REG_GENERAL(syscall_regs[i]), &tcp->u_arg[i]) < 0)
 			return -1;
 	}
 # elif defined(X86_64)
-	int i;
 	static const int argreg[SUPPORTED_PERSONALITIES][MAX_ARGS] = {
 		{ 8 * RDI, 8 * RSI, 8 * RDX, 8 * R10, 8 * R8 , 8 * R9  }, /* x86-64 ABI */
 		{ 8 * RBX, 8 * RCX, 8 * RDX, 8 * RSI, 8 * RDI, 8 * RBP }  /* i386 ABI */
 	};
 
-	if (tcp->scno >= 0 && tcp->scno < nsyscalls && sysent[tcp->scno].nargs != -1)
-		tcp->u_nargs = sysent[tcp->scno].nargs;
-	else
-		tcp->u_nargs = MAX_ARGS;
 	for (i = 0; i < tcp->u_nargs; i++) {
 		if (upeek(tcp, argreg[current_personality][i], &tcp->u_arg[i]) < 0)
 			return -1;
 	}
 # elif defined(MICROBLAZE)
-	int i;
-	if (tcp->scno >= 0 && tcp->scno < nsyscalls)
-		tcp->u_nargs = sysent[tcp->scno].nargs;
-	else
-		tcp->u_nargs = 0;
 	for (i = 0; i < tcp->u_nargs; i++) {
 		if (upeek(tcp, (5 + i) * 4, &tcp->u_arg[i]) < 0)
 			return -1;
 	}
 # elif defined(CRISV10) || defined(CRISV32)
-	int i;
-	static const int crisregs[] = {
+	static const int crisregs[MAX_ARGS] = {
 		4*PT_ORIG_R10, 4*PT_R11, 4*PT_R12,
 		4*PT_R13     , 4*PT_MOF, 4*PT_SRP
 	};
 
-	if (tcp->scno >= 0 && tcp->scno < nsyscalls)
-		tcp->u_nargs = sysent[tcp->scno].nargs;
-	else
-		tcp->u_nargs = 0;
 	for (i = 0; i < tcp->u_nargs; i++) {
 		if (upeek(tcp, crisregs[i], &tcp->u_arg[i]) < 0)
 			return -1;
 	}
 # elif defined(TILE)
-	int i;
-	if (tcp->scno >= 0 && tcp->scno < nsyscalls && sysent[tcp->scno].nargs != -1)
-		tcp->u_nargs = sysent[tcp->scno].nargs;
-	else
-		tcp->u_nargs = MAX_ARGS;
 	for (i = 0; i < tcp->u_nargs; ++i) {
 		if (upeek(tcp, PTREGS_OFFSET_REG(i), &tcp->u_arg[i]) < 0)
 			return -1;
 	}
 # elif defined(M68K)
-	int i;
-	if (tcp->scno >= 0 && tcp->scno < nsyscalls && sysent[tcp->scno].nargs != -1)
-		tcp->u_nargs = sysent[tcp->scno].nargs;
-	else
-		tcp->u_nargs = MAX_ARGS;
 	for (i = 0; i < tcp->u_nargs; i++) {
 		if (upeek(tcp, (i < 5 ? i : i + 2)*4, &tcp->u_arg[i]) < 0)
 			return -1;
 	}
 # else /* Other architecture (like i386) (32bits specific) */
-	int i;
-	if (tcp->scno >= 0 && tcp->scno < nsyscalls && sysent[tcp->scno].nargs != -1)
-		tcp->u_nargs = sysent[tcp->scno].nargs;
-	else
-		tcp->u_nargs = MAX_ARGS;
 	for (i = 0; i < tcp->u_nargs; i++) {
 		if (upeek(tcp, i*4, &tcp->u_arg[i]) < 0)
 			return -1;

-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20110823/f68def57/attachment.bin>


More information about the Strace-devel mailing list