Remove DYNAMICTOP_PTR #12057

kripken · 2020-08-26T20:42:37Z

DYNAMICTOP_PTR has been a pointer to where DYNAMICTOP is stored,
which represented the top of "dynamic" memory (not static, and not stack; or
in other words, dynamic == managed by sbrk/malloc). We represented it on the
JS side so that we could allocate from JS directly, in particular during startup.
However, that has caused a bunch of complexity that is not really worth it, and
it's also work done after link that we'd like to remove (WebAssembly/binaryen#3043).

This PR moves us to a place where malloc-like allocation is disallowed from
JS during startup. Instead, JS should call sbrk or malloc normally (which can
only be done after startup).

This change removes the final static allocation from JS, which means that we
no longer adjust memory layout after link, and therefore this PR removes the
special updating of the stack pointer and sbrk location that we used to do.
(This just removes the --pass-arg stuff for those in emcc.py, to show that
this works and passes tests - there is a bunch more code that can be removed
but is deferred to keep this as small as possible.)

Dynamic linking, however is an exception: we need to allocate room for
dynamic libraries during startup (the main module needs them before it
starts to run). To support that, this PR still keeps around getMemory()
(which does a dynamic allocation during startup) and dynamicAlloc in
a simplified form. That form just updates __heap_base as we allocate,
and then when the main program starts, __heap_base is a normal
extern global that it receives, and it initializes sbrk using that (after
that point, dynamic allocations are disallowed and asserted against).

This change should not be user-visible, except for removing the allocate()
JS function's ALLOC_DYNAMIC option (since we no longer allow that type
of allocation).

This can only land when it removes the last static allocation from JS,
which means it is blocked by #12042 and #12055.

This has a small code size benefit, basically it removes things like
these:

340,341d339
< var DYNAMIC_BASE = 5263904, DYNAMICTOP_PTR = 20992;
< 
361,362d358
< HEAP32[DYNAMICTOP_PTR >> 2] = DYNAMIC_BASE;
<

The benefit to pthreads builds is larger as it removes a bunch of special
code for DYNAMICTOP_PTR that we had there.

Note that I'm not totally happy with the sbrk implementation, but I
tried some other approaches and they were worse for code size. But
ideas welcome.

This reverts commit 464a3da.

sbc100

I ❤️ this change. I've been wanting to do this for a very long time.

sbc100 · 2020-08-27T11:36:53Z

src/library_emmalloc.js

  emmalloc_unclaimed_heap_memory: function() {
-  	var dynamicTop = HEAPU32[_emscripten_get_sbrk_ptr()>>2];
+	var dynamicTop = _sbrk();


I love the idea of just use _sbrk() here.. in my various attempts at this I just moved _emscripten_get_sbrk_ptr into normal C code (which also works), but this is simpler.

sbc100 · 2020-08-27T11:40:33Z

src/runtime_safe_heap.js

-  assert(DYNAMICTOP_PTR);
-  assert(HEAP32[DYNAMICTOP_PTR>>2] <= HEAP8.length);
+  if (dest + bytes > brk) abort('segmentation fault, exceeded the top of the available dynamic heap when storing ' + bytes + ' bytes to address ' + dest + '. DYNAMICTOP=' + brk);
+  assert(brk);


This assert is no long meaningful I think since we are no longer dealing with a pointer here.

I think it still has value. 0 is never a valid break location, and may indicate that we've failed to initialize sbrk properly.

In fact this could be expanded into assert(brk >= STACK_BASE) since the break has to be after globals and the stack. Or is that too much?

Adding the stronger assert now, but I can remove it if you think I'm missing something.

sbc100 · 2020-08-27T11:40:44Z

src/runtime_safe_heap.js

-  assert(DYNAMICTOP_PTR);
-  assert(HEAP32[DYNAMICTOP_PTR>>2] <= HEAP8.length);
+  if (dest + bytes > brk) abort('segmentation fault, exceeded the top of the available dynamic heap when loading ' + bytes + ' bytes from address ' + dest + '. DYNAMICTOP=' + brk);
+  assert(brk);


system/include/emscripten/heap.h

sbc100 · 2020-08-27T11:45:12Z

system/lib/sbrk.c

@@ -29,6 +29,23 @@
 #define SET_ERRNO()
 #endif

+static intptr_t sbrk_val = 0;


Can you just do static intptr_t sbrk_val = (intptr_t)&__heap_base right here at least in the non-pthread case?

In fact, even in the pthread case I should work since it will generate an assignment from GOT.mem.__heap_base on startup

That can work, but is less good for code size it seems. The issue is that static intptr_t sbrk_val = (intptr_t)&__heap_base; adds that number into the memory segment initializer. If wasm-opt then manages to remove malloc and sbrk, it still can't remove that. So super-tiny programs end up almost empty, but with a couple of bytes in a memory segment for no obvious reason. We have a test that fails on this, in fact, the metadce test that exports nothing and expects to get an empty wasm file with no data and no functions.

So it's better for super-tiny programs to do it this way. And for large programs that use malloc, it doesn't matter much either way we do it. But, I'm not totally happy with this I admit...

I have quite a strong preference for static intptr_t sbrk_val = (intptr_t)&__heap_base; ... since it lets the linker do the work of assign the correct value.

Also, in smaller programs that don't reference sbrk_val at all the linker can eliminate this global completely, no?

This kind of micro optimization doesn't seem worth it. There is also the cost in terms of wasm code size because the size of you emscripten_get_sbrk_ptr is bigger that just:

return &sbrk_val

Also, in smaller programs that don't reference sbrk_val at all the linker can eliminate this global completely, no?

No, because we link in malloc by default, and only remove it through metadce when possible.

I think it would be good to move towards a model that does not link in malloc by default, and can then do it the way we both prefer, but that's not trivial and I think should be kept separate from this. For now, I'd like to not regress code size or remove the existing test that breaks unless this is properly optimized.

(It's true this has a small cost in code size when malloc is included. But if malloc is there then it's a tiny addition to the much larger malloc; and if malloc isn't there, the saved code size with this optimization is much more noticeable.)

I'm still not convinced... the only programs that can benefit here are ones that don't use the heap at all (not just malloc... but any heap usage at all right?)

Do you think we need to be optimizing for such programs?

How about we add an option to not export malloc by default that such (very unusual) programs could use to get this saving?

MINIMAL_RUNTIME and STANDALONE_WASM programs already don't export malloc by default so won't be effected.

I guess I'm OK with the TODO here... but I'd perhaps rather invert it by added a TODO to emcc.py line 1408 that says "TODO: exporting malloc here causes sbrk_ptr to be included and it cannot be removed by metadce. Find a way to avoid exporting malloc by default?"

:)

But lgtm this way too if you feel strongly.

I'm still not convinced... the only programs that can benefit here are ones that don't use the heap at all (not just malloc... but any heap usage at all right?) Do you think we need to be optimizing for such programs?

Yes, I think it's nice that we emit a tiny wasm for

(module (func "double_it" (param $x i32) (i32.mul (i32.const 2) (local.get $x))))

Such pure computational stuff shouldn't have data segments, and it would be a regression to change that.

But the good news is that I have a PR almost ready for this, in the nomalloc branch! So I don't think any TODO comment is needed here. It passes tests and shrinks code size, but I'll do some more careful work to try to avoid regressions before I open it.

sbc100 · 2020-08-27T18:08:44Z

system/lib/sbrk.c

@@ -41,7 +58,6 @@ void *sbrk(intptr_t increment) {
  intptr_t expected;
  while (1) {
 #endif // __EMSCRIPTEN_PTHREADS__
-
    intptr_t* sbrk_ptr = emscripten_get_sbrk_ptr();


You can also drop this line and just &sbrk_val everywhere in this function instead?

See emscripten-core/emscripten#9039 (comment) The valid stack area is a region [A, B] in memory. Previously we just checked that new stack positions S were S >= A, which prevented us from growing too much (the stack grows down). But that only worked if the growth was small enough to not overflow and become a big unsigned value. This PR makes us check the other way too, which requires us to know where the stack starts out at. This still supports the old way of just passing in the growth limit. We can remove it after the roll. In principle this can all be done on the LLVM side too after emscripten-core/emscripten#12057 but I'm not sure of the details there, and this is easy to fix here and get testing up (which can help with later LLVM work). This helps emscripten-core/emscripten#11860 by allowing us to clean up some fastcomp-specific stuff in tests.

The statement that lived inside this block was removed in #12057.

All this setting was doing was setting MALLOC=none so it doesn't seem like it needs to exist as a separate setting. I think it used to do more, for example, prior to #12790 it used was used in some JS library code. Another PR that removed a lot of usage of this settings was #12057. Simply removing it simplifies another change I'm working on: #18905. Also, the docs were incorrect in stating it was ignored except under MINIMAL_RUNTIME, but we had no testing for it.

kripken added 30 commits August 24, 2020 15:00

wip

ebaea06

[ci skip]

743bed8

Remvoe

e6b5864

changelog

76e7ac2

Merge branch 'no-noheapcopy' into yolo

023d9bb

more

1fec98b

fix

e9399f7

Fix [ci skip]

0c28616

NO DYNAMICTOP_PTR [ci skip]

f4369e3

a hack for now

25946c1

fix tests

4ffbd6f

fixes

2e4e702

fixes [ci skip]

299a959

fix

2401c79

fix

83e6a85

all looks good [ci skip]

cfe01e5

__heap_base? [ci skip]

3876ec2

Merge remote-tracking branch 'origin/master' into yolo2

f6557cf

todo

5296cb3

fix

6162325

hacky

4503ab8

Merge remote-tracking branch 'origin/master' into yolo2

c027517

fix

83ae76f

update tests

05f04c4

cleanup

cba813d

more cleanup

04ab551

more cleanup

26701ce

more cleanup

4ca22ff

Merge remote-tracking branch 'origin/master' into yolo2

2c56ef5

update tests

c92f7cc

kripken added 4 commits August 26, 2020 11:58

comments

cfd2748

simplify

464a3da

Revert "simplify"

7b9fc2e

This reverts commit 464a3da.

[ci skip]

741b041

kripken requested a review from sbc100 August 26, 2020 20:42

sbc100 reviewed Aug 27, 2020

View reviewed changes

kripken and others added 8 commits August 28, 2020 08:35

Merge remote-tracking branch 'origin/master' into yolo2

1d631f7

comment [ci skip]

dbe8b15

Merge remote-tracking branch 'origin/master' into yolo2

e87ce35

Better assertion

a968def

fix

323e16c

fix changelog

eaf9c7e

fix

7ff21a5

Merge remote-tracking branch 'origin/master' into yolo2

4c4831d

kripken mentioned this pull request Sep 1, 2020

StackCheck: Check both under and overflow WebAssembly/binaryen#3091

Merged

sbc100 approved these changes Sep 2, 2020

View reviewed changes

merge

be4eb06

kripken merged commit 0882510 into master Sep 2, 2020

kripken deleted the yolo2 branch September 2, 2020 21:50

sbc100 mentioned this pull request Sep 22, 2020

Remove unused stack-pointer and emscripten-sbrk-ptr pass args from PostEmscripten WebAssembly/binaryen#3161

Merged

sbc100 added a commit that referenced this pull request Oct 30, 2020

Remove empty block from runtime_init_memory.js. NFC.

b4d3df8

The statement that lived inside this block was removed in #12057.

sbc100 mentioned this pull request Oct 30, 2020

Remove empty block from runtime_init_memory.js. NFC. #12649

Merged

sbc100 added a commit that referenced this pull request Oct 30, 2020

Remove empty block from runtime_init_memory.js. NFC. (#12649)

78fbe5f

The statement that lived inside this block was removed in #12057.

zackangelo mentioned this pull request Mar 26, 2022

emscripten unexpected dynamic_base 0 when trying to run a wasm file. wasmerio/wasmer#1898

Closed

sbc100 mentioned this pull request Apr 12, 2023

Deprecate the USES_DYNAMIC_ALLOC setting #19164

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove DYNAMICTOP_PTR #12057

Remove DYNAMICTOP_PTR #12057

kripken commented Aug 26, 2020

sbc100 left a comment

sbc100 Aug 27, 2020

sbc100 Aug 27, 2020

kripken Aug 27, 2020

kripken Aug 31, 2020

sbc100 Aug 27, 2020

sbc100 Aug 27, 2020

kripken Aug 27, 2020

sbc100 Aug 27, 2020

kripken Aug 27, 2020

sbc100 Aug 31, 2020

sbc100 Aug 31, 2020

sbc100 Aug 31, 2020

kripken Aug 31, 2020

sbc100 Aug 31, 2020

sbc100 Aug 27, 2020

Remove DYNAMICTOP_PTR #12057

Remove DYNAMICTOP_PTR #12057

Conversation

kripken commented Aug 26, 2020

sbc100 left a 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

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