Re: [PATCH] Fix type-punning issues

[Junio C Hamano <gitster@pobox.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.

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

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

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

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