Re: [PATCH] Fix git-pack-objects for 64-bit platforms

From: Junio C Hamano <junkio@cox.net>
Date: May 11, 2006, 18:52 UTC
Message-ID: <7v7j4swg0r.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.64.0605111054290.3866@g5.osdl.org>

Linus Torvalds <torvalds@osdl.org> writes:

Show 12 quoted lines

> On Thu, 11 May 2006, Dennis Stosberg wrote:
>> 
>> I am not sure whether an int cast or an int32_t cast is more
>> appropriate here.  An int is not guaranteed to be four bytes wide,
>> but I don't know of any modern platform where that's not the case.
>> On the other hand int32_t is not necessarily available before C99.
>> 
>> Any opinions?  I wonder why no one has hit this on x86_64...
>
> I think the "ntohl()" hides it. It loads a 64-bit value, but since x86-64 
> is little-endian, the low 32 bits are correct. The htonl() will then strip 
> the high bits and make it all be big-endian.

That sounds sensible.

Since I saw a patch that touches only one place, I thought I'd better point this out...

There are a few more places that knows about this ((char*)base_pointer + (entry_count * 24)) magic in our code.

$ git grep -n -e '24  *\*' -e '\*  *24' master -- '*.c'

master:pack-objects.c:159:		long hl = *((long *)(index + 24 * i));

	This is yours.

master:sha1_file.c:447: if (idx_size != 4*256 + nr * 24 + 20 + 20) master:sha1_file.c:1148: memcpy(sha1, (index + 24 * n + 4), 20); master:sha1_file.c:1162: int cmp = memcmp(index + 24 * mi + 4, sha1, 20);

	These three should be OK, I think.

master:sha1_file.c:1164:			e->offset = ntohl(*((int*)(index + 24 * mi)));

	This you might want to look at; I suspect it is what
	Linus suggests.

Also we _might_ have uglier magic that assumes the base_pointer to be a pointer to a 4-byte integer and uses offset of multiple of 6 instead of 24, although I do not think it is likely.

I have to leave the keyboard in a few minutes so I cannot verify nor fix them myself for the next 8 hours or so. Sorry.

Show 6 quoted lines

> And while I actually run a 64-bit big-endian machine myself (G5 ppc64), my 
> user space is all 32-bit by default, so it never showed up on linux-ppc64 
> either.
>
> Anyway, the correct type to use is "uint32_t" in this case. That's what 
> htonl() takes.

Is uint32_t guaranteed to be exactly 32-bit, or merely enough to hold 32-bit?