Re: [PATCH] fix signed range problems with hex conversions

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: May 30, 2007, 17:32 UTC
Message-ID: <alpine.LFD.0.98.0705301025170.26602@woody.linux-foundation.org>
In-Reply-To: <alpine.LFD.0.99.0705301213450.11491@xanadu.home>

On Wed, 30 May 2007, Nicolas Pitre wrote:

Show 5 quoted lines

> 
> This patch adds a range test to hexval() in order to prevent this.  Also 
> let's index the hexval_table array directly in get_sha1_hex() using 
> explicitly unsigned chars to avoid the range test producing faster
> code.

Please just make hexval() take a "unsigned char" instead, solving the problem in a manner that makes it much easier for the compiler to realize that it never needs to sign-extend the value or test the end result..

Ie I think your patch would be better off something like the following instead (it would be a one-liner, but I agree that marking hexval_table "const" is also a good idea).

With this, gcc can just generate:

	movzbl  (%rdi), %eax
	movsbl  hexval_table(%rax),%edx
	movzbl  1(%rdi), %eax
	movsbl  hexval_table(%rax),%eax
	sall    $4, %edx
	orl     %eax, %edx

for the code to generate a byte from two hex characters.

		Linus

----

diff --git a/cache.h b/cache.h
index ec85d93..0da7070 100644
--- a/cache.h
+++ b/cache.h
@@ -359,8 +359,8 @@ extern void *map_sha1_file(const unsigned char *sha1, unsigned long *);
 extern int has_pack_file(const unsigned char *sha1);
 extern int has_pack_index(const unsigned char *sha1);
 
-extern signed char hexval_table[256];
-static inline unsigned int hexval(unsigned int c)
+extern const signed char hexval_table[256];
+static inline unsigned int hexval(unsigned char c)
 {
 	return hexval_table[c];
 }
diff --git a/sha1_file.c b/sha1_file.c
index 12d2ef2..f959fe5 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -33,7 +33,7 @@ const unsigned char null_sha1[20];
 
 static unsigned int sha1_file_open_flag = O_NOATIME;
 
-signed char hexval_table[256] = {
+const signed char hexval_table[256] = {
 	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 00-07 */
 	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 08-0f */
 	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 10-17 */