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

[Nicolas Pitre <nico@cam.org>]

On Wed, 30 May 2007, Linus Torvalds wrote:

> On Wed, 30 May 2007, Nicolas Pitre wrote:
> > 
> > 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..

Well, I did it the way I did for two reasons, the first being that 
random hexval() usage won't mask a bad value if it is passed an int 
which happens to be out of range (think EOF, or better yet -208, -207, 
etc).

Yet gcc appears to be smart enough to skip the test if it is passed a 
value that cannot exceed the test range, like an unsigned char.

> 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.

My patch already produces code that looks like that for get_sha1_hex() 
on i386.  But my second reason for the patch is that on ARM it 
should becomes:

	/* r4 = hexval_table, ip = hex */
	ldrb	r1, [ip], #1
	ldrb    r2, [ip], #1
	ldrsb   r0, [r4, r1]
	ldrsb   r3, [r4, r2]
	orr     r0, r3, r0, asl #4

I.e. the compiler has a greater chance to fold the post increment with 
the load byte instruction as above without the need for an extra
add instruction.


Nicolas