I don't think this is required. Seems like the PR has shown it isn't.
That's because sizeof(item) is aligned already.

It's not sure. E.g. -Os can pack the structs automatically, so its size can be e.g. 27 bytes

No that's not possible. Else, if you had an array of those structures, the 2nd element in the array would cause unaligned access. The compiler must align the structure field to at least the most constrained field. It can pad the structure size (so if you do sizeof(*item) on a 27 bytes structure, it'll be 28 not 27, even if only the first 27 bytes are used in the structure).

In all cases, I don't think it's an issue, since if you ALIGN something that's already aligned, it'll give you the same number. It just make the code harder to follow.

No that's not possible. Else, if you had an array of those structures, the 2nd element in the array would cause unaligned access.

GCC is a little too smart sometimes. I don't know if it happens specifically in this case, but if it knows the target CPU (e.g. Cortex-M7) can support unaligned accesses, and you don't explicitly tell it not to use them with -mno-unaligned-access, I have absolutely seen it generate code that uses unaligned accesses like this.

I remember that we had issues with GCC packing structs that it wasn't able to read later. That's why we have introduced LV_ATTRIBUTE_MEM_ALIGN in lv_conf.h.

My experience is that there are too many exotic compilers and hardware so we should do all we can to support special cases too. Especially if it's so simple.

If the CPU can support unaligned access, then there's no alignment constraint anymore, right?

Damn, this makes my head hurt. You are right, there's the standard on one side with all its rules and there's GCC that bending them to its will and its strong knowledge of each platform. I'm 99% sure that because of "array like" access, structs must be aligned. Even an empty struct has a non zero sizeof because of this, the second element's address must be different than the former.

I don't know all architecture LVGL builds upon. But on ARM and ESP32 (which are both platform with alignment constraint I know about), their rules are quite simple, to access a 16 bits variable, it must be on (at least) a 16 bits boundary, and so on.
So, on those platform, the ALIGN() above will do nothing since it'll happen for both pointers if the item is aligned (which is ensured by my initial PR's ALIGN usage). The last map (error_acc might be problematic since its 24 bits wide), but since it's the last, it won't push the other map to unaligned positions. This last map might require alignment to 32 bits or 16 bits, I don't know, probably need testing on a platform where alignment is an issue.

@kisvegabor If I guess correctly, you are using a ARM cpu (a mac ?) so you can assert my claims by removing those ALIGN lines and it should still work.

Please notice that keeping them, at worst, waste memory since any 16bits variable (lv_color_t get pushed to a 32bits or 64bits boundary). So at best, we are wasting 6 bytes per gradient (3 maps x 32 bits alignment) and 12 bytes at worst (3 maps x 64 bits alignment) for this maybe useless alignment constraint. Since we can have many of those when animating/occluding/etc... it might worth double checking to avoid this waste.

I can't test it on an ARM based MAC at this moment but I'd accept the waste for higher reliability.

We could save much more if we didn't allocate error_acc and hmap if LV_DITHER_NONE is used.

Anyway, I'd like to merge this fix as soon as possible to not have broken code in master and we can think about improvement options later.

Anyway, I'd like to merge this fix as soon as possible to not have broken code in master and we can think about improvement options later.

Sure, feel free to merge. We can change later on to allocate upon requirement (by passing the dither argument to the gradient_get function)