I would rather add those padding pointers next to another pointer, than at the end of the struct. It helps visualizing/understanding that pointers should always be in sets of even numbers in a struct to ensure proper alignment on both 32 and 64bits systems?

Not sure what @Brecht Van Lommel (brecht) or @Campbell Barton (campbellbarton) think about it?

Is indeed easiest to put void* padding next to pointers.

From own experience with arm64 platform this might or might not be enough, as the whole struct size might be padded with some rules which are not immediately clear to me. Someone with rpi can easily see this.

Proper fix for this kind of eternal cycle of tweaking padding would be to make dna_verify.c the source of truth for sizes/alignments. I don't get it why we try to do manual calculation so that then we have to verify it against what compiler tells it should be.

Bastien Montagne (mont29) added subscribers: Campbell Barton (campbellbarton), Brecht Van Lommel (brecht), Bastien Montagne (mont29).

I added the platform builds and test group, since it had all the relevant people in it, I'm somewhat confused on when to add groups and when to add individuals, any chance someone can clarify this?

I would rather add those padding pointers next to another pointer, than at the end of the struct. It helps visualizing/understanding that pointers should always be in sets of even numbers in a struct to ensure proper alignment on both 32 and 64bits systems?

I'm gonna disagree there, the pointer themselves are not at fault here and not every pointer needs a "buddy pointer" to keep it out of trouble.

lets walk though an example

typedef struct struct_with_3_pointers
{
    void *a;
    void *b;
    void *c;
} struct_with_3_pointers;

That's pretty straight forward, no issues, lets add something

typedef struct struct_with_3_pointers_and_an_int
{
    struct_with_3_pointers a;
    int x;
} struct_with_3_pointers_and_an_int;

This needs 4 bytes of padding on 64 bit, on 32 bit the alignment requirement is 4, so adding 4 more is not an issue

and the final structure becomes

typedef struct struct_with_3_pointers_and_an_int
{
    struct_with_3_pointers a;
    int x;
    char _pad[4];  // or a single int, whatever floats your boat, we do not seem very consistent in this regard
} struct_with_3_pointers_and_an_int;

Now lets ruin everything and add a 64 bit field to the party like for instance our friend the SessionUUID

typedef struct SessionUUID {
  uint64_t uuid_;
} SessionUUID;

So far so good, now lets put it all together

typedef struct struct_with_3_pointers_and_an_int_and_a_session_ID
{
    struct_with_3_pointers a;
    int x;
    SessionUUID y;
    char _pad[4];  // or a single int, whatever floats your boat, we do not seem very consistent in this regard
} struct_with_3_pointers_and_an_int_and_a_session_ID;

The only way to make both architectures happy here is to add a padding pointer

typedef struct struct_with_3_pointers_and_an_int_and_a_session_ID
{
    struct_with_3_pointers a;
    int x;
    SessionUUID y;
    char _pad[4];  // or a single int, whatever floats your boat, we do not seem very consistent in this regard
    void *_pad1;
} struct_with_3_pointers_and_an_int_and_a_session_ID;

glorious success.

Now who is at fault? The pointers in struct_with_3_pointers are clearly not the issue since it was fine on its own, it was even fine in struct_with_3_pointers_and_an_int it wasn't until we mixed it with a sessionID in struct_with_3_pointers_and_an_int_and_a_session_ID things broke down. The sore spot is clearly the "combination" struct.

which leads me to to disagree with "every pointer needs a buddy pointer" the issue is not the pointer, the issue is a mix of pointers with a 64 bit variable.

this issue is not a field padding issue, it is a struct size padding issue and struct size padding happens at the end of a struct, not arbitrarily next to a pointer that may or may not cause issues in the future.

I don't get it why we try to do manual calculation so that then we have to verify it against what compiler tells it should be.

the sole reason dna_verify.c exists is because makesdna is bad at what it does (it let all the issues in this diff slide by, but dna_verify.c cought them), the compiler and makesdna did not agree on the sizes and field offsets leading to heap corruption issues (T63164) to combat this, we added a sanity check to verify everything has the size and location we think it should have at compile time. Why does makesdna count manually? no idea, makesdna existed a long long time before I joined, I bet there's a reason, I'm just unaware of it. if i had to guess, it is to have an alert for both 32 bit and 64 bit alignment issues?

if we want to replace makesdna or give it a large make over that be a story for a different diff, this one is about correcting the bad things the current version let slip by.

The debian guys dropped by today, they are still getting bitten by alignment errors for their 32 bit builds, this hopefully fixes it for master, I gave them a probable fix for 2.93.2 in P2313

we can't keep going like this, either makesdna has to get it together and detect these issues or we should offer devs a tool like in D9465 to be alerted/fix to these issues,

@Campbell Barton (campbellbarton) wasn't super in having another C/C++ parser, so D9465 may not be an option.

@Ray Molenkamp (LazyDodo), Thanks for the update!

One thing which seems wrong to me is the fact that we are building a tools on top of the makesdna to perform validness checks. Other than need-to-be-careful when doing changes in makesdna, why can't we convert dna_verify.c to become the source of truth for offsets and sizes? What are we gaining by replicating those calculations on our side?

I have on clue why makesdna is the way it is, long long before my time, judging on how the code looks, it may actually be the oldest code we have in our codebase?