Partially revert "Don't use sbrk(0) to determine the initial heap size" #386

yamt · 2023-01-19T03:58:13Z

While the problem it addressed (memory.grow use outside of malloc) is real, it has been broken for long in this implementation. IMO, the fix doesn't warrant breaking other working use cases, (Pre-populating heap with --init-memory) especially when a better solution (__heap_end) is on the horizon.

sbc100

I tend to agree with this approach ..

sbc100 · 2023-01-19T04:17:19Z

dlmalloc/src/malloc.c

@@ -5228,22 +5228,10 @@ static void try_init_allocator(void) {
  char *base = &__heap_base;
  // Try to use the linker pseudo-symbol `__heap_end` for the initial size of
  // the heap, but if that's not defined due to LLVM being too old perhaps then


can you remove perhaps here.

How about ".. but if that's not defined (due to LLVM being too old) ..."

sbc100 · 2023-01-19T04:18:23Z

dlmalloc/src/malloc.c

-  // correct if the we're the first to try to grow the heap. If the heap has
-  // grown elsewhere, such as a different allocator in place, then this would
-  // incorrectly claim such memroy as our own.
+  // fall back to `CALL_MORECORE(0)`.


"fall back to assuming the heap extends to end of linear memory (CALL_MORECORE(0))"

While the problem it addressed (`memory.grow` use outside of malloc) is real, it has been broken for long in this implementation. IMO, the fix doesn't warrant breaking other working use cases, (Pre-populating heap with `--init-memory`) especially when a better solution (`__heap_end`) is on the horizon.

sunfishcode · 2023-01-19T23:47:02Z

From the perspective of wasi-sdk releases, this isn't needed. Wasi-sdk 18 never had any artifacts uploaded, and I've now downgraded it to a pre-release to discourage people from using it. So the main relevant releases are wasi-sdk 17, which has the old behavior, and wasi-sdk 19, which has the good __heap_end fix.

We do have users who download just wasi-sysroot though, but I wonder if it would be enough for such users to package this PR as a special release.

Part of my reason for hesitating here is that the old behavior of malloc claiming all the memory up to memory.size(0) makes it harder to polyfill preview1 binaries, which is an important part of how WASI as a whole will move beyond preview1. I'm aware that in mature projects, this would be viewed as a new feature and thus lexically inferior to fixing a regression. But WASI is in its infancy. It's desirable to find a balance that allows us to support existing users while still being able to make progress.

@yamt Would it work for your use case if we merged this PR onto a dedicated branch, rather than main, and made a special release build for it? We could distribute the build artifacts here just like normal releases, but we'd have special release notes explaining the --initial-memory situation and the consequences for polyfilling. We could also add a mention of this branch to the wasi-sdk-19 release notes.

sbc100 · 2023-01-19T23:56:08Z

From the perspective of wasi-sdk releases, this isn't needed. Wasi-sdk 18 never had any artifacts uploaded, and I've now downgraded it to a pre-release to discourage people from using it. So the main relevant releases are wasi-sdk 17, which has the old behavior, and wasi-sdk 19, which has the good __heap_end fix.

Sorry, I'm a little confused, which is the old behaviour you are talking about in wasi-sdk 17? Is the behaviour where malloc grabs all the memory up to memory.size(0)? If so then it will not be possible to polyfill binaries built with wasi-sdk 17?

So if once wants to build a binary that supports the polyfill one would need to download wasi-sdk 19? (Which contains the __heap_end fix? )

sbc100 · 2023-01-20T00:00:08Z

Another question: can you be clear about which users you imagine might be broken by landing this change?

Are you worried about users using tip-of-tree wasi-libc but wanted to use it with and older versions of wam-ld? That seems conceivable but fairly unlikely. Or is there some other group of users that I'm not thinking of?

sunfishcode · 2023-01-20T00:12:40Z

From the perspective of wasi-sdk releases, this isn't needed. Wasi-sdk 18 never had any artifacts uploaded, and I've now downgraded it to a pre-release to discourage people from using it. So the main relevant releases are wasi-sdk 17, which has the old behavior, and wasi-sdk 19, which has the good __heap_end fix.

Sorry, I'm a little confused, which is the old behaviour you are talking about in wasi-sdk 17? Is the behaviour where malloc grabs all the memory up to memory.size(0)? If so then it will not be possible to polyfill binaries built with wasi-sdk 17?

Yes. wasi-sdk-17 will claim all memory up to memory.size(0) at the time it initializes itself. That makes it impossible for a polyfill to allocate any memory before the first malloc call. It's not impossible to polyfill such binaries, but it does require a polyfill to work differently, which is a lot more work.

So if once wants to build a binary that supports the polyfill one would need to download wasi-sdk 19? (Which contains the __heap_end fix? )

That is the current situation. It's not ideal. We're all doing the best we can within practical constraints.

Another question: can you be clear about which users you imagine might be broken by landing this change?

Are you worried about users using tip-of-tree wasi-libc but wanted to use it with and older versions of wam-ld? That seems conceivable but fairly unlikely. Or is there some other group of users that I'm not thinking of?

I don't have any numbers, but anecdotally, we do occasionally get people asking how to download the wasi-sysroot separately from everything else, and bug reports from people using third-party clang builds (such as the clang provided by homebrew).

sbc100 · 2023-01-20T01:08:51Z

I don't have any numbers, but anecdotally, we do occasionally get people asking how to download the wasi-sysroot separately from everything else, and bug reports from people using third-party clang builds (such as the clang provided by homebrew).

Thanks, that does help clear things up for me. Now I understand that argument for not landing this.

yamt · 2023-01-20T01:23:44Z

From the perspective of wasi-sdk releases, this isn't needed. Wasi-sdk 18 never had any artifacts uploaded, and I've now downgraded it to a pre-release to discourage people from using it. So the main relevant releases are wasi-sdk 17, which has the old behavior, and wasi-sdk 19, which has the good __heap_end fix.

We do have users who download just wasi-sysroot though, but I wonder if it would be enough for such users to package this PR as a special release.

Part of my reason for hesitating here is that the old behavior of malloc claiming all the memory up to memory.size(0) makes it harder to polyfill preview1 binaries, which is an important part of how WASI as a whole will move beyond preview1. I'm aware that in mature projects, this would be viewed as a new feature and thus lexically inferior to fixing a regression. But WASI is in its infancy. It's desirable to find a balance that allows us to support existing users while still being able to make progress.

sorry, i don't understand your polyfill reasoning.

with or without this revert,

it's difficult to polyfill binaries built with wasi-sdk-17
you can polyfill binaries built with wasi-sdk-19 and later because __heap_end is available

am i missing something?

@yamt Would it work for your use case if we merged this PR onto a dedicated branch, rather than main, and made a special release build for it? We could distribute the build artifacts here just like normal releases, but we'd have special release notes explaining the --initial-memory situation and the consequences for polyfilling. We could also add a mention of this branch to the wasi-sdk-19 release notes.

do you mean to say something like "if you want to use --init-memory, use this version of sysroot. but such binaries won't work with polyfill"?
do you mean to release the extra sysroot for every future releases? at least until the new wasm-ld gets ubiquitous?

after thinking about this topic a bit more, i'm now feeling it might be simpler to just require __heap_end. (use a non-weak reference)
i consider that a link-time breakage is more helpful for users than saying "if you don't understand this complex situation, it might bite you later".
how do you think?

sbc100 · 2023-01-20T02:03:35Z

after thinking about this topic a bit more, i'm now feeling it might be simpler to just require __heap_end. (use a non-weak reference)

I think that would be great solution, but it really depends how many of our users out there want to use their own (older) version llvm rather than the latest.

yamt · 2023-01-20T03:31:03Z

after thinking about this topic a bit more, i'm now feeling it might be simpler to just require __heap_end. (use a non-weak reference)

I think that would be great solution, but it really depends how many of our users out there want to use their own (older) version llvm rather than the latest.

right. my assumption here is that users who can't use newer llvm is rare.

yamt · 2023-02-13T04:13:06Z

can we make a decision?
options are:

a. do nothing
b. apply this PR
c. require __heap_end (use a normal reference, not a weak reference)

my preference is c. >= b. > a.

i don't like the current state because it will leave us something difficult to trouble shoot.
(either --init-memory or preview1 polyfill)
after all, users will not likely remember which version of llvm they used to build their modules.

sunfishcode · 2023-02-15T15:23:58Z

I'd like to avoid applying this PR, as this PR would make it even more complex for users to determine whether they have a toolchain that has the bug fix.

Could we make wasi-libc require __heap_end? That would make it depend on a newer LLVM. So far we've tried to support some older LLVM versions, but perhaps this issue justifies dropping support for them?

yamt · 2023-02-17T04:21:20Z

I'd like to avoid applying this PR, as this PR would make it even more complex for users to determine whether they have a toolchain that has the bug fix.

Could we make wasi-libc require __heap_end? That would make it depend on a newer LLVM. So far we've tried to support some older LLVM versions, but perhaps this issue justifies dropping support for them?

ok. see #394

abrown · 2023-07-11T22:34:18Z

I think this is superseded by #394; now we always depend on the __heap_end value instead of sbrk(0).

yamt mentioned this pull request Jan 19, 2023

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

Merged

sbc100 reviewed Jan 19, 2023

View reviewed changes

yamt force-pushed the revert branch from b906aba to 2aa8a0d Compare January 19, 2023 04:25

abrown closed this Jul 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partially revert "Don't use sbrk(0) to determine the initial heap size" #386

Partially revert "Don't use sbrk(0) to determine the initial heap size" #386

yamt commented Jan 19, 2023

sbc100 left a comment

sbc100 Jan 19, 2023

yamt Jan 19, 2023

sbc100 Jan 19, 2023

yamt Jan 19, 2023

sunfishcode commented Jan 19, 2023

sbc100 commented Jan 19, 2023 •

edited

Loading

sbc100 commented Jan 20, 2023

sunfishcode commented Jan 20, 2023

sbc100 commented Jan 20, 2023

yamt commented Jan 20, 2023

sbc100 commented Jan 20, 2023

yamt commented Jan 20, 2023

yamt commented Feb 13, 2023

sunfishcode commented Feb 15, 2023

yamt commented Feb 17, 2023

abrown commented Jul 11, 2023

Partially revert "Don't use sbrk(0) to determine the initial heap size" #386

Partially revert "Don't use sbrk(0) to determine the initial heap size" #386

Conversation

yamt commented Jan 19, 2023

sbc100 left a comment

Choose a reason for hiding this comment

sbc100 Jan 19, 2023

Choose a reason for hiding this comment

yamt Jan 19, 2023

Choose a reason for hiding this comment

sbc100 Jan 19, 2023

Choose a reason for hiding this comment

yamt Jan 19, 2023

Choose a reason for hiding this comment

sunfishcode commented Jan 19, 2023

sbc100 commented Jan 19, 2023 • edited Loading

sbc100 commented Jan 20, 2023

sunfishcode commented Jan 20, 2023

sbc100 commented Jan 20, 2023

yamt commented Jan 20, 2023

sbc100 commented Jan 20, 2023

yamt commented Jan 20, 2023

yamt commented Feb 13, 2023

sunfishcode commented Feb 15, 2023

yamt commented Feb 17, 2023

abrown commented Jul 11, 2023

sbc100 commented Jan 19, 2023 •

edited

Loading