Re: [PATCH] Fix type-punning issues
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 12, 2009, 07:57 UTC
- Message-ID
- <7vy6t292ix.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1242091058-25197-1-git-send-email-dpmcgee@gmail.com>
Dan McGee <dpmcgee@gmail.com> writes:
> In these two places we are casting part of our unsigned char sha1 array into > an unsigned int, which violates GCCs strict-aliasing rules (and probably > other compilers).
Yay.
Show 7 quoted lines
> static unsigned int hash_obj(const struct object *obj, unsigned int n)
> {
> - unsigned int hash = *(unsigned int *)obj->sha1;
> + unsigned int hash;
> + memcpy(&hash, obj->sha1, sizeof(unsigned int));
> return hash % n;
> }I noticed this breakage when I borrowed a friend's FC11 preview (as I still do not have a replacement machine X-<), but didn't manage to spend enough time to fix it myself. I was hoping a way to tell the compiler that this particular pointer usage is Ok in a less hacky way that does not upset older compilers, without resorting to low-level memcpy. But your version to use memcpy() is a literal translation of what the code does, and I think it is an acceptable solution.
Show 6 quoted lines
> @@ -16,7 +17,7 @@ static void *insert_decoration(struct decoration *n, const struct object *base,
> {
> int size = n->size;
> struct object_decoration *hash = n->hash;
> - int j = hash_obj(base, size);
> + unsigned int j = hash_obj(base, size);These type changes should be Ok, but I would have preferred them to be a separate patch. From the context, you cannot see if the function in places outside the patch context uses "j" for purposes other than holding the return value of hash_obj() that requires it to be able to hold a netagive value to make sure that this conversion is correct; on the other hand, we know n->size is a sane small value that unsigned vs signed int does not matter, so in that sense making this change in the same patch makes it not about fixing pointer aliasing warning.
But the code here is correct (I read outside the context).
Show 6 quoted lines
> @@ -68,7 +69,7 @@ void *add_decoration(struct decoration *n, const struct object *obj,
> /* Lookup a decoration pointer */
> void *lookup_decoration(struct decoration *n, const struct object *obj)
> {
> - int j;
> + unsigned int j;Same here.
Show 16 quoted lines
> diff --git a/object.c b/object.c
> index 7e6a92c..96ef32d 100644
> --- a/object.c
> +++ b/object.c
> @@ -43,15 +43,16 @@ int type_from_string(const char *str)
> die("invalid object type \"%s\"", str);
> }
>
> -static unsigned int hash_obj(struct object *obj, unsigned int n)
> +static unsigned int hash_char(const unsigned char *sha1, unsigned int n)
> {
> - unsigned int hash = *(unsigned int *)obj->sha1;
> - return hash % n;
> + unsigned int i;
> + memcpy(&i, sha1, sizeof(unsigned int));
> + return (int)(i % n);Huh? The original looks the same as the one in decorate.c but why is the conversion different? I am not talking about the difference between hash and i, but am wondering about the cast to int (and then back to unsigned int by the function signature).
It might make more sense to have one canonical
unsigned int hash_obj(const struct object *obj, unsigned int n)
here, export it to object.h, and remove the one in decorate.c.
Or am I missing something?