[PATCH] D42030: [WebAssembly] Define __heap_base global

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: wasm/Writer.cpp:38
 static constexpr int kStackAlignment = 16;
+static constexpr int kHeapAlignment = 4096;
 
----------------
ncw wrote:
> sbc100 wrote:
> > dschuff wrote:
> > > Why 4096? Since wasm pages are 16k, is there some particular reason for it to be this as opposed to something else?
> > No really any good reason...  should we just make it 16-byte aligned like the stack?  In which case there is not need to do any more alignedment since the heap will follow the stack region which is already 16-byte aligned.
> > 
> > Incidentally our musl limits.h currently defines PAGE_SIZE to 4k.. should we change it to 16k?
> (I think Wasm pages are actually 64KiB: https://webassembly.github.io/spec/core/exec/runtime.html#page-size)
> 
> I //wouldn't// change the Musl PAGE_SIZE to match. (I looked into this when doing my Musl port, and Rich Felker added this as a review comment.) PAGE_SIZE doesn't have to match anything the CPU does, what it really has to match is what mmap does. So whatever alignment your mmap (and hence malloc) perform should be exposed via PAGE_SIZE. In my Musl, I chose 32KiB for the page size, and mmap works in those units, so that malloc rounds up "large" allocations (>100KiB) to the nearest libc page. It doesn't matter that grow_memory/current_memory Wasm instructions work in 64KiB units, the mmap implementation does the appropriate translation.
I discusses with with @dschuff yesterday.   We decide tt makes sense to page size to be consistent with the level of granularity of the underlying memory system.   There is no reason for mmap or brk to operate on anything smaller than 64k chunks since you we can't allocate in smaller chunks.  i.e.  it makes sense to have the page size match.   Perhaps there is a reason I'm missing but I can't see any benefit in having two different concepts of page size.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D42030