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

From: Nicolas Pitre <nico@cam.org>
Date: May 30, 2007, 18:20 UTC
Message-ID: <alpine.LFD.0.99.0705301346580.11491@xanadu.home>
In-Reply-To: <alpine.LFD.0.98.0705301025170.26602@woody.linux-foundation.org>

On Wed, 30 May 2007, Linus Torvalds wrote:

Show 10 quoted lines

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

Show 14 quoted lines

> 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