Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't use sbrk(0) to determine the initial heap size #377

Merged
merged 4 commits into from
Jan 9, 2023

Conversation

alexcrichton
Copy link
Collaborator

This commit changes the try_init_allocator function as part of dlmalloc to not use sbrk(0) to determine the initial heap size. The purpose of this function is to use the extra memory at the end of linear memory for the initial allocation heap before memory.grow is used to allocate more memory. To learn the extent of this region the code previously would use sbrk(0) to find the current size of linear memory. This does not work, however, when other systems have called memory.grow before this function is called. For example if another allocator is used or if another component of a wasm binary grows memory for its own purposes then that memory will be incorrectly claimed to be owned by dlmalloc.

Instead this commit rounds up the __heap_base address to the nearest page size, since that must be allocatable. Otherwise anything above this rounded address is assumed to be used by something else, even if it's addressable.

This commit changes the `try_init_allocator` function as part of
dlmalloc to not use `sbrk(0)` to determine the initial heap size. The
purpose of this function is to use the extra memory at the end of linear
memory for the initial allocation heap before `memory.grow` is used to
allocate more memory. To learn the extent of this region the code
previously would use `sbrk(0)` to find the current size of linear
memory. This does not work, however, when other systems have called
`memory.grow` before this function is called. For example if another
allocator is used or if another component of a wasm binary grows memory
for its own purposes then that memory will be incorrectly claimed to be
owned by dlmalloc.

Instead this commit rounds up the `__heap_base` address to the nearest
page size, since that must be allocatable. Otherwise anything above this
rounded address is assumed to be used by something else, even if it's
addressable.
@yamt
Copy link
Contributor

yamt commented Jan 7, 2023

it breaks the case where initial heap is more than 1 page, doesn't it?

@alexcrichton
Copy link
Collaborator Author

This doesn't break that use case, but it does make it less efficient. The default output of LLD doesn't ever have more than one page at the end of memory (or at least not that I'm aware of), and I'm not sure if such a setup is commonly used in the wild. If it is then I think it would perhaps make sense to add a new pseudo-symbol to LLD such as __heap_base along the lines of __heap_initial_end or something like that, and then that can be used instead of the round-up-to-page-size calculation done here.

@TerrorJack
Copy link
Contributor

Note there's __heap_end added re discussion in #338, so that can be used to avoid the roudup calculation here.

Also, re avoiding collision with userland sbrk calls, I still believe making try_init_allocator a ctor is a simpler solution.

@sbc100
Copy link
Member

sbc100 commented Jan 9, 2023

So is __heap_end the solution here? Maybe can use a weak reference to it and call fall to the current implementation when it is missing (i.e. on older llvm versions)?

@alexcrichton
Copy link
Collaborator Author

Yes I've updated this to prioritize using __heap_end through a weak symbol now.

Note @TerrorJack that using a ctor is also not going to work here since ctors are not run as part of the wasm start function (or at least not that I know of) which means that the initialization here would still run too late and embedders could, for example, grow memory before calling exports of the module.

@TerrorJack
Copy link
Contributor

ctors are not run as part of the wasm start function

IIUIC wasm-ld takes care of ctors at link time and puts them in the _initialize/_start function per wasi reactor/command spec?

@alexcrichton
Copy link
Collaborator Author

Yes that is distinct from the start function. When the wasm module is instantiated the _start or _initialize function isn't run, it's only when explicitly requested by an embedder that it's executed. In the use case that discovered this issue it was before the _start function was run that memory was grown, so a ctor would not solve this issue.

dlmalloc/src/malloc.c Outdated Show resolved Hide resolved
dlmalloc/src/malloc.c Outdated Show resolved Hide resolved
dlmalloc/src/malloc.c Outdated Show resolved Hide resolved
@TerrorJack
Copy link
Contributor

That's fair. Maybe a bit offtopic but I do wonder why the ctors aren't put into the module start function instead.

In the use case that discovered this issue it was before the _start function was run that memory was grown

Sounds like undefined behavior to call anything before _start/_initialize in the wasi land. But it's also reasonable to try working around this (perhaps common) user mistake, so in that case indeed ctor isn't a good solution.

@alexcrichton
Copy link
Collaborator Author

Ah excellent suggestions @sbc100, updated to include those.

Sounds like undefined behavior to call anything before _start/_initialize in the wasi land.

Personally I don't consider that a great stance for wasi-libc to take. That's a pretty onerous restriction to work around when relatively simple lazy initialization, such as what happens in dlmalloc right now, fixes the issue.

@sbc100
Copy link
Member

sbc100 commented Jan 9, 2023

That's fair. Maybe a bit offtopic but I do wonder why the ctors aren't put into the module start function instead.

There is a long discussion on this here: WebAssembly/design#1160

TLDR: Calling host functions normally requires exports to have been received by the host (e.g. the host can do much until is has a handle to the memory export). Exports are not received by the host until after the start function runs, therefore its not possible, in the general case, to call host functions during start function.

@sbc100
Copy link
Member

sbc100 commented Jan 9, 2023

That's fair. Maybe a bit offtopic but I do wonder why the ctors aren't put into the module start function instead.

There is a long discussion on this here: WebAssembly/design#1160

TLDR: Calling host functions normally requires exports to have been received by the host (e.g. the host can do much until is has a handle to the memory export). Exports are not received by the host until after the start function runs, therefore its not possible, in the general case, to call host functions during start function.

For what its worth the wasm-ld linker will use the start function for some things if it knows that code is only doing internal stuff (i.e. loading memory segments, applying relocations). its only if user code is involved that we must wait until after start has run.

@sunfishcode
Copy link
Member

The approach of using __heap_end as a weak symbol, and falling back to the page alignment logic looks good to me. __heap_end was added relatively recently so it isn't in LLVM 15, but when it is available, it's exactly what we need here.

@sunfishcode sunfishcode merged commit f2aac5f into WebAssembly:main Jan 9, 2023
sunfishcode added a commit to WebAssembly/wasi-sdk that referenced this pull request Jan 9, 2023
Update to wasi-libc a1c7c2c7a4b2813c6f67bd2ef6e0f430d31cebad
 - Don't use sbrk(0) to determine the initial heap size (WebAssembly/wasi-libc#377)
 - Fix more headers to avoid depending on `max_align_t` (WebAssembly/wasi-libc#375)
 - Use `ENOENT` rather than `ENOTCAPABLE` for missing preopens. (WebAssembly/wasi-libc#370)
 - Adjust Makefile for LLVM trunk (16) as of 2022-11-08 (WebAssembly/wasi-libc#344)
@alexcrichton alexcrichton deleted the dlmalloc-less-sbrk branch January 9, 2023 17:02
@jedisct1
Copy link
Member

jedisct1 commented Jan 9, 2023

Shouldn't the same thing be done for emmalloc?

sunfishcode added a commit that referenced this pull request Jan 9, 2023
Avoid using sbrk(0) in emmalloc too.
@sunfishcode
Copy link
Member

Shouldn't the same thing be done for emmalloc?

Good point; I've submitted #378 for this.

sunfishcode added a commit to WebAssembly/wasi-sdk that referenced this pull request Jan 9, 2023
Update to wasi-libc a1c7c2c7a4b2813c6f67bd2ef6e0f430d31cebad
 - Don't use sbrk(0) to determine the initial heap size (WebAssembly/wasi-libc#377)
 - Fix more headers to avoid depending on `max_align_t` (WebAssembly/wasi-libc#375)
 - Use `ENOENT` rather than `ENOTCAPABLE` for missing preopens. (WebAssembly/wasi-libc#370)
 - Adjust Makefile for LLVM trunk (16) as of 2022-11-08 (WebAssembly/wasi-libc#344)
@yamt
Copy link
Contributor

yamt commented Jan 10, 2023

This doesn't break that use case, but it does make it less efficient.

i feel you have a different definition of break from mine.
dropping a few pages is critical for some of my environment.

The default output of LLD doesn't ever have more than one page at the end of memory (or at least not that I'm aware of), and I'm not sure if such a setup is commonly used in the wild. If it is then I think it would perhaps make sense to add a new pseudo-symbol to LLD such as __heap_base along the lines of __heap_initial_end or something like that, and then that can be used instead of the round-up-to-page-size calculation done here.

while it might not be too common setup, i occasionally use such a setup.
also, i suspect the use of memory.grow out of malloc isn't common either.
maybe you can explain your use case a bit more?

@sunfishcode
Copy link
Member

LLVM has added a __heap_end symbol which is the eventual path forward here. It unfortunately didn't make it into LLVM 15, but it will be in future versions. The patch is https://reviews.llvm.org/D136110.

Does wasm-ld have some option which enables extra patches to be allocated after __heap_base? If not, do you use a patched wasm-ld? If so, if you also add the patch from https://reviews.llvm.org/D136110, then this PR will work for you, because it'll use __heap_end.

@alexcrichton
Copy link
Collaborator Author

I'll reiterate again that these are just my own personal thoughts, and I'm not really a core maintainer here so my thoughts I don't think should really hold all that much weight for maintaining wasi-libc. I don't think that the old behavior is correct, to me it's broken. To me that's enough reason to preserve the old behavior because I don't think that a bug should be preserved just because it's been there for awhile.

Looking at the history this behavior was specifically added in #114 so while it's pretty old at this point it may not be so long as "wasi-libc never allowed this". Regardless though, in my opinion, how old it is I don't think should have a bearing on fixing the underlying bug.

I again don't want to appear like I'm diminishing or brushing away use cases here. My point is that the use case of someone else calling memory.grow before malloc is fundamentally broken today, and has been since #114 was merged. Fixing that to me, personally at least, takes a higher priority than preserving an alternative use case in the interm while LLVM is updated to specify __heap_end everywhere.

Again though I don't maintain wasi-libc, I just contribute on occasion. If this patch is reverted I'll find other ways to solve my problems, so I don't think it necessarily needs my own personal approval.

@yamt
Copy link
Contributor

yamt commented Jan 12, 2023

my understanding of the situation is same.

my preference is the opposite though.
in this case, preserving a working use case outweighs fixing a use case which has been broken for long, especially a better fix (__heap_end) is on the horizon.

@sbc100
Copy link
Member

sbc100 commented Jan 12, 2023

My point is that the use case of someone else calling memory.grow before malloc is fundamentally broken today,

This is somewhat of a new use case to me. I had always imagined that __heap_base onwards was considered the malloc heap. Do you imagine that it should be OK to grab chunks of memory in this way after malloc has been called? Or is it only OK to do this before malloc is called?

jedisct1 added a commit to jedisct1/zig that referenced this pull request Jan 12, 2023
The symbol was introduced in LLD 15.0.7, as a way to know how
much memory can be allocated:

llvm/llvm-project@1095870
WebAssembly/wasi-libc#377
jedisct1 added a commit to ziglang/zig that referenced this pull request Jan 12, 2023
The symbol was introduced in LLD 15.0.7, as a way to know how
much memory can be allocated:

llvm/llvm-project@1095870
WebAssembly/wasi-libc#377
@sunfishcode
Copy link
Member

LLVM 15.0.7 is now released, with the fix.

@alexcrichton
Copy link
Collaborator Author

I believe the current status of wasi-libc is that:

  • If memory.grow is called before the first malloc, then wasi-libc claims that memory for itself
  • If memory.grow is called after the first malloc, then wasi-libc avoids using that memory

I personally think wasi-libc's malloc should not assume aggressive ownership of the entire address space. There might be multiple allocators in play or other custom embedding logic which uses memory and allocates via memory.grow. For example my use case is component-model related where state for a separate module than the "main module" using wasi-libc is allocated via memory.grow because the malloc function is not guaranteed to be exported.

@sbc100
Copy link
Member

sbc100 commented Jan 12, 2023

I believe the current status of wasi-libc is that:

  • If memory.grow is called before the first malloc, then wasi-libc claims that memory for itself

I must be misunderstanding, I though that point of this PR was to avoid exactly this case. After this PR doesn't wasi-libc avoid using this region?

  • If memory.grow is called after the first malloc, then wasi-libc avoids using that memory

Are you sure about this? Looking at dlmalloc.c it seems that we currently build with MORECORE_CONTIGUOUS defined (this the default and recommended mode for dlmalloc). IIRC this means that there is an assumption the sbkr region returns a contiguously increasing range of memory. Any regions claimed by memory.grow after initialization will be fair game for dlmalloc to use I believe.

Unless I'm misunderstanding what MORECORE_CONTIGUOUS means?

@sbc100
Copy link
Member

sbc100 commented Jan 12, 2023

Oh, maybe MORECORE_CONTIGUOUS does actually handle discontinuities:

MORECORE_CONTIGUOUS       default: 1 (true) if HAVE_MORECORE                     
  If true, take advantage of fact that consecutive calls to MORECORE             
  with positive arguments always return contiguous increasing                    
  addresses.  This is true of unix sbrk. It does not hurt too much to            
  set it true anyway, since malloc copes with non-contiguities.                  
  Setting it false when definitely non-contiguous saves time                        
  and possibly wasted space it would take to discover this though.  

@alexcrichton
Copy link
Collaborator Author

Sorry, I should clarify I'm describing the pre-this-PR behavior since that's also what I think you're talking about.

As for libc's behavior, I determined this empirically:

use std::alloc;
use std::arch::wasm32::memory_grow;

const PAGESIZE: usize = 64 * 1024;

#[no_mangle]
extern "C" fn entry() {
    // 0x110000 - before first malloc
    println!("{:#x}", memory_grow::<0>(1) * PAGESIZE);

    // 0x102600 - note that this extends into the grown region
    println!("{:p}", allocate(PAGESIZE));

    // 0x120000 - grow after first malloc
    println!("{:#x}", memory_grow::<0>(1) * PAGESIZE);

    // 0x130010 - note that this does not conflict with second `memory.grow`
    println!("{:p}", allocate(PAGESIZE));
}

fn allocate(amt: usize) -> *mut u8 {
    unsafe { alloc::alloc(alloc::Layout::from_size_align(amt, 8).unwrap()) }
}

This prints:

$ rustc foo.rs --crate-type cdylib --target wasm32-wasi && wasmtime foo.wasm --invoke entry
0x110000
0x102600
0x120000
0x130010

Here the return value of malloc overlaps with the first memory.grow, but is disjoint from the second memory.grow.

I also saw MORECORE_CONTIGUOUS and I don't know why it doesn't have the expected effect. I did not dig deeply into dlmalloc's implementation.

@yamt
Copy link
Contributor

yamt commented Jan 13, 2023

I believe the current status of wasi-libc is that:

* If `memory.grow` is called before the first `malloc`, then wasi-libc claims that memory for itself

* If `memory.grow` is called after the first `malloc`, then wasi-libc avoids using that memory

I personally think wasi-libc's malloc should not assume aggressive ownership of the entire address space. There might be multiple allocators in play or other custom embedding logic which uses memory and allocates via memory.grow. For example my use case is component-model related where state for a separate module than the "main module" using wasi-libc is allocated via memory.grow because the malloc function is not guaranteed to be exported.

we are all fine with __heap_end and the remaining question is what to do without __heap_end, right?
i prefer to keep the old behavior for better compatibility.
can't you wait for __heap_end for some reasons? why not?

sunfishcode added a commit to WebAssembly/wasi-sdk that referenced this pull request Jan 13, 2023
* Update llvm-project to the latest release/15.x

This pulls in the `__heap_end` symbol, which fixes the issue discussed
in WebAssembly/wasi-libc#377.

* Update to the official 15.0.7 release.
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Jan 13, 2023
This commit updates the wasi-libc revision used to build with the
wasm32-wasi target. This notably pulls in WebAssembly/wasi-libc#377
which is needed to fix a use case I've been working on recently. This
should be a relatively small update hopefully and is not expected to
have any user impact.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 14, 2023
…, r=cuviper

Update the wasi-libc used for the wasm32-wasi target

This commit updates the wasi-libc revision used to build with the wasm32-wasi target. This notably pulls in WebAssembly/wasi-libc#377 which is needed to fix a use case I've been working on recently. This should be a relatively small update hopefully and is not expected to have any user impact.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 14, 2023
…, r=cuviper

Update the wasi-libc used for the wasm32-wasi target

This commit updates the wasi-libc revision used to build with the wasm32-wasi target. This notably pulls in WebAssembly/wasi-libc#377 which is needed to fix a use case I've been working on recently. This should be a relatively small update hopefully and is not expected to have any user impact.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 14, 2023
…, r=cuviper

Update the wasi-libc used for the wasm32-wasi target

This commit updates the wasi-libc revision used to build with the wasm32-wasi target. This notably pulls in WebAssembly/wasi-libc#377 which is needed to fix a use case I've been working on recently. This should be a relatively small update hopefully and is not expected to have any user impact.
matu3ba pushed a commit to matu3ba/zig that referenced this pull request Jan 15, 2023
The symbol was introduced in LLD 15.0.7, as a way to know how
much memory can be allocated:

llvm/llvm-project@1095870
WebAssembly/wasi-libc#377
@yamt
Copy link
Contributor

yamt commented Jan 19, 2023

i submitted a partial revert #386

yamt added a commit to yamt/wasi-libc that referenced this pull request Feb 17, 2023
This commit effectively drops the support of older wasm-ld. (LLVM <15)

We have two relevant use cases:

* `memory.grow` use outside of malloc
  (eg. used by polyfill preview1 binaries)

* `--init-memory` to somehow preallocate heap
  (eg. avoid dynamic allocations, especially on small environments)

While WebAssembly#377
fixed the former, it broke the latter if you are using
an older LLVM, which doesn't provide the `__heap_end` symbol,
to link your module.

As we couldn't come up with a solution which satisfies all parties,
this commit simply makes it require new enough LLVM which provides
`__heap_end`. After all, a link-time failure is more friendly to users
than failing later in a subtle way.
yamt added a commit to yamt/wasi-libc that referenced this pull request Feb 17, 2023
This commit effectively drops the support of older wasm-ld. (LLVM <15.0.7)

We have two relevant use cases:

* `memory.grow` use outside of malloc
  (eg. used by polyfill preview1 binaries)

* `--init-memory` to somehow preallocate heap
  (eg. avoid dynamic allocations, especially on small environments)

While WebAssembly#377
fixed the former, it broke the latter if you are using
an older LLVM, which doesn't provide the `__heap_end` symbol,
to link your module.

As we couldn't come up with a solution which satisfies all parties,
this commit simply makes it require new enough LLVM which provides
`__heap_end`. After all, a link-time failure is more friendly to users
than failing later in a subtle way.
john-sharratt pushed a commit to john-sharratt/wasix-libc that referenced this pull request Mar 6, 2023
* Don't use sbrk(0) to determine the initial heap size

This commit changes the `try_init_allocator` function as part of
dlmalloc to not use `sbrk(0)` to determine the initial heap size. The
purpose of this function is to use the extra memory at the end of linear
memory for the initial allocation heap before `memory.grow` is used to
allocate more memory. To learn the extent of this region the code
previously would use `sbrk(0)` to find the current size of linear
memory. This does not work, however, when other systems have called
`memory.grow` before this function is called. For example if another
allocator is used or if another component of a wasm binary grows memory
for its own purposes then that memory will be incorrectly claimed to be
owned by dlmalloc.

Instead this commit rounds up the `__heap_base` address to the nearest
page size, since that must be allocatable. Otherwise anything above this
rounded address is assumed to be used by something else, even if it's
addressable.

* Use `__heap_end` if defined

* Move mstate initialization earlier
yamt added a commit to yamt/wasi-libc that referenced this pull request Mar 22, 2023
This commit effectively drops the support of older wasm-ld. (LLVM <15.0.7)

We have two relevant use cases:

* `memory.grow` use outside of malloc
  (eg. used by polyfill preview1 binaries)

* `--init-memory` to somehow preallocate heap
  (eg. avoid dynamic allocations, especially on small environments)

While WebAssembly#377
fixed the former, it broke the latter if you are using
an older LLVM, which doesn't provide the `__heap_end` symbol,
to link your module.

As we couldn't come up with a solution which satisfies all parties,
this commit simply makes it require new enough LLVM which provides
`__heap_end`. After all, a link-time failure is more friendly to users
than failing later in a subtle way.
abrown pushed a commit that referenced this pull request Jul 11, 2023
This commit effectively drops the support of older wasm-ld. (LLVM <15.0.7).

We have two relevant use cases:

* `memory.grow` use outside of malloc
  (eg. used by polyfill preview1 binaries)

* `--init-memory` to somehow preallocate heap
  (eg. avoid dynamic allocations, especially on small environments)

While #377
fixed the former, it broke the latter if you are using
an older LLVM, which doesn't provide the `__heap_end` symbol,
to link your module.

As we couldn't come up with a solution which satisfies all parties,
this commit simply makes it require new enough LLVM which provides
`__heap_end`. After all, a link-time failure is more friendly to users
than failing later in a subtle way.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants