[PATCH] Stop mprotect() changing MAP_SHARED and other cleanup

[PATCH] Stop mprotect() changing MAP_SHARED and other cleanup

[Jamie Lokier]

Linus Torvalds wrote:
> How about something like this that at least gets it closer?

Here's my offering, which is more like your earlier request.
With luck it'll optimise to the same thing :)

Patch: mprotect-2.6.0-test4-1

This patch:

  1. Removes the logic bug in mprotect() which allows it to change
     MAP_SHARED if PROT_SEM is passed.

  2. Moves the mapping of PROT_* bits to VM_* bits from mmap.c to
     the common header file <linux/mman.h>.

  3. Uses that function in mprotect() the same as mmap().  Previously
     mprotect() assumed the PROT_* and VM_* bits were numerically
     the same, which is what caused the PROT_SEM bug.

  4. Fixes an unlikely overflow in vm_locked accounting.

Enjoy,
-- Jamie


diff -urN --exclude-from=dontdiff orig-2.6.0-test4/include/linux/mman.h mprotect-2.6.0-test4/include/linux/mman.h
--- orig-2.6.0-test4/include/linux/mman.h	2003-07-12 17:57:37.000000000 +0100
+++ mprotect-2.6.0-test4/include/linux/mman.h	2003-09-04 20:24:35.000000000 +0100
@@ -2,6 +2,7 @@
 #define _LINUX_MMAN_H
 
 #include <linux/config.h>
+#include <linux/mm.h>
 
 #include <asm/atomic.h>
 #include <asm/mman.h>
@@ -27,4 +28,32 @@
 	vm_acct_memory(-pages);
 }
 
+/* Optimisation macro. */
+#define _calc_vm_trans(x,bit1,bit2) \
+  ((bit1) <= (bit2) ? ((x) & (bit1)) * ((bit2) / (bit1)) \
+   : ((x) & (bit1)) / ((bit1) / (bit2)))
+
+/*
+ * Combine the mmap "prot" argument into "vm_flags" used internally.
+ */
+static inline unsigned long
+calc_vm_prot_bits(unsigned long prot)
+{
+	return _calc_vm_trans(prot, PROT_READ,  VM_READ ) |
+	       _calc_vm_trans(prot, PROT_WRITE, VM_WRITE) |
+	       _calc_vm_trans(prot, PROT_EXEC,  VM_EXEC );
+}
+
+/*
+ * Combine the mmap "flags" argument into "vm_flags" used internally.
+ */
+static inline unsigned long
+calc_vm_flag_bits(unsigned long flags)
+{
+	return _calc_vm_trans(flags, MAP_GROWSDOWN,  VM_GROWSDOWN ) |
+	       _calc_vm_trans(flags, MAP_DENYWRITE,  VM_DENYWRITE ) |
+	       _calc_vm_trans(flags, MAP_EXECUTABLE, VM_EXECUTABLE) |
+	       _calc_vm_trans(flags, MAP_LOCKED,     VM_LOCKED    );
+}
+
 #endif /* _LINUX_MMAN_H */
diff -urN --exclude-from=dontdiff orig-2.6.0-test4/include/linux/mm.h mprotect-2.6.0-test4/include/linux/mm.h
--- orig-2.6.0-test4/include/linux/mm.h	2003-09-02 23:06:10.000000000 +0100
+++ mprotect-2.6.0-test4/include/linux/mm.h	2003-09-04 20:18:21.000000000 +0100
@@ -25,6 +25,7 @@
 #include <asm/pgtable.h>
 #include <asm/processor.h>
 #include <asm/atomic.h>
+#include <asm/mman.h>
 
 #ifndef MM_VM_SIZE
 #define MM_VM_SIZE(mm)	TASK_SIZE
@@ -85,12 +86,13 @@
 #define VM_READ		0x00000001	/* currently active flags */
 #define VM_WRITE	0x00000002
 #define VM_EXEC		0x00000004
-#define VM_SHARED	0x00000008
 
 #define VM_MAYREAD	0x00000010	/* limits for mprotect() etc */
 #define VM_MAYWRITE	0x00000020
 #define VM_MAYEXEC	0x00000040
-#define VM_MAYSHARE	0x00000080
+
+#define VM_SHARED	0x00000008	/* shared AND may write to pages */
+#define VM_MAYSHARE	0x00000080	/* set iff MAP_SHARED was set */
 
 #define VM_GROWSDOWN	0x00000100	/* general info on the segment */
 #define VM_GROWSUP	0x00000200
diff -urN --exclude-from=dontdiff orig-2.6.0-test4/mm/mmap.c mprotect-2.6.0-test4/mm/mmap.c
--- orig-2.6.0-test4/mm/mmap.c	2003-08-27 22:56:05.000000000 +0100
+++ mprotect-2.6.0-test4/mm/mmap.c	2003-09-04 20:10:02.000000000 +0100
@@ -136,29 +136,6 @@
 	return retval;
 }
 
-/* Combine the mmap "prot" and "flags" argument into one "vm_flags" used
- * internally. Essentially, translate the "PROT_xxx" and "MAP_xxx" bits
- * into "VM_xxx".
- */
-static inline unsigned long
-calc_vm_flags(unsigned long prot, unsigned long flags)
-{
-#define _trans(x,bit1,bit2) \
-((bit1==bit2)?(x&bit1):(x&bit1)?bit2:0)
-
-	unsigned long prot_bits, flag_bits;
-	prot_bits =
-		_trans(prot, PROT_READ, VM_READ) |
-		_trans(prot, PROT_WRITE, VM_WRITE) |
-		_trans(prot, PROT_EXEC, VM_EXEC);
-	flag_bits =
-		_trans(flags, MAP_GROWSDOWN, VM_GROWSDOWN) |
-		_trans(flags, MAP_DENYWRITE, VM_DENYWRITE) |
-		_trans(flags, MAP_EXECUTABLE, VM_EXECUTABLE);
-	return prot_bits | flag_bits;
-#undef _trans
-}
-
 #ifdef DEBUG_MM_RB
 static int browse_rb(struct rb_node * rb_node) {
 	int i = 0;
@@ -500,19 +477,17 @@
 	 * to. we assume access permissions have been handled by the open
 	 * of the memory object, so we don't do any here.
 	 */
-	vm_flags = calc_vm_flags(prot,flags) | mm->def_flags |
-			VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;
+	vm_flags = calc_vm_prot_bits(prot) | calc_vm_flag_bits(flags) |
+			mm->def_flags | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;
 
-	if (flags & MAP_LOCKED) {
+	if (vm_flags & VM_LOCKED) {
+		unsigned long locked;
 		if (!capable(CAP_IPC_LOCK))
 			return -EPERM;
-		vm_flags |= VM_LOCKED;
-	}
-	/* mlock MCL_FUTURE? */
-	if (vm_flags & VM_LOCKED) {
-		unsigned long locked = mm->locked_vm << PAGE_SHIFT;
+		locked = (mm->locked_vm << PAGE_SHIFT);
 		locked += len;
-		if (locked > current->rlim[RLIMIT_MEMLOCK].rlim_cur)
+		if (locked < len || /* Overflow check. */
+		    locked > current->rlim[RLIMIT_MEMLOCK].rlim_cur)
 			return -EAGAIN;
 	}
 
diff -urN --exclude-from=dontdiff orig-2.6.0-test4/mm/mprotect.c mprotect-2.6.0-test4/mm/mprotect.c
--- orig-2.6.0-test4/mm/mprotect.c	2003-07-12 17:57:37.000000000 +0100
+++ mprotect-2.6.0-test4/mm/mprotect.c	2003-09-04 20:12:34.000000000 +0100
@@ -257,8 +257,11 @@
 			goto out;
 		}
 
-		newflags = prot | (vma->vm_flags & ~(PROT_READ | PROT_WRITE | PROT_EXEC));
-		if ((newflags & ~(newflags >> 4)) & 0xf) {
+		newflags = calc_vm_prot_bits(prot) |
+			(vma->vm_flags & ~(VM_READ | VM_WRITE | VM_EXEC));
+
+		/* Check against VM_MAYREAD, VM_MAYWRITE and VM_MAYEXEC. */
+		if ((newflags & ~(newflags >> 4)) & (VM_READ | VM_WRITE | VM_EXEC)) {
 			error = -EACCES;
 			goto out;
 		}

Re: [PATCH] Stop mprotect() changing MAP_SHARED and other cleanup

[Muli Ben-Yehuda]

[-- Attachment #1: Type: text/plain, Size: 5673 bytes --]

On Thu, Sep 04, 2003 at 08:34:54PM +0100, Jamie Lokier wrote:
> Linus Torvalds wrote:
> > How about something like this that at least gets it closer?
> 
> Here's my offering, which is more like your earlier request.
> With luck it'll optimise to the same thing :)

Hi Jamie, 

> +/* Optimisation macro. */
> +#define _calc_vm_trans(x,bit1,bit2) \
> +  ((bit1) <= (bit2) ? ((x) & (bit1)) * ((bit2) / (bit1)) \
> +   : ((x) & (bit1)) / ((bit1) / (bit2))

Why is this necessary? the original version of the macro was much
simpler. If this isn't just for shaving a couple of optimization,
please document it. If it is, I urge you to reconsider ;-) 

Here's my version of the same macro, which is a 1-to-1 copy of what
the _trans() macro did: 

+/* check if bit1 is on in 'in'. If it is, return bit2
+ * this is used for transltating from one bit field domain to another,
+ * e.g. PROT_XXX to VM_XXX 
+ */ 
+static unsigned long trans_bit(unsigned long in, unsigned long bit1, 
+		             unsigned long bit2)
+{
+        if (bit1 == bit2)
+                return (in & bit1); 
+
+        return  (in & bit1) ? bit2 : 0;
+}

here's my version of the patch, which only touches the calc_xxx
bits. Compiles and boots. 

Index: include/linux/mman.h
===================================================================
RCS file: /home/cvs/linux-2.5/include/linux/mman.h,v
retrieving revision 1.5
diff -u -u -r1.5 mman.h
--- include/linux/mman.h	3 Jul 2003 05:49:35 -0000	1.5
+++ include/linux/mman.h	4 Sep 2003 18:23:19 -0000
@@ -2,6 +2,7 @@
 #define _LINUX_MMAN_H
 
 #include <linux/config.h>
+#include <linux/mm.h> 
 
 #include <asm/atomic.h>
 #include <asm/mman.h>
@@ -25,6 +26,33 @@
 static inline void vm_unacct_memory(long pages)
 {
 	vm_acct_memory(-pages);
+}
+
+/* check if bit1 is on in 'in'. If it is, return bit2
+ * this is used for transltating from one bit field domain to another,
+ * e.g. PROT_XXX to VM_XXX 
+ */ 
+static unsigned long trans_bit(unsigned long in, unsigned long bit1, 
+			       unsigned long bit2)
+{
+	if (bit1 == bit2)
+		return (in & bit1); 
+
+	return  (in & bit1) ? bit2 : 0;
+}
+
+/* Combine the mmap "prot" argument into a bit representation suitable for
+ * "vm_flags".  Essentially, translate the "PROT_xxx" bits into "VM_xxx".
+ */ 
+static inline unsigned long calc_vm_prot(unsigned long prot)
+{
+	unsigned long prot_bits; 
+
+	prot_bits = trans_bit(prot, PROT_READ, VM_READ) |
+		trans_bit(prot, PROT_WRITE, VM_WRITE) |
+		trans_bit(prot, PROT_EXEC, VM_EXEC); 
+
+	return prot_bits; 
 }
 
 #endif /* _LINUX_MMAN_H */
Index: mm/mmap.c
===================================================================
RCS file: /home/cvs/linux-2.5/mm/mmap.c,v
retrieving revision 1.90
diff -u -u -r1.90 mmap.c
--- mm/mmap.c	11 Jul 2003 02:46:56 -0000	1.90
+++ mm/mmap.c	4 Sep 2003 18:23:22 -0000
@@ -136,27 +136,18 @@
 	return retval;
 }
 
-/* Combine the mmap "prot" and "flags" argument into one "vm_flags" used
- * internally. Essentially, translate the "PROT_xxx" and "MAP_xxx" bits
- * into "VM_xxx".
- */
-static inline unsigned long
-calc_vm_flags(unsigned long prot, unsigned long flags)
-{
-#define _trans(x,bit1,bit2) \
-((bit1==bit2)?(x&bit1):(x&bit1)?bit2:0)
+/* Combine the mmap "flags" argument into a bit representation suitable for
+ * "vm_flags".  Essentially, translate the "MAP_xxx" bits into "VM_xxx".
+ */ 
+static inline unsigned long calc_vm_flags(unsigned long flags)
+{
+	unsigned long flag_bits;
+
+	flag_bits = trans_bit(flags, MAP_GROWSDOWN, VM_GROWSDOWN) |
+		trans_bit(flags, MAP_DENYWRITE, VM_DENYWRITE) |
+		trans_bit(flags, MAP_EXECUTABLE, VM_EXECUTABLE);
 
-	unsigned long prot_bits, flag_bits;
-	prot_bits =
-		_trans(prot, PROT_READ, VM_READ) |
-		_trans(prot, PROT_WRITE, VM_WRITE) |
-		_trans(prot, PROT_EXEC, VM_EXEC);
-	flag_bits =
-		_trans(flags, MAP_GROWSDOWN, VM_GROWSDOWN) |
-		_trans(flags, MAP_DENYWRITE, VM_DENYWRITE) |
-		_trans(flags, MAP_EXECUTABLE, VM_EXECUTABLE);
-	return prot_bits | flag_bits;
-#undef _trans
+	return flag_bits;
 }
 
 #ifdef DEBUG_MM_RB
@@ -500,8 +491,8 @@
 	 * to. we assume access permissions have been handled by the open
 	 * of the memory object, so we don't do any here.
 	 */
-	vm_flags = calc_vm_flags(prot,flags) | mm->def_flags |
-			VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;
+	vm_flags = calc_vm_flags(flags) | calc_vm_prot(prot) | mm->def_flags | 
+		VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;
 
 	if (flags & MAP_LOCKED) {
 		if (!capable(CAP_IPC_LOCK))
Index: mm/mprotect.c
===================================================================
RCS file: /home/cvs/linux-2.5/mm/mprotect.c,v
retrieving revision 1.24
diff -u -u -r1.24 mprotect.c
--- mm/mprotect.c	3 Jul 2003 05:49:35 -0000	1.24
+++ mm/mprotect.c	4 Sep 2003 18:23:22 -0000
@@ -224,7 +224,7 @@
 asmlinkage long
 sys_mprotect(unsigned long start, size_t len, unsigned long prot)
 {
-	unsigned long nstart, end, tmp;
+	unsigned long nstart, end, tmp, flags; 
 	struct vm_area_struct * vma, * next, * prev;
 	int error = -EINVAL;
 
@@ -239,6 +239,8 @@
 	if (end == start)
 		return 0;
 
+	flags = calc_vm_prot(prot); 
+
 	down_write(&current->mm->mmap_sem);
 
 	vma = find_vma_prev(current->mm, start, &prev);
@@ -257,7 +259,7 @@
 			goto out;
 		}
 
-		newflags = prot | (vma->vm_flags & ~(PROT_READ | PROT_WRITE | PROT_EXEC));
+		newflags = flags | (vma->vm_flags & ~(VM_READ | VM_WRITE | VM_EXEC));
 		if ((newflags & ~(newflags >> 4)) & 0xf) {
 			error = -EACCES;
 			goto out;


-- 
Muli Ben-Yehuda
http://www.mulix.org


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Stop mprotect() changing MAP_SHARED and other cleanup

[Jamie Lokier]

Muli Ben-Yehuda wrote:
> > +/* Optimisation macro. */
> > +#define _calc_vm_trans(x,bit1,bit2) \
> > +  ((bit1) <= (bit2) ? ((x) & (bit1)) * ((bit2) / (bit1)) \
> > +   : ((x) & (bit1)) / ((bit1) / (bit2))
> 
> Why is this necessary? the original version of the macro was much
> simpler. If this isn't just for shaving a couple of optimization,
> please document it. If it is, I urge you to reconsider ;-) 

When the bits don't match, mine reduces to a mask-and-shift.  The
original reduces to a mask-and-conditional, which is usually slower.

Hopefully GCC optimises the latter to the former these days, but there
is no harm in helping.

I vaguely recall GCC being able to optimise (x&mask1) | (x&mask2) to
x&(mask1|mask2), not 100% sure though.  If so, the PROT_ bits
translation will reduce to prot & 7.

-- Jamie

Re: [PATCH] Stop mprotect() changing MAP_SHARED and other cleanup

[Jamie Lokier]

Jamie Lokier wrote:
> +#include <asm/mman.h>

This is a resend of the mprotect() fix with a small change.  The
previous version added an unnecessary #include to <asm/mm.h>.
This one doesn't.

Patch: mprotect-2.6.0-test4-2

This patch:

  1. Removes the logic bug in mprotect() which allows it to change
     MAP_SHARED if PROT_SEM is passed.

  2. Moves the mapping of PROT_* bits to VM_* bits from mmap.c to
     the common header file <linux/mman.h>.

  3. Uses that function in mprotect() the same as mmap().  Previously
     mprotect() assumed the PROT_* and VM_* bits were numerically
     the same, which is what caused the PROT_SEM bug.

  4. Fixes an unlikely overflow in vm_locked accounting.

Enjoy,
-- Jamie


diff -urN --exclude-from=dontdiff orig-2.6.0-test4/include/linux/mman.h mprotect-2.6.0-test4/include/linux/mman.h
--- orig-2.6.0-test4/include/linux/mman.h	2003-07-12 17:57:37.000000000 +0100
+++ mprotect-2.6.0-test4/include/linux/mman.h	2003-09-04 20:24:35.000000000 +0100
@@ -2,6 +2,7 @@
 #define _LINUX_MMAN_H
 
 #include <linux/config.h>
+#include <linux/mm.h>
 
 #include <asm/atomic.h>
 #include <asm/mman.h>
@@ -27,4 +28,32 @@
 	vm_acct_memory(-pages);
 }
 
+/* Optimisation macro. */
+#define _calc_vm_trans(x,bit1,bit2) \
+  ((bit1) <= (bit2) ? ((x) & (bit1)) * ((bit2) / (bit1)) \
+   : ((x) & (bit1)) / ((bit1) / (bit2)))
+
+/*
+ * Combine the mmap "prot" argument into "vm_flags" used internally.
+ */
+static inline unsigned long
+calc_vm_prot_bits(unsigned long prot)
+{
+	return _calc_vm_trans(prot, PROT_READ,  VM_READ ) |
+	       _calc_vm_trans(prot, PROT_WRITE, VM_WRITE) |
+	       _calc_vm_trans(prot, PROT_EXEC,  VM_EXEC );
+}
+
+/*
+ * Combine the mmap "flags" argument into "vm_flags" used internally.
+ */
+static inline unsigned long
+calc_vm_flag_bits(unsigned long flags)
+{
+	return _calc_vm_trans(flags, MAP_GROWSDOWN,  VM_GROWSDOWN ) |
+	       _calc_vm_trans(flags, MAP_DENYWRITE,  VM_DENYWRITE ) |
+	       _calc_vm_trans(flags, MAP_EXECUTABLE, VM_EXECUTABLE) |
+	       _calc_vm_trans(flags, MAP_LOCKED,     VM_LOCKED    );
+}
+
 #endif /* _LINUX_MMAN_H */
diff -urN --exclude-from=dontdiff orig-2.6.0-test4/include/linux/mm.h mprotect-2.6.0-test4/include/linux/mm.h
--- orig-2.6.0-test4/include/linux/mm.h	2003-09-02 23:06:10.000000000 +0100
+++ mprotect-2.6.0-test4/include/linux/mm.h	2003-09-04 20:18:21.000000000 +0100
@@ -85,12 +86,13 @@
 #define VM_READ		0x00000001	/* currently active flags */
 #define VM_WRITE	0x00000002
 #define VM_EXEC		0x00000004
-#define VM_SHARED	0x00000008
 
 #define VM_MAYREAD	0x00000010	/* limits for mprotect() etc */
 #define VM_MAYWRITE	0x00000020
 #define VM_MAYEXEC	0x00000040
-#define VM_MAYSHARE	0x00000080
+
+#define VM_SHARED	0x00000008	/* shared AND may write to pages */
+#define VM_MAYSHARE	0x00000080	/* set iff MAP_SHARED was set */
 
 #define VM_GROWSDOWN	0x00000100	/* general info on the segment */
 #define VM_GROWSUP	0x00000200
diff -urN --exclude-from=dontdiff orig-2.6.0-test4/mm/mmap.c mprotect-2.6.0-test4/mm/mmap.c
--- orig-2.6.0-test4/mm/mmap.c	2003-08-27 22:56:05.000000000 +0100
+++ mprotect-2.6.0-test4/mm/mmap.c	2003-09-04 20:10:02.000000000 +0100
@@ -136,29 +136,6 @@
 	return retval;
 }
 
-/* Combine the mmap "prot" and "flags" argument into one "vm_flags" used
- * internally. Essentially, translate the "PROT_xxx" and "MAP_xxx" bits
- * into "VM_xxx".
- */
-static inline unsigned long
-calc_vm_flags(unsigned long prot, unsigned long flags)
-{
-#define _trans(x,bit1,bit2) \
-((bit1==bit2)?(x&bit1):(x&bit1)?bit2:0)
-
-	unsigned long prot_bits, flag_bits;
-	prot_bits =
-		_trans(prot, PROT_READ, VM_READ) |
-		_trans(prot, PROT_WRITE, VM_WRITE) |
-		_trans(prot, PROT_EXEC, VM_EXEC);
-	flag_bits =
-		_trans(flags, MAP_GROWSDOWN, VM_GROWSDOWN) |
-		_trans(flags, MAP_DENYWRITE, VM_DENYWRITE) |
-		_trans(flags, MAP_EXECUTABLE, VM_EXECUTABLE);
-	return prot_bits | flag_bits;
-#undef _trans
-}
-
 #ifdef DEBUG_MM_RB
 static int browse_rb(struct rb_node * rb_node) {
 	int i = 0;
@@ -500,19 +477,17 @@
 	 * to. we assume access permissions have been handled by the open
 	 * of the memory object, so we don't do any here.
 	 */
-	vm_flags = calc_vm_flags(prot,flags) | mm->def_flags |
-			VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;
+	vm_flags = calc_vm_prot_bits(prot) | calc_vm_flag_bits(flags) |
+			mm->def_flags | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC;
 
-	if (flags & MAP_LOCKED) {
+	if (vm_flags & VM_LOCKED) {
+		unsigned long locked;
 		if (!capable(CAP_IPC_LOCK))
 			return -EPERM;
-		vm_flags |= VM_LOCKED;
-	}
-	/* mlock MCL_FUTURE? */
-	if (vm_flags & VM_LOCKED) {
-		unsigned long locked = mm->locked_vm << PAGE_SHIFT;
+		locked = (mm->locked_vm << PAGE_SHIFT);
 		locked += len;
-		if (locked > current->rlim[RLIMIT_MEMLOCK].rlim_cur)
+		if (locked < len || /* Overflow check. */
+		    locked > current->rlim[RLIMIT_MEMLOCK].rlim_cur)
 			return -EAGAIN;
 	}
 
diff -urN --exclude-from=dontdiff orig-2.6.0-test4/mm/mprotect.c mprotect-2.6.0-test4/mm/mprotect.c
--- orig-2.6.0-test4/mm/mprotect.c	2003-07-12 17:57:37.000000000 +0100
+++ mprotect-2.6.0-test4/mm/mprotect.c	2003-09-04 20:12:34.000000000 +0100
@@ -257,8 +257,11 @@
 			goto out;
 		}
 
-		newflags = prot | (vma->vm_flags & ~(PROT_READ | PROT_WRITE | PROT_EXEC));
-		if ((newflags & ~(newflags >> 4)) & 0xf) {
+		newflags = calc_vm_prot_bits(prot) |
+			(vma->vm_flags & ~(VM_READ | VM_WRITE | VM_EXEC));
+
+		/* Check against VM_MAYREAD, VM_MAYWRITE and VM_MAYEXEC. */
+		if ((newflags & ~(newflags >> 4)) & (VM_READ | VM_WRITE | VM_EXEC)) {
 			error = -EACCES;
 			goto out;
 		}

Re: [PATCH] Stop mprotect() changing MAP_SHARED and other cleanup

[Muli Ben-Yehuda]

[-- Attachment #1: Type: text/plain, Size: 990 bytes --]

On Thu, Sep 04, 2003 at 11:04:35PM +0100, Jamie Lokier wrote:
> Muli Ben-Yehuda wrote:
> > > +/* Optimisation macro. */
> > > +#define _calc_vm_trans(x,bit1,bit2) \
> > > +  ((bit1) <= (bit2) ? ((x) & (bit1)) * ((bit2) / (bit1)) \
> > > +   : ((x) & (bit1)) / ((bit1) / (bit2))
> > 
> > Why is this necessary? the original version of the macro was much
> > simpler. If this isn't just for shaving a couple of optimization,

I meant "shaving a couple of instructions", of course. 

> > please document it. If it is, I urge you to reconsider ;-) 
> 
> When the bits don't match, mine reduces to a mask-and-shift.  The
> original reduces to a mask-and-conditional, which is usually slower.

Ok. Your version is also incomprehensible (to me, at least) without
working it out using a pen and paper, whereas the original is clear
and concise. Are the saved CPU cycles worth the wasted programmer
cycles in this case? I doubt it.
-- 
Muli Ben-Yehuda
http://www.mulix.org


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]