[systemd-devel] [PATCH 21/26] hashmap: rewrite the implementation

[Michal Schmidt]

On 10/20/2014 08:42 PM, Lennart Poettering wrote:
> On Thu, 16.10.14 09:50, Michal Schmidt (mschmidt at redhat.com) wrote:
>> +/* Fields that all hashmap/set types must have */
>> +struct HashmapBase {
>> +        const struct hash_ops *hash_ops;  /* hash and compare ops to use */
>> +
>> +        union _packed_ {
>> +                struct indirect_storage indirect; /* if  has_indirect */
>> +                struct direct_storage direct;     /* if !has_indirect */
>> +        };
>> +
>> +        uint8_t type:2;             /* HASHMAP_TYPE_* */
> 
> Should probably be a named enum (see above)
> 
>> +        uint8_t has_indirect:1;     /* whether indirect storage is
>> used */
> 
> Should really be a "bool", no?

Interestingly, changing this confuses pahole greatly.
With all the three bitfields declared as uint8_t pahole showed the struct
layout sanely:

struct HashmapBase {
	const struct hash_ops  *   hash_ops;             /*     0     8 */
	union {
		struct indirect_storage indirect;        /*          39 */
		struct direct_storage direct;            /*          39 */
	};                                               /*     8    39 */
	uint8_t                    type:2;               /*    47: 6  1 */
	uint8_t                    has_indirect:1;       /*    47: 5  1 */
	uint8_t                    n_direct_entries:3;   /*    47: 2  1 */

	/* size: 48, cachelines: 1, members: 5 */
	/* bit_padding: 2 bits */
	/* last cacheline: 48 bytes */
};

With "type" changed to a named enum, "has_indirect" to bool,
and "n_direct_entries" to unsigned, pahole says:

base_type__name_to_size: base_type __unknown__
base_type__name_to_size: base_type __unknown__
die__process_inline_expansion: tag not supported (INVALID)!
base_type__name_to_size: base_type __unknown__
...
struct HashmapBase {
	const struct hash_ops  *   hash_ops;             /*     0     8 */
	union {
		struct indirect_storage indirect;        /*          39 */
		struct direct_storage direct;            /*          39 */
	};                                               /*     8    39 */

	/* Bitfield combined with next fields */

	enum HashmapType           type:2;               /*    44: 6  4 */

	/* XXX 22 bits hole, try to pack */
	/* Bitfield combined with next fields */

	_Bool                      has_indirect:1;       /*    47: 5  1 */

	/* Bitfield combined with previous fields */

	__unknown__                n_direct_entries:3;   /*    44: 2  0 */

	/* size: 48, cachelines: 1, members: 5 */
	/* bit holes: 1, sum bit holes: 22 bits */
	/* padding: 4 */
	/* bit_padding: 252 bits */
	/* last cacheline: 48 bytes */

	/* BRAIN FART ALERT! 48 != 48 + 0(holes), diff = 0 */

};

But as far as I can tell, generated code looks OK and it still runs
fine, so I won't worry about it.

Michal