Stop including malloc/free by default #12213
Conversation
This reverts commit d1ec7dc.
| @@ -1846,10 +1846,14 @@ var LibrarySDL = { | |||
| SDL_SetError: function() {}, | |||
|
|
|||
| SDL_malloc__sig: 'ii', | |||
| SDL_malloc: 'malloc', | |||
There was a problem hiding this comment.
How did this work previously?
There was a problem hiding this comment.
It worked thanks to how aliases work: a: "b" means a is an alias of b, so it just emits
function a() { b() }Basically it does an indirection. And that happens to work if b is in JS or in compiled code. It is confusing though which is why I changed it.
Thinking on it now, though, even better would be to implement these in C. However it's for SDL1 and not 2 so it matters less.
src/runtime_strings_extra.js
Outdated
| @@ -208,10 +208,14 @@ function lengthBytesUTF32(str) { | |||
| // Allocate heap space for a JS string, and write it there. | |||
| // It is the responsibility of the caller to free() that memory. | |||
| function allocateUTF8(str) { | |||
| #if '_malloc' in IMPLEMENTED_FUNCTIONS | |||
| var size = lengthBytesUTF8(str) + 1; | |||
| var ret = _malloc(size); | |||
There was a problem hiding this comment.
I'm surprised not to see the pattern used more in the library code? Is that idea to wrap all malloc calls in #if '_malloc' in IMPLEMENTED_FUNCTIONS
I wonder if this could be rewritten as:
var size = lengthBytesUTF8(str) + 1;
var ret = {{{ makeMalloc("allocateUTF8") }}} (size);
if (ret) stringToUTF8Array(str, HEAP8, ret, size);
return ret;
Thus avoiding the ifdef here?
var size = lengthBytesUTF8(str) + 1;
There was a problem hiding this comment.
Thanks, good idea to use a macro here, done.
This doesn't happen more because the deps_info mechanism is usually enough. But in 2 places we can't use deps (in the FS we don't want any FS usage to force malloc, and allocUTF8 isn't tracked by deps - so in both cases we don't have the fine-grained tracking we need for deps_info).
tests/test_other.py
Outdated
| @@ -9670,5 +9652,50 @@ def test_standalone_export_main(self): | |||
| # We should consider making this a warning since the `_main` export is redundant. | |||
| self.run_process([EMCC, '-sEXPORTED_FUNCTIONS=[_main]', '-sSTANDALONE_WASM', '-c', path_from_root('tests', 'core', 'test_hello_world.c')]) | |||
|
|
|||
| def test_unincluded_malloc(self): | |||
There was a problem hiding this comment.
test_missing_malloc_export ?
tests/test_other.py
Outdated
| } | ||
| ''') | ||
| self.run_process([EMCC, 'unincluded_malloc.c']) | ||
| self.assertContained('malloc was not included, but is needed in allocateUTF8. Adding "_malloc" to EXPORTED_FUNCTIONS should fix that. This may be a bug in the compiler, please file an issue.', self.run_js('a.out.js')) |
There was a problem hiding this comment.
use self.do_runf(<filename>, <expected_string>) ?
tests/test_other.py
Outdated
| self.assertContained("malloc() called but not included in the build - add '_malloc' to EXPORTED_FUNCTIONS", out) | ||
| self.assertContained("free() called but not included in the build - add '_free' to EXPORTED_FUNCTIONS", out) | ||
|
|
||
| def test_unincluded_malloc_2(self): |
There was a problem hiding this comment.
test_missing_malloc_export_indirect_call? Or something more meaningful than "2" ?
After we no longer include malloc/free by default (#12213) we can remove the hack in sbrk which avoided using static memory. This gives some nice code size improvements. However this can't be as simple as we'd hope because of dynamic linking plus SAFE_HEAP. In that mode we may try to use sbrk_val before the dynamic linker has set it up. This is because SAFE_HEAP is not aware of dynamic linking - it just instruments all loads and stores, period, including ones that happen during dynamic linking startup. To avoid ordering issues there, initialize sbrk_val manually.
This removes the mmap dependency from musl's locale code, and by doing that fixes a dependency bug with libc++ using locale code that causes mmap to be included, and mmap from JS needs malloc/free which we no longer include since #12213 That seems nicer anyhow, as mmap was only used there when the user provided MUSL_LOCPATH - I don't think it's worth it for us to support that nonstandard option.
Before this we would always include malloc/free, and rely on metadce to remove them
when not needed. But this has downsides:
sbrkusing__heap_base, asif malloc is included first and removed by metadce that would still
leave some data in the memory initialization, increasing code size
without the workaround we had in
sbrkto avoid it.Much of the work here is to get
deps_info.jsonto handle things properly andinclude malloc/free when needed. I did this twice to double-check, but it is still
someone error-prone, as we need each code path that reached a malloc from
JS to be identified properly.
Some places use malloc/free in locations that are not part of the JS deps
system, and those now use
makeMallocAbort. In those places we knowwe should not reach them if malloc is not included, so we just have an
abort there. That would catch errors with
deps_info.json.In assertions builds, also help users by adding malloc/free in JS that show
a clear error if called, if malloc/free was not included but the user called
them anyhow.
With this PR we can remove the hack for sbrk, but I wanted to keep that
for a later PR as this is already not small.
#11860 makes this practical, but it is still tricky...