[prev in list] [next in list] [prev in thread] [next in thread]
List: git
Subject: Re: [PATCH] fix signed range problems with hex conversions
From: Nicolas Pitre <nico () cam ! org>
Date: 2007-05-30 18:20:21
Message-ID: alpine.LFD.0.99.0705301346580.11491 () xanadu ! home
[Download RAW message or body]
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
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic