This is an archive of the discontinued LLVM Phabricator instance.

[lld][WebAssembly] Align __heap_base
ClosedPublic

Authored by aykevl on Jul 21 2021, 2:54 PM.

Details

Summary

__heap_base was not aligned. In practice, it will often be aligned simply because it follows the stack, but when the stack is placed at the beginning (with the --stack-first option), the __heap_base might be unaligned. It could even be byte-aligned.

At least wasi-libc appears to expect that __heap_base is aligned: https://github.com/WebAssembly/wasi-libc/blob/659ff414560721b1660a19685110e484a081c3d4/dlmalloc/src/malloc.c#L5224

While WebAssembly itself does not appear to require any alignment for memory accesses, it is sometimes required when sharing a pointer externally. For example, WASI might expect alignment up to 8: https://github.com/WebAssembly/WASI/blob/main/phases/snapshot/docs.md#-timestamp-u64

This issue got introduced with the addition of the --stack-first flag: https://reviews.llvm.org/D46141
I suspect the lack of alignment wasn't intentional here.

Diff Detail

Event Timeline

aykevl created this revision.Jul 21 2021, 2:54 PM
aykevl requested review of this revision.Jul 21 2021, 2:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2021, 2:54 PM
aykevl edited the summary of this revision. (Show Details)Jul 21 2021, 2:55 PM
sbc100 accepted this revision.Jul 21 2021, 3:10 PM

Nice! Thanks.

lld/wasm/Writer.cpp
46

I think it would make sense to use 16 here since the default ABI has a max_align_t of 16.

Maybe just call this heapAlignment?

This revision is now accepted and ready to land.Jul 21 2021, 3:10 PM
aykevl updated this revision to Diff 360628.Jul 21 2021, 3:49 PM
aykevl edited the summary of this revision. (Show Details)
  • change alignment to 16
  • change heapBaseAlignment to heapAlignment
aykevl edited the summary of this revision. (Show Details)Jul 21 2021, 3:49 PM
aykevl edited the summary of this revision. (Show Details)

Thank you for the quick review! I'll commit this soon-ish unless someone has comments on the patch.

lld/wasm/Writer.cpp
46

Sounds good! I've updated the patch according to your suggestions.

Looks good to me too!

This revision was automatically updated to reflect the committed changes.