dlmalloc: require __heap_end #394

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

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.

dlmalloc/src/malloc.c

sbc100 · 2023-02-17T04:54:49Z

.github/workflows/main.yml

@@ -9,7 +9,7 @@ jobs:
      matrix:
        os: [ubuntu-latest, macos-latest, windows-latest]
        # oldest and newest supported LLVM version
-        clang_version: [10.0.0, 14.0.0]
+        clang_version: [10.0.0, 15.0.7]


Wouldn't we want to update the min version here? I think the idea is to test with the oldest supported version and also with a current/newer version.

even before this PR, we were not testing app link for 10.0.0.

yamt · 2023-02-17T05:04:34Z

i couldn't find any llvm >=15 releases which have assets for both of x86-64 ubuntu and x86-64 darwin.
what to do?

sbc100 · 2023-02-17T05:14:17Z

i couldn't find any llvm >=15 releases which have assets for both of x86-64 ubuntu and x86-64 darwin. what to do?

15.0.0 looks like it has those binaries: https://github.com/llvm/llvm-project/releases/tag/llvmorg-15.0.0

And 15.0.2: https://github.com/llvm/llvm-project/releases/tag/llvmorg-15.0.2

Strange that some of the others don't those. Might be worth opening an issue on the llvm tracker?

yamt · 2023-02-17T05:21:41Z

i couldn't find any llvm >=15 releases which have assets for both of x86-64 ubuntu and x86-64 darwin. what to do?

15.0.0 looks like it has those binaries: https://github.com/llvm/llvm-project/releases/tag/llvmorg-15.0.0

And 15.0.2: https://github.com/llvm/llvm-project/releases/tag/llvmorg-15.0.2

which assets are you talking about? can you give me the exact urls of assets?
i only see powerpc for ubuntu.

Strange that some of the others don't those. Might be worth opening an issue on the llvm tracker?

while i'm not familiar with llvm release process, i guess it's like volunteers building and uploading each binaries.
eg. https://discourse.llvm.org/t/llvm-15-0-5-has-been-tagged/66615

sbc100 · 2023-02-17T05:24:15Z

i couldn't find any llvm >=15 releases which have assets for both of x86-64 ubuntu and x86-64 darwin. what to do?

15.0.0 looks like it has those binaries: https://github.com/llvm/llvm-project/releases/tag/llvmorg-15.0.0

https://github.com/llvm/llvm-project/releases/download/llvmorg-15.0.0/clang+llvm-15.0.0-x86_64-apple-darwin.tar.xz
https://github.com/llvm/llvm-project/releases/download/llvmorg-15.0.0/clang+llvm-15.0.0-x86_64-linux-gnu-rhel-8.4.tar.xz

yamt · 2023-02-17T06:16:01Z

i couldn't find any llvm >=15 releases which have assets for both of x86-64 ubuntu and x86-64 darwin. what to do?

15.0.0 looks like it has those binaries: https://github.com/llvm/llvm-project/releases/tag/llvmorg-15.0.0

https://github.com/llvm/llvm-project/releases/download/llvmorg-15.0.0/clang+llvm-15.0.0-x86_64-apple-darwin.tar.xz https://github.com/llvm/llvm-project/releases/download/llvmorg-15.0.0/clang+llvm-15.0.0-x86_64-linux-gnu-rhel-8.4.tar.xz

the rhel assert worked on ubuntu.
but it reminded me we actually need >=15.0.7.

yamt · 2023-02-17T06:27:51Z

a. wait for llvm 16 and hope it will have necessary assets
b. use clang from wasi-sdk. feeling a bit chicken-and-egg
c. bulid clang on our ci

TerrorJack · 2023-02-17T09:49:37Z

d. use https://apt.llvm.org/ on linux

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.

cf. https://reviews.llvm.org/D125860

yamt · 2023-03-22T02:13:37Z

as a compromise, i made the ci to use different versions of llvm among platforms.

yamt · 2023-04-14T06:49:49Z

i still want to merge this. any concerns?

sbc100 · 2023-04-14T07:00:52Z

dlmalloc/src/malloc.c

+    //
+    // Note: This is a linker bug: https://github.com/llvm/llvm-project/issues/60829
+    return;
+  }


I think we should just remove this block and assume end is non-NULL.

llvm/llvm-project#60829 has been closed (and its really inconceivable to me that somebody would provide a weak definition of this (internal) symbol). If you want to keep some kind of check then perhaps just assert(end); with note referring to the bug.

its really inconceivable to me that somebody would provide a weak definition of this (internal) symbol

i actually have seen such a code. dump these values for diagnostic purposes.

If you want to keep some kind of check then perhaps just assert(end); with note referring to the bug.

it's rare for users to use wasi-libc w/o NDEBUG.

its really inconceivable to me that somebody would provide a weak definition of this (internal) symbol

i actually have seen such a code. dump these values for diagnostic purposes.

But I don't think we want to support that configuration, right? Why would somebody weakly define a linker-internal symbol? Can you point to the place where you have seen this? Maybe just if (!end) abort() in that case, with a TODO to remove once we can depend on the llvm that contains that fix?

its really inconceivable to me that somebody would provide a weak definition of this (internal) symbol

i actually have seen such a code. dump these values for diagnostic purposes.

But I don't think we want to support that configuration, right? Why would somebody weakly define a linker-internal symbol?

diagnostic purposes.
it just dumps something potentially useful for later trouble shooting.

Can you point to the place where you have seen this?

not open source.
but it's something similar to https://github.com/yamt/garbage/blob/03f9a1a1ee142194905818380f7f0e34287f9c64/wasm/hello/hello.c#L48

Maybe just if (!end) abort() in that case, with a TODO to remove once we can depend on the llvm that contains that fix?

i feel it's too early to assume https://reviews.llvm.org/D144747
given that even the latest wasi-sdk doesn't have the fix yet.

sbc100 · 2023-04-14T07:52:41Z

dlmalloc/src/malloc.c

-  if (end == NULL)
-    end = (char*) page_align((size_t) base);
+  if (end == NULL) {
+    // This can happen when 1. you are using an old wasm-ld which doesn't


This (1) case is not true anymore I think. Older versions of llvm will simply cause link failure, not result in a zero-addressed version of __heap_end.

i know you fixed the linker bug. (thank you)
but how does it fix old llvm?

I don't think we need to worry that much about it, since it will only effect people that are both (a) using the old linker and (b) are weakly referencing __heap_end elsewhere in their program. I think this is highly unlikely isn't it?

Maybe at least leave a TODO to completely remove this check once we drop support for older wasm-ld?

Just to be clear, is https://reviews.llvm.org/D144747 actually required in order to guarantee that this symbol will be non-NULL? Or is it simply enough to have 15.0.7? IIUC if you have 15.0.7 then this can never be null, so this check is only in support of < 15.0.7?

sbc100 · 2023-04-17T15:41:28Z

dlmalloc/src/malloc.c

+  if (end == NULL) {
+    // This can happen when 1. you are using an old wasm-ld which doesn't
+    // provide `__heap_end` and 2. something (other libraries or maybe
+    // your app?) provide a weak reference to `__heap_end`.


"provide a weak reference to" -> "includes a weak reference to".

Can you add:

"and (3) the weak reference is found by the linker before this strong reference."

yamt · 2023-04-19T02:55:06Z

@sbc100 after thinking a bit, i changed my mind and turned it to a trap. the whole purpose of this PR is to force users to use newer linker. in that sense, it's better to make it fail loudly even if it's on runtime.

sbc100 · 2023-06-28T16:52:39Z

Is this change good to land now?

yamt · 2023-06-29T05:22:31Z

Is this change good to land now?

yes.
i'm not aware of any open issues.

yamt mentioned this pull request Feb 17, 2023

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

Closed

sbc100 reviewed Feb 17, 2023

View reviewed changes

yamt force-pushed the require-heap-end branch 2 times, most recently from b24446a to c385fdb Compare February 17, 2023 05:02

yamt marked this pull request as draft February 17, 2023 05:05

yamt force-pushed the require-heap-end branch from b509a8b to ba4a797 Compare February 17, 2023 06:21

yamt added 3 commits March 22, 2023 09:03

dlmalloc: add a comment

f5f2d77

CI: Bump llvm version

1cc8c8f

yamt force-pushed the require-heap-end branch from ba4a797 to 1cc8c8f Compare March 22, 2023 00:30

Deal with the resource dir change

adc74d7

cf. https://reviews.llvm.org/D125860

yamt marked this pull request as ready for review March 22, 2023 02:11

MoritzS mentioned this pull request Apr 2, 2023

Fixed malloc with concurrent executions of memory.grow #404

Open

sbc100 reviewed Apr 14, 2023

View reviewed changes

sbc100 approved these changes Apr 17, 2023

View reviewed changes

yamt added 3 commits April 19, 2023 10:50

dlmalloc: a comment

01cac38

dlmalloc: ABORT on the known linker bug

fd9a8ce

dlmalloc: avoid LLVM optimizer eliminating the __heap_end check

aa35d19

yamt mentioned this pull request Jun 21, 2023

Update CI to use latest Clang version #423

Closed

sbc100 requested a review from sunfishcode June 28, 2023 16:52

abrown merged commit d8abbaa into WebAssembly:main Jul 11, 2023

cuviper mentioned this pull request Aug 26, 2023

Port the fix from #377 to emmmalloc. #378

Open

cuviper mentioned this pull request Jan 10, 2024

emmalloc: use __heap_end instead of sbrk(0) #462

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dlmalloc: require __heap_end #394

dlmalloc: require __heap_end #394

yamt commented Feb 17, 2023 •

edited

Loading

sbc100 Feb 17, 2023

yamt Feb 17, 2023

yamt commented Feb 17, 2023

sbc100 commented Feb 17, 2023

yamt commented Feb 17, 2023

sbc100 commented Feb 17, 2023

yamt commented Feb 17, 2023

yamt commented Feb 17, 2023

TerrorJack commented Feb 17, 2023

yamt commented Mar 22, 2023

yamt commented Apr 14, 2023

sbc100 Apr 14, 2023

yamt Apr 14, 2023

sbc100 Apr 14, 2023

yamt Apr 17, 2023

sbc100 Apr 14, 2023

yamt Apr 17, 2023

sbc100 Apr 17, 2023

sbc100 Apr 17, 2023

sbc100 Apr 17, 2023

yamt commented Apr 19, 2023

sbc100 commented Jun 28, 2023

yamt commented Jun 29, 2023

dlmalloc: require __heap_end #394

dlmalloc: require __heap_end #394

Conversation

yamt commented Feb 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamt commented Feb 17, 2023

sbc100 commented Feb 17, 2023

yamt commented Feb 17, 2023

sbc100 commented Feb 17, 2023

yamt commented Feb 17, 2023

yamt commented Feb 17, 2023

TerrorJack commented Feb 17, 2023

yamt commented Mar 22, 2023

yamt commented Apr 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamt commented Apr 19, 2023

sbc100 commented Jun 28, 2023

yamt commented Jun 29, 2023

yamt commented Feb 17, 2023 •

edited

Loading