Fix warnings issued by -Wpadded (gcc/clang).

[silvioprog]

This PR fixes the following errors (passing -Wall -Werror -Wextra -Wpedantic -Wpadded):

src/uthash.h:1177:27: error: padding struct to align ‘tail’ [-Werror=padded]
    struct UT_hash_handle *tail; /* tail hh in app order, for fast append    */
                           ^
src/uthash.h:1204:1: error: padding struct size to alignment boundary [-Werror=padded]
 } UT_hash_table;

[Quuxplusone]

This is a duplicate of #118, where the state is "we'll take a patch, but it's got to preserve ABI compatibility somehow."

[silvioprog]

Hm... I'm not sure about the progress in #118. 😕 However, if understood well, a possible way to fix it would be adding a -DUTHASH_OLD_LAYOUT. If you prefer I can edit my PR implementing the flag UTHASH_OLD_LAYOUT.

[silvioprog]

Hey dude, what do you think about to add -DUTHASH_OLD_LAYOUT as you suggested at #118? It will fix the problem in a simple way, if someone finds a better solution just send a new patch.

[Quuxplusone]

I think this patch would be improved by your adding -DUTHASH_OLD_LAYOUT (under the name -DUTHASH_V2_ABI, perhaps?). I might still be wary of accepting it; but I wouldn't accept it without such a macro.
I'd think that we'd want to add that macro to opt-into the old ABI, and then immediately tag a "v3.0.0" release.
The other thing to look at here is: Will you just have to do the whole process over again as soon as they invent 128-bit pointers? 128-bit long long? Or can you arrange the elements in a future-proof way that won't warn in the future either?
And, what are the performance implications if any? I don't have any benchmarks for uthash.
And, check the open issues to see if you can fix all the ABI-related issues in one patch.

[silvioprog]

The commit 93d1dfc adds the UTHASH_V2_ABI. I'm not sure if it is the best way to fix this problem to finally make uthash conformable to -Wpadded, but it compiles properly and reduced the UT_hash_table size from 64 to 56. Any change in my PR are welcome. :-)

[Quuxplusone]

I see why you did this, but actually I meant UTHASH_V2_ABI to be the current/old ABI (because the current uthash version number is 2.x). Anyway, I've got a more future-proof(?) idea now:

#ifndef UTHASH_ABI_VERSION
#define UTHASH_ABI_VERSION 3
#endif

#if UTHASH_ABI_VERSION >= 3
    tail, hho, num_buckets, log2_num_buckets, num_items
#else
    num_buckets, log2_num_buckets, num_items, tail, hho
#endif

This would make it easier to jump again to V4, V5,... if we needed to, while ensuring that anyone who needed ABI stability would only ever have to learn one pattern: -DUTHASH_ABI_VERSION=(whatever today's major version number is).

[silvioprog]

+1 for UTHASH_ABI_VERSION. We should upgrade the UTHASH_VERSION to 3.0.0 too, what do you thing?

[Quuxplusone]

As long as you're changing the struct layout, I wonder whether we should add explicit "padding" fields at the end of the struct, since (with the Bloom filter) it currently contains 8x+5 bytes in the 32-bit case and 8x+1 bytes in the 64-bit case. But I worry that the same people complaining about the difference between 56 bytes and 64 bytes might just as well start complaining about not being able to store things in the tail-padding of this struct. 😛

[silvioprog]

What do you thing about to declare two entire separated structs? For example:

#if UTHASH_ABI_VERSION >= 3
typedef struct UT_hash_table {
  all >=3 members ...
}
#else
typedef struct UT_hash_table {
  all <=2 members ...
}
#endif

Using -Werror=padded/-Wpadded we can't declare tail/hho as tail in the struct because error "padding struct to align ‘tail’".

[Quuxplusone]

By "tail-padding", I meant "padding after the last member of the struct whatever it is," not "padding before/after the member whose name is tail."

[silvioprog]

I'm not sure if I understood "padding after the last member of the struct whatever it is" correct, but it compiles:

gcc main.c

main.c

struct foo {
    int a;
    char *b;
};

int main() {
    struct foo bar;
    printf("%zd - %zd\n", sizeof(bar.a), sizeof(bar.b));
    return 0;
}

it raises error at compile time (because b is larger than a):

gcc -Wpadded main.c 

main.c:5:11: error: padding struct to align ‘b’ [-Wpadded]
     char *b;
           ^
cc1: some warnings being treated as errors

[silvioprog]

The GCC/Clang has the attribute __attribute__((packed)), but I'm not attribute expert so I don't know any performance impact by adding such option in the uthash struct. Anyway, the following code compiles fine even using -Wpadded:

typedef struct
#if UTHASH_ABI_VERSION >= 3
  __attribute__((packed))
#endif
UT_hash_table {
   UT_hash_bucket *buckets;
   unsigned num_buckets, log2_num_buckets, num_items;
   struct UT_hash_handle *tail;
   ptrdiff_t hho;
   unsigned ideal_chain_maxlen;
...
} UT_hash_table;

[silvioprog]

Hello @Quuxplusone . It seems the -Wpadded options is a little problematic: https://gnunet.org/bugs/view.php?id=5302 . I'm going to remove it from my building system. 😅

[Quuxplusone]

Christian Grothoff is blunter than I am, but I think he's got essentially the right attitude on this one. 😛

[silvioprog]

Hehehe