How to not UB/segfault with mat4s fields in dyn-alloc'd struct arrays?

[metaleap]

I tried global #define CGLM_ALL_UNALIGNED 1 alone, I tried CGLM_ALIGN_MAT modifier for my mat4s field (in a struct MyStruct) alone, I tried both macros together, and also with both absent — I keep getting a segfault that UBSan translates to "runtime error: store to misaligned address 0x5555579d7ed0 for type 'struct MyStruct', which requires 32 byte alignment" — when writing MyStruct values into a dynamically-allocated MyStruct array (ie a MyStruct*).

N00bish of me to ask, but what's the way to go here? =)

_{(Notes. I did verify the segfault/UB at my end is directly due to the mere presence of mat4s fields — even with all their usage sites commented-out! Then, just by commenting-out these unused mat4s-typed struct fields too, the segfault during struct array realloc goes away; when I bring just any one of them back in, kaboom.}

_{I should add that the actual realloc call that heap-allocs those MyStruct arrays isn't hand-written but in a macro-generated List type's macro-generated Add method. Generally works totally fine for all other (non-mat4s-containing) struct types. So I'd rather not just willy-nilly hardcode some special-cased one-off alignment stuff into there, but anyway eager to learn about the options here =)}

[metaleap]

For now, I found the only workaround (an ugly one) that lets me keep mat4s fields (in structs that are heap-alloc'd in arrays) to be changing my old List Add method from realloc to a manually-written-out alloc / memcpy / free combination. Which is of course sub-optimal to native realloc in reality.

This must have come up before with cglm, since your docs mention dynamic allocation (but no clear how-to) — @recp what would be the standard practice to avoid such suboptimal workarounds as above? Considering simple plain-old structs like below?

[mghcoder]

You could use a pointer to matrix and use aligned memory allocation:

mat4s *m = aligned_alloc(_Alignof(mat4s), sizeof(mat4s));

For a single SceneNode allocation you could just do:

SceneNode *node = aligned_alloc(32, sizeof(SceneNode));

Another alternative would be to add padding to your struct in order to get aligned array members when doing array allocation:

/* this will also align the size of the struct to 32 bytes */
typedef struct __attribute__((__aligned__(32)) SceneNode { 
    mat4s localTransform;
    mat4s worldTransform;
    struct SceneNodes* children;
    struct SceneNode* parent;
    MeshAsset *mesh;
    /* compiler adds 8 bytes of padding here to align sizeof(SceneNode) to 32 bytes */
} SceneNode;

size_t array_length = 128;
SceneNode *array = aligned_alloc(_Alignof(SceneNode), sizeof(SceneNode) * array_length);

printf("sizeof(SceneNode): %zu\n", sizeof(SceneNode)); / * should print: sizeof(SceneNode): 160, which is 32 byte aligned. */

printf("_Alignof(SceneNode): %zu\n", _Alignof(SceneNode)); / * should print: _Alignof(SceneNode): 32 */

void *aligned_alloc(size_t alignment, size_t size); is part of the C11 standard.

The same can be done for "struct RenderObject". Just be careful and pack your mat4s members together to avoid wasting too many bytes to padding:

/* the bad struct */
typedef struct bad_dummy {
    mat4s rotation;
    struct dummy *child;
    /* compiler adds 24 bytes of padding here for aligning the offset of member mat4s translation; */
    mat4s translation;
    struct dummy *parent;
    /* compiler adds another 24 bytes of padding here */
    mat4s scale;
} bad_dummy_t;

/* the good struct */
typedef struct good_dummy {
    mat4s rotation;
    /* no padding added */
    mat4s translation;
    /* no padding added */
    mat4s scale;
    /* no padding added */
    struct dummy *child;
    struct dummy *parent;
    /* no padding added */
} good_dummy_t;

/* the good aligned struct */
typedef struct __attribute__((__aligned__(32)) good_aligned_dummy {
    mat4s rotation;
    /* no padding added */
    mat4s translation;
    /* no padding added */
    mat4s scale;
    /* no padding added */
    struct dummy *child;
    struct dummy *parent;
    /* compiler adds 16 bytes of padding here to align sizeof(good_aligned_dummy_t) to 32 bytes */
} good_aligned_dummy_t;

/* single struct is aligned */
bad_dummy_t *bad = aligned_alloc(_Alignof(bad), sizeof(*bad));
good_dummy_t *good = aligned_alloc(_Alignof(good), sizeof(*good));
good_aligned_dummy_t *aligned= aligned_alloc(_Alignof(aligned), sizeof(*aligned));

/* only the first member of the array is aligned */
size_t array_len = 128;
bad_dummy_t *bad = aligned_alloc(_Alignof(bad), sizeof(*bad) * array_len);
good_dummy_t *good = aligned_alloc(_Alignof(good), sizeof(*good) * array_len);

/* all members of the array are aligned */
size_t array_len = 128;
good_aligned_dummy_t *aligned= aligned_alloc(_Alignof(aligned), sizeof(*aligned) * array_len);

[metaleap]

Thx heaps @mghcoder , it's really weird tho, even with this struct re-definition:

typedef struct __attribute__((__aligned__(32))) SceneNode {
  mat4s              worldTransform;
  mat4s              localTransform;
  struct SceneNodes* children;
  struct SceneNode*  parent;
  MeshAsset*         mesh;
  Uint64             __pad; // theoretically unneeded, but anyway
} SceneNode;

as soon as I switch my array-growing from addAligned to add it still blows up with:

../../vkguide/vk_scenenode.c:7:1: runtime error: store to misaligned address 0x55555679c3f0 for type 'struct SceneNode', which requires 32 byte alignment
0x55555679c3f0: note: pointer points here
 00 00 00 00  00 00 00 00 00 00 00 00  b5 c5 47 be 00 00 08 3f  63 cf 73 3f 2d ad c7 bd  08 ea 93 be
              ^ 
    #0 0x555555932719 in SceneNodes_add (/home/_/c/c/z/.build/debug/vkguide.exec+0x3de719) (BuildId: 528b631a46a4ac3b71a08bc4203464c298bf9a19)
    #1 0x5555558b500c in vkeInitDefaultData (/home/_/c/c/z/.build/debug/vkguide.exec+0x36100c) (BuildId: 528b631a46a4ac3b71a08bc4203464c298bf9a19)
    #2 0x5555558b5487 in vkeInit (/home/_/c/c/z/.build/debug/vkguide.exec+0x361487) (BuildId: 528b631a46a4ac3b71a08bc4203464c298bf9a19)
    #3 0x5555558a6a9a in main (/home/_/c/c/z/.build/debug/vkguide.exec+0x352a9a) (BuildId: 528b631a46a4ac3b71a08bc4203464c298bf9a19)
    #4 0x7ffff6234e07  (/usr/lib/libc.so.6+0x25e07) (BuildId: aed3a2b0cf4e6cc12296052529af22f6a450a75a)
    #5 0x7ffff6234ecb in __libc_start_main (/usr/lib/libc.so.6+0x25ecb) (BuildId: aed3a2b0cf4e6cc12296052529af22f6a450a75a)
    #6 0x5555558a43e4 in _start (/home/_/c/c/z/.build/debug/vkguide.exec+0x3503e4) (BuildId: 528b631a46a4ac3b71a08bc4203464c298bf9a19)

That's after deleting build dir, doing ccache --clean and ccache --clear and a full rebuild.

But it's totally frankenbuggy, with this result I can just swap the ordering of the 2 consecutive mat4s fields (ie. move the second one just above the first one) and it works. Change sth else and same crash again. Totally random. Seems like I gotta live with slightly-suboptimal addAligned (malloc/free/memcpy) instead of realloc-based add.. 😵‍💫

As for those prints you exemplified above, clangd below agrees =)

[mghcoder]

What is blowing up your alignment is the realloc() call in add

It seems random probably because sometimes realloc() is returning 32 byte aligned addresses and sometimes addresses aligned to less than 32 bytes (then the memory sanitizer throws an exception).

The C standard has no aligned_realloc(), Windows has _aligned_realloc(). As for other OSs, there might be some malloc like library that offers aligned_realloc(). When sticking with the standard library, you could do what you already do, aligned_alloc() + memcpy(); or request extra bytes to realloc(), align the address and memmove():

type struct list {
    void *ptr;
    node_t *nodes;
    size_t nodes_offset;
} list_t;
list_t list;
list.nodes = list.ptr = aligned_alloc(_Alignof(list.nodes), sizeof(*list.nodes) * capacity);
list.nodes_offset = 0;
list->ptr = realloc(list.ptr, sizeof(*list.nodes) * new_capacity + _Alignof(list.nodes));
list.nodes_offset = 32 - ((uintptr_t)list.ptr & 31ULL);
list.nodes = (node_t*)((uintptr_t)list.ptr + list.nodes_offset);
memmove(list.nodes, list.ptr, sizeof(*list.nodes) * capacity);
free(list.ptr);

Don't know which is better, aligned_alloc() + memcpy() or realloc() + offset + memmove(). The former is probably less prone to bugs, the latter would have to be well contained.

[recp]

Hi @metaleap,

The CGLM library makes alignment optional via the CGLM_ALL_UNALIGNED macro, but in the struct API (actually for mat4 too), alignment is hard-coded through CGLM_ALIGN_MAT. In the mat4s definition, if AVX is enabled, the type gets a 32-byte alignment (or 16-byte otherwise), regardless of CGLM_ALL_UNALIGNED. Since a struct’s alignment is dictated by its most strictly aligned member, any struct containing a mat4s will inherit that alignment requirement.

To fully respect CGLM_ALL_UNALIGNED, we might consider using CGLM_ALIGN_IF() instead of CGLM_ALIGN() in CGLM_ALIGN_MAT. However, this change should only be made once we’re confident it won’t introduce unintended side effects.

In its current definition, CGLM_ALIGN doesn't depend on CGLM_ALL_UNALIGNED:

#ifdef __AVX__
#  define CGLM_ALIGN_MAT CGLM_ALIGN(32)
#else
#  define CGLM_ALIGN_MAT CGLM_ALIGN(16)
#endif

[metaleap]

Thx @recp , really glad about cglm by the way I should add — am progressing well thru vkguide.dev with it these days =)

In its current definition, CGLM_ALIGN doesn't depend on CGLM_ALL_UNALIGNED

Yup I noticed =) actually don't mind these wise choices & defaults, I was just sort of hungering for some way to nudge / tweak those structs such that realloc'd arrays of them can be stored into. Kinda like what @mghcoder outlined nicely there (and I briefly played with similar yesterday, just more blindly, by trying manual padding bytes but also misplacing or mispelling that aligned attr I think) but somehow those struct tricks result in frankenbuggy outcomes — after some builds, it all works, later I change some totally-unrelated triviality elsewhere causing a rebuild and boom, crash, back to addAligned. Really has me stumped 😁 but clearly also not a cglm issue whatsoever =)