-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
emmalloc: Add an option to not define the standard exports #20487
Changes from all commits
988dc95
dbc03c1
5137356
8fec8dd
d7013e0
a5b17a4
c33fe4a
de0c5db
9667e10
2d29660
54500c9
34f67aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,36 @@ | ||||||||||
#include <assert.h> | ||||||||||
#include <emscripten/console.h> | ||||||||||
#include <emscripten/emmalloc.h> | ||||||||||
|
||||||||||
int main() { | ||||||||||
// Verify we can call both malloc and emmalloc_malloc, and that those are | ||||||||||
// different functions, unless TEST_EMMALLOC_IS_MALLOC is set (in that case, | ||||||||||
// emmalloc is malloc because we let emmalloc define the standard exports like | ||||||||||
// malloc). | ||||||||||
|
||||||||||
// We have allocated nothing so far, but there may be some initial allocation | ||||||||||
// from startup. | ||||||||||
size_t initial = emmalloc_dynamic_heap_size(); | ||||||||||
emscripten_console_logf("initial: %zu\n", initial); | ||||||||||
|
||||||||||
const size_t ONE_MB = 1024 * 1024; | ||||||||||
void* one = malloc(ONE_MB); | ||||||||||
assert(one); | ||||||||||
#ifndef TEST_EMMALLOC_IS_MALLOC | ||||||||||
// We have allocated using malloc, but not emmalloc, so emmalloc reports no | ||||||||||
// change in usage. | ||||||||||
assert(emmalloc_dynamic_heap_size() == initial); | ||||||||||
#else | ||||||||||
// malloc == emmalloc_malloc, so emmalloc will report additional usage (of the | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could maybe simplify this test by simply asserting that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By taking the address, you mean? I wasn't sure that is entirely reliable in general. In particular I'd worry about there being some kind of trampoline but it still going to the same target. (With aliases that wouldn't happen, but we might refactor to use another method.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we use another method I think its fine to update this test at that point... but that other method would seem like a bad idea since it would incur a cost. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there would be a risk that we could forget to update this test... it would be a needle in the haystack of the testsuite, that is, we'd need to somehow realize it needs to be updated then. Most likely we'd forget and it could start to pass incorrectly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But surely the tests would fail because the assert "malloc == emmalloc" would fail right away, no? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In other words, I think it's safer to test this by checking the functionality - that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough. This lgtm. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I realize you're right that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried to add emscripten/system/lib/emmalloc.c Lines 836 to 839 in 4608c2e
So in a debug build the two are obviously different. It turns out that even in a release build they are, because LLVM allocates two function pointer addresses, one for each. Optimizations (inlining + merging similar functions) end up making them identical, but there are still two function pointers, and the table ends up with two slots with the same function name in it... It might be worth using an alias rather than a trampoline here, but I'm not sure if there was a reason not to do that. Anyhow, let's leave that separate from this PR, so I'll land this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't get I fixed the code to not have trampolines, and the function pointers are identical, and it works in a debug build, but the LLVM optimizer assumes they are not equal all the time. I seem to remember something about that being undefined behavior, that in some architectures function pointers behave very oddly... so I guess it's that. |
||||||||||
// size of the allocation, or perhaps more if it overallocated as an | ||||||||||
// optimization). | ||||||||||
assert(emmalloc_dynamic_heap_size() >= initial + ONE_MB); | ||||||||||
#endif | ||||||||||
|
||||||||||
void* two = emmalloc_malloc(ONE_MB); | ||||||||||
assert(two); | ||||||||||
// We have allocated using emmalloc, so now emmalloc definitely reports usage. | ||||||||||
assert(emmalloc_dynamic_heap_size() >= initial + ONE_MB); | ||||||||||
|
||||||||||
emscripten_console_log("success"); | ||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14018,3 +14018,18 @@ def test_hello_world_argv(self): | |
|
||
def test_arguments_global(self): | ||
self.emcc(test_file('hello_world_argv.c'), ['-sENVIRONMENT=web', '-sSTRICT', '--closure=1', '-O2']) | ||
|
||
@parameterized({ | ||
'no_std_exp': (['-DEMMALLOC_NO_STD_EXPORTS'],), | ||
# When we let emmalloc build with the standard exports like malloc, | ||
# emmalloc == malloc. | ||
'with_std_exp': (['-DTEST_EMMALLOC_IS_MALLOC'],), | ||
}) | ||
def test_emmalloc_in_addition(self, args): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't love the name of this test (or the I'm not sure you need the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I considered "standalone", but I felt it is too similar to "standalone wasm" and so it's confusing. I could rename to "explicit" but I think "in addition" is slightly more clear? I agree There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about "alongside" instead of "in addition" or "explicit"? This does test that we can have emmalloc alongside another malloc at the same time. |
||
# Test that we can use emmalloc in addition to another malloc impl. When we | ||
# build emmalloc using -DEMMALLOC_NO_STD_EXPORTS it will not export malloc | ||
# etc., and only provide the emmalloc_malloc etc. family of functions that | ||
# we can use. | ||
emmalloc = path_from_root('system', 'lib', 'emmalloc.c') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When you build emmalloc in addition to malloc here.. how does it work? How does emmalloc get it underlying memory? Are they both able to grab memory from the sbrk region somehow? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both use sbrk to allocate. Neither frees anything to sbrk, so things work out. (This won't be an issue for the intended use of this, though, which is that the second allocator calls emmalloc which calls sbrk.) |
||
self.run_process([EMCC, test_file('other/test_emmalloc_in_addition.c'), emmalloc] + args) | ||
self.assertContained('success', self.run_js('a.out.js')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, do you know why we are not using aliases here? Seems like it would be code size and performance win to not go through this wrapper. Perhaps we should make that change as a separate change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I just saw this comment now after commenting above on this. I'm not sure why we trampoline here, and as I said there, optimizations remove 99% of the overhead but we do keep a table slot for them, so yeah, this might be worth improving.