Re: struct hashmap_entry packing

[Karsten Blees <karsten.blees@gmail.com>]

Am 28.07.2014 19:17, schrieb Jeff King:
> Hi Karsten,
> 
> The hashmap_entry documentation claims:
> 
>   `struct hashmap_entry`::
> 
> 	An opaque structure representing an entry in the hash table,
> 	which must be used as first member of user data structures.
> 	Ideally it should be followed by an int-sized member to prevent
> 	unused memory on 64-bit systems due to alignment.
> 
> I'm not sure if the statement about alignment is true. If I have a
> struct like:
> 
>     struct magic {
> 	    struct hashmap_entry map;
> 	    int x;
>     };
> 
> the statement above implies that I should be able to fit this into only
> 16 bytes on an LP64 system. But I can't convince gcc to do it. And I
> think that makes sense, if you consider code like:
> 
>    memset(&magic.map, 0, sizeof(struct hashmap_entry));
> 
> The sizeof() has to be the same regardless of whether the hashmap_entry
> is standalone or in another struct, and therefore must be padded up to
> 16 bytes. If we stored "x" in that padding in the combined struct, it
> would be overwritten by our memset.
> 

The struct-packing patch was ultimately dropped because there was no way
to reliably make it work on all platforms. See [1] for discussion, [2] for
the final, 'most compatible' version.

> Am I missing anything? If this is the case, we should probably drop that
> bit from the documentation.

Hmmm. Now that we have "__attribute__((packed))" in pack-bitmap.h, perhaps
we should do the same for stuct hashmap_entry? (Which was the original
proposal anyway...). Only works for GCC, but that should cover most builds
/ platforms.

Btw.: Using struct-packing on 'struct bitmap_disk_entry' means that the
binary format of .bitmap files is incompatible between GCC and other
builds, correct?

> It's possible that we could get around it by
> embedding the hashmap_entry elements directly into the parent struct,

Already tried that, see [3].

[1] http://article.gmane.org/gmane.comp.version-control.git/239069
[2] http://article.gmane.org/gmane.comp.version-control.git/241865
[3] http://article.gmane.org/gmane.comp.version-control.git/239435