Apply the sbrk pointer's location to the wasm, avoiding the need to import it by kripken · Pull Request #9397 · emscripten-core/emscripten

kripken · 2019-09-06T01:16:16Z

Importing DYNAMICTOP_PTR is not compatible with wasi, and it's smaller anyhow to just have the value in the binary.

This does so by computing the sbrk ptr's static location in the JS compiler, as always, and noting it in the metadata. We then call the binaryen post-emscripten pass with that data later and it applies it.

As a nice result, we can get rid of a bunch of code for importing DYNAMICTOP_PTR, and replace the sbrk implementation in JS with a very simple one in C.

Please note:

Since our malloc's threadsafety is done using locks, this sbrk does no additional guarding. That's more efficient than before (we used atomics in sbrk). However, in theory someone might call sbrk not through malloc - I'm not sure that's worth supporting or not?
This slightly regresses other.test_minimal_runtime_code_size expectations, since now we use a standard sbrk in C, while before there were a bunch of custom options for the library.js version like USES_DYNAMIC_ALLOC=2. That mode 2 isn't documented so I'm not sure how to reproduce similar behavior in the new implementation. (Also it looks like MALLOC=none would achieve the same as USES_DYNAMIC_ALLOC=0, so maybe I'm misunderstanding this option, or maybe they are redundant?) In any case, the regression is quite small, and this is just on fastcomp - this sbrk refactoring helps on the wasm backend side. cc @juj
This depends on [wasm2js] Fix memory.size WebAssembly/binaryen#2330 , SafeHeap: Prepare for emscripten_get_sbrk_ptr WebAssembly/binaryen#2331, and Handle sbrk import by emscripten+upstream in SafeHeap WebAssembly/binaryen#2332 to land and roll before tests can pass.

After this PR, I can open a PR with a WASI option, as this is the last main blocker for getting "hello world" to be a valid wasi program.

The _callback function being included in EXPORT_FUNCTIONS but it is a static function which is not exportable with the wasm backend. Since this is never used simply remove it from the list.

This reverts commit dbe1523 which was mistakenly committed to master.

…This also helps wasi.

To allow #2331 to roll, I forgot that upstream and fastcomp handle sbrk differently. This fixes that, and handles the upstream case where we import sbrk itself from JS. We can simplify this after emscripten-core/emscripten#9397 lands, however, it may also be nice to keep the backwards compatibility for running on existing wasm files in the wild.

jgravelle-google · 2019-09-06T18:55:28Z

          if shared.Settings.BINARYEN_PASSES:
            passes += parse_passes(shared.Settings.BINARYEN_PASSES)
        else:
+          # Safe heap must run before post-emscripten, so post-emscripten can apply the sbrk ptr


Should this be enforced in binaryen itself? Or, are there other places where we might want to enforce this invariant? If a user (of binaryen, or emscripten via -s BINARYEN_PASSES) is manually specifying passes, and does this the other way around, should we do anything?

Hmm, good question.

In theory a user may want the opposite order, if they don't want such functions to be affected. That seems unlikely though, so perhaps a warning in Binaryen would be useful. We currently don't have a mechanism for that, though (it just runs the passes blindly) but maybe we should add one. Another possible example is people running asyncify without optimizations (so not an order issue, but one pass that works better with others, maybe also worth a warning).

jgravelle-google · 2019-09-06T19:12:15Z

 #endif // ALLOW_MEMORY_GROWTH
  },

-#if MINIMAL_RUNTIME && !ASSERTIONS && !ALLOW_MEMORY_GROWTH


Yay deleting code 🎉

… we can ignore the sbrk-related checks, but still do alignment and segfault checking.

…eed to be emterpreted, they are safe to run normally.

kripken · 2019-09-07T15:54:56Z

Some more fixes:

Emterpreter fixes: We don't need to emterpret the new sbrk and emscripten_get_sbrk_ptr, and also it is safe to call the latter at all times (it just returns a number), so the special Emterpreter assertions that check if we are running invalid code during a sleep can ignore it.
I added support for ignoring errno (SUPPORT_ERRNO=0) in the new sbrk. That reduces part of the minimal_runtime size regression here, since it uses that flag in the test.

kripken · 2019-09-13T00:50:21Z

Ok, I worried some parts of this might be controversial, so kept open for a week, but looks like there are no concerns, landing.

dschuff · 2019-09-13T20:45:30Z

Hey, I arrived just in time for a post-commit drive-by comment!
I think sbrk needs to be thread-safe. It's a pretty well-known syscall (even more so since it's the only one we have for allocating memory) and it's common enough to implement one's own allocator that I think we can't assume that everyone is always using one of our mallocs. The cmpxchg behavior seems fine, and we can #ifdef it out the way dlmalloc does.

dschuff · 2019-09-13T20:46:19Z

+  intptr_t old_brk = *sbrk_ptr;
+  // TODO: overflow checks
+  intptr_t new_brk = old_brk + increment;
+#if __wasm__


Shouldn't this be #ifdef __wasm__? I don't think fastcomp defines __wasm__ == 0?

dschuff · 2019-09-13T20:47:25Z

+  if (new_brk > old_size) {
+    // Try to grow memory.
+    intptr_t diff = new_brk - old_size;
+    if (!emscripten_resize_heap(new_brk)) {


It seems odd that we are using __builtin_wasm_memory_size but not __builtin_wasm_grow_memory. Why the discrepancy?

The tricky thing is that growing memory requires us to update the JS views. Doing the resizing in JS makes that easy.

kripken · 2019-09-13T20:56:24Z

Thanks @dschuff, will open a followup PR for those issues.

Followup to #9397

sbc100 · 2019-09-17T18:53:13Z

-    }[self.malloc])]
+    }[self.malloc])
+    sbrk = shared.path_from_root('system', 'lib', 'sbrk.c')
+    return [malloc, sbrk]


Why isn't this just part of libc? Why compile it N times into each libmalloc if its the same code in each one?

I guess it could be part of libc too.

In general there aren't N builds, though, there is normally just one build of malloc. Unless the user builds with flags that actually require a different sbrk build, like pthreads or errno etc. So it wouldn't be the same code in each one.

…mport it (emscripten-core#9397) Importing DYNAMICTOP_PTR is not compatible with wasi, and it's smaller anyhow to just have the value in the binary. This does so by computing the sbrk ptr's static location in the JS compiler, as always, and noting it in the metadata. We then call the binaryen post-emscripten pass with that data later and it applies it. As a nice result, we can get rid of a bunch of code for importing DYNAMICTOP_PTR, and replace the sbrk implementation in JS with a very simple one in C. * Since our malloc's threadsafety is done using locks, this sbrk does no additional guarding. That's more efficient than before (we used atomics in sbrk). However, in theory someone might call sbrk not through malloc, which this does not handle atm. * This slightly regresses other.test_minimal_runtime_code_size expectations, since now we use a standard sbrk in C, while before there were a bunch of custom options for the library.js version like USES_DYNAMIC_ALLOC=2. That mode 2 isn't documented so I'm not sure how to reproduce similar behavior in the new implementation. (Also it looks like MALLOC=none would achieve the same as USES_DYNAMIC_ALLOC=0, so maybe I'm misunderstanding this option, or maybe they are redundant?) In any case, the regression is quite small, and this is just on fastcomp - this sbrk refactoring helps on the wasm backend side. * Emterpreter fixes: We don't need to emterpret the new sbrk and emscripten_get_sbrk_ptr, and also it is safe to call the latter at all times (it just returns a number), so the special Emterpreter assertions that check if we are running invalid code during a sleep can ignore it.

Followup to emscripten-core#9397

sbc100 and others added 30 commits October 19, 2018 01:12

Fix sqlite test

dbe1523

The _callback function being included in EXPORT_FUNCTIONS but it is a static function which is not exportable with the wasm backend. Since this is never used simply remove it from the list.

Revert "Fix sqlite test"

941bbc6

This reverts commit dbe1523 which was mistakenly committed to master.

use wasi's fd_write

59fae79

fix fastcomp

798c658

fastcomp fix

f40aa15

fix

13937d7

fixes

26ee381

fixes

8554c6a

fixes

fbf832d

comment [ci skip]

918c40f

fix pthreads [ci skip]

476316d

text

3ccc7e2

fixes

55a889f

fix

8be488a

Optimize stdout stream handling in musl: Avoid unnecessary syscalls. …

70c0a13

…This also helps wasi.

test fixes

c59108b

fix

72b25e8

feddback

05e81f8

undo

c3ede2f

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

5f74679

Merge remote-tracking branch 'origin/incoming' into wasi3

db053b8

Merge remote-tracking branch 'origin/wasi3' into wasi4

ac9bacc

Merge remote-tracking branch 'origin/incoming' into wasi4

8ca6e3d

Merge remote-tracking branch 'origin/incoming' into wasi3

8c188e2

wip

2c76770

fix

eed6052

wip [ci skip]

76f3066

fix [ci skip]

f0952f5

fix

87f9c2e

fixes

e8de5a9

Merge remote-tracking branch 'origin/incoming' into wasi5_small

6d15e35

jgravelle-google approved these changes Sep 6, 2019

View reviewed changes

kripken and others added 7 commits September 6, 2019 14:46

fix fastcomp asm.js safe heap [ci skip]

c9a52a4

Merge remote-tracking branch 'origin/incoming' into wasi5_small

ac63424

fix a comment

153b76a

Fix SAFE_HEAP in asm.js when sbrk is not even linked in. In that case…

3913244

… we can ignore the sbrk-related checks, but still do alignment and segfault checking.

Support ignoring errno in sbrk, which helps minimal_runtime tests.

162d8d1

flake8

b8d7f1f

Emterpretify fixes: the new sbrk and emscripten_get_sbrk_ptr do not n…

7916f47

…eed to be emterpreted, they are safe to run normally.

kripken and others added 7 commits September 11, 2019 16:50

Merge remote-tracking branch 'origin/incoming' into wasi4_small

34c7577

update

ef7baa6

update fastcomp too

10b434e

Merge remote-tracking branch 'origin/incoming' into wasi5_small

3e61ded

update

e58c737

Merge remote-tracking branch 'origin/incoming' into wasi5_small

27e4a7b

update

52b213d

kripken merged commit afc1616 into incoming Sep 13, 2019

kripken deleted the wasi5_small branch September 13, 2019 00:51

dschuff reviewed Sep 13, 2019

View reviewed changes

kripken mentioned this pull request Sep 13, 2019

Make sbrk threadsafe #9438

Merged

kripken added a commit that referenced this pull request Sep 13, 2019

Make sbrk threadsafe (#9438)

e421fa1

Followup to #9397

sbc100 reviewed Sep 17, 2019

View reviewed changes

Squareys mentioned this pull request Oct 6, 2019

Empty EmscriptenApplication Crashed mosra/magnum#376

Closed

belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020

Make sbrk threadsafe (emscripten-core#9438)

42838cd

Followup to emscripten-core#9397

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply the sbrk pointer's location to the wasm, avoiding the need to import it#9397

Apply the sbrk pointer's location to the wasm, avoiding the need to import it#9397
kripken merged 67 commits into
incomingfrom
wasi5_small

kripken commented Sep 6, 2019 •

edited

Loading

Uh oh!

jgravelle-google Sep 6, 2019

Uh oh!

kripken Sep 7, 2019

Uh oh!

jgravelle-google Sep 6, 2019

Uh oh!

kripken commented Sep 7, 2019

Uh oh!

kripken commented Sep 13, 2019

Uh oh!

dschuff commented Sep 13, 2019

Uh oh!

dschuff Sep 13, 2019

Uh oh!

dschuff Sep 13, 2019

Uh oh!

kripken Sep 13, 2019

Uh oh!

kripken commented Sep 13, 2019

Uh oh!

sbc100 Sep 17, 2019

Uh oh!

kripken Sep 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

kripken commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented Sep 7, 2019

Uh oh!

kripken commented Sep 13, 2019

Uh oh!

dschuff commented Sep 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented Sep 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kripken commented Sep 6, 2019 •

edited

Loading