On Thu, Jan 23, 2014 at 11:52:06AM -0800, Jonathan Nieder wrote:
> > After my patches, t5310 runs fine for me. I didn't try your patch, but
> > mine are similar. Let me know if you still see the problem (there may
> > simply be a bug in yours, but I didn't see it).
>
> I had left out a cast to unsigned, producing an overflow.
>
> My main worry about the patches is that they will probably run into
> an analagous problem to the one that v1.7.12-rc0~1^2~2 (block-sha1:
> avoid pointer conversion that violates alignment constraints,
> 2012-07-22) solved. By casting the pointer to (uint32_t *) we are
> telling the compiler it is 32-bit aligned (C99 section 6.3.2.3).
Yeah, maybe. We go via memcpy, which takes a "void *", so that part is
good. However, the new code looks like:
foo = align_ntohl(*(uint32_t *)ptr);
I think this probably works in practice because align_ntohl is inlined,
and any sane compiler will never actually load the variable. If we
change the signature of align_ntohl, we can do this:
uint32_t align_ntohl(void *ptr)
{
uint32_t x;
memcpy(x, ptr, sizeof(x));
return ntohl(x);
}
...
foo = align_ntohl(ptr);
The memcpy solution is taken from read-cache.c, but as we noted, it
probably hasn't been used a lot. The blk_sha1 get_be may be faster, as
it converts as it reads. However, the bulk of the data is copied via
a single memcpy and then modified in place. I don't know if that would
be faster or not (for a big-endian system it probably is, since we can
omit the modification loop entirely).
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
