malloc results are not aligned to alignof(max_align_t)

[Akaricchi]

It looks like malloced pointers are aligned to 8 bytes, but alignof(max_align_t) returns 16. This is a violation of the C standard; max_align_t is supposed to reflect the minimum malloc alignment.

This has caused me a great deal of debugging confusion today...

[sbc100]

Yes, this is due to dlmalloc:
https://github.com/emscripten-core/emscripten/blob/incoming/system/lib/dlmalloc.c#L79

emscripten/system/lib/dlmalloc.c

Line 284 in 748c80c

MALLOC_ALIGNMENT default: (size_t)(2 * sizeof(void *))

I default to 2 * sizeof(ptr) here:

emscripten/system/lib/dlmalloc.c

Line 661 in 748c80c

#define MALLOC_ALIGNMENT ((size_t)(2 * sizeof(void *)))

We should probably update it to use max_align_t. To be fair when dlmalloc was written that type did not exist :). There will be some cost in memory usage / fragmentation, but perhaps its worth it?

Out of interest what how was this issue manifesting? Are you using v128 types?

[Akaricchi]

The case is not very interesting and it's kind of stupid really. In short, my code can actually work with 8-byte alignment just fine. It just has lots of pointer casts, compiles with -Wcast-align, and contains paranoid amounts of __builtin_assume_aligned and alignment assertions (abstracted away with macros). Lots of pointers are cast between a "base" struct and derived types (this is C, not C++), and for simplicity the base struct has a forced alignment requirement of max_align_t. Everything just happens to work fine with NDEBUG, but otherwise the assertions are failing.

Are you using v128 types?

max_align_t actually only needs to be just strict enough for the primitive types. It's expected and fine that SIMD stuff typically needs to be over-aligned. The only primitive type that's actually 16-byte aligned right now is long double, which does not exist in wasm natively. I'm not sure about this, but I suspect that it doesn't actually need that 16-byte alignment. So fixing that and lowering max_align_t to 8 may be a better option.

[sbc100]

The fact that long double is 16 bytes in webassembly is a very long running source of contention. This is just one of the many annoying side effect of that choice. Unfortunately I think that ship has sailed.

[Akaricchi]

Interesting. What was the reasoning behind that choice, and what's the main difficulty with reverting it? Maybe there's some previous discussion of this that I can read up on?

[Akaricchi]

To be clear, my point is that long double could still be 16 bytes in size, but 8-byte aligned, unless I'm missing something here.

[sbc100]

I didn't know that, are there existing types in llvm where _Alignof(x) < sizeof(x)?

Regarding the old debate about sizeof(long double) I don't have links to discussions to hand but I doubt this issue will be significant enough to warrant change the ABI. I think we can just change dlmalloc without incurring too much cost (am I wrong?)

[Akaricchi]

I didn't know that, are there existing types in llvm where _Alignof(x) < sizeof(x)?

All the _Complex types, though I'm not sure how exactly llvm handles these. They are defined to be value pairs by the C standard (i.e. layout of _Complex double is equivalent to double[2], where the real part comes first). So they typically have the same alignment as the base type, but double the size.

On x86_64 with -malign-double, clang makes long double 8-byte aligned but still 16 bytes in size: https://godbolt.org/z/m66mUR
Interestingly, the behavior of that switch seems to contradict the GCC description of it.

On x86_32, 4 is the natural alignment of both double and long double, and sizeof(long double) is 12: https://godbolt.org/z/xFhDyW

[sbc100]

Well, how about we fix dlmalloc to honor max_align_t in emscripten then we will continue to do the right thing whichever way that discussion might go? There is no reason this issue needs to blocked on changinng wasms ABI.

[Akaricchi]

Oh sure, I wasn't implying it should be blocked. Fixing dlmalloc is fine and I don't think it'll cost too much.

Though I do hope for a future ABI change. long double is not used very often, and an alignment requirement relaxation doesn't seem too invasive to me.

[juj]

Oh sure, I wasn't implying it should be blocked. Fixing dlmalloc is fine and I don't think it'll cost too much.

Though I do hope for a future ABI change. long double is not used very often, and an alignment requirement relaxation doesn't seem too invasive to me.

This is an odd reasoning. Switching dlmalloc alignment from 8 to 16 bytes is fine and does not cost too much, but long double being 16 byte aligned is too invasive and would like to be 8 bytes instead?

Way way more applications use dlmalloc compared to long doubles, so not quite sure how/why worsening dlmalloc alignment is then perceived ok, but that same alignment on long double is invasive?

[Akaricchi]

Switching dlmalloc alignment from 8 to 16 bytes is fine and does not cost too much, but long double being 16 byte aligned is too invasive and would like to be 8 bytes instead?

That is not what I said. I did not say that "long double being 16 byte aligned is too invasive", I said that changing the alignment of long double would probably not be too invasive of a change...

so not quite sure how/why worsening dlmalloc alignment is then perceived ok

What do you mean by "worsening" the alignment? Is 16-byte alignment "worse" because it increases memory usage/fragmentation, or is it "better" because it actually satisfies the alignment requirements of every primitive type, as it's supposed to?

To be clear: I think ideally, long double and max_align_t should have been 8-byte aligned, and in that case, there would be no problem with a 8-byte aligned malloc. But as long as long double (and therefore max_align_t) remains 16-byte aligned, malloc must be as well.

[sbc100]

@juj what is your feeling about increasing malloc alignment to 16 (max_align_t)? Do you think it will have noticeable effects on real world code? Would you be ok with that change?