Skip to content

Reapply "Prefer calloc over of malloc+zeroMemory. NFC" #22596

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

Merged
merged 2 commits into from
Sep 20, 2024

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Sep 19, 2024

This reverts commit 50e0b07.

This change also adds calloc to the list of symbols that we provide emscripten_builtin_ variants for.

@sbc100 sbc100 requested a review from dschuff September 19, 2024 23:56
@sbc100
Copy link
Collaborator Author

sbc100 commented Sep 19, 2024

See the second commit in this PR for the fix to the lsan issue.

@dschuff
Copy link
Member

dschuff commented Sep 20, 2024

LGTM; can you also mention in the commit message that this makes the calloc symbol handled more like malloc and free?

@sbc100 sbc100 merged commit 16a0bf1 into emscripten-core:main Sep 20, 2024
28 checks passed
@sbc100 sbc100 deleted the calloc branch September 20, 2024 02:02
@dschuff
Copy link
Member

dschuff commented Sep 25, 2024

This seems to be breaking test_externref_emjs_dynlink in UBSan mode:
https://logs.chromium.org/logs/emscripten-releases/buildbucket/cr-buildbucket/8735974517183441729/+/u/Emscripten_testsuite__UBSan_/stdout

error: undefined symbol: malloc (referenced by $withBuiltinMalloc, referenced by $getMemory, referenced by $loadWebAssemblyModule, referenced by $loadDynamicLibrary, referenced by $loadDylibs, referenced by root reference (e.g. compiled C/C++ code))
warning: To disable errors for undefined symbols use `-sERROR_ON_UNDEFINED_SYMBOLS=0`
warning: _malloc may need to be added to EXPORTED_FUNCTIONS if it arrives from a system library
error: undefined symbol: free (referenced by $withBuiltinMalloc, referenced by $getMemory, referenced by $loadWebAssemblyModule, referenced by $loadDynamicLibrary, referenced by $loadDylibs, referenced by root reference (e.g. compiled C/C++ code))
warning: _free may need to be added to EXPORTED_FUNCTIONS if it arrives from a system library
error: undefined symbol: memalign (referenced by $withBuiltinMalloc, referenced by $getMemory, referenced by $loadWebAssemblyModule, referenced by $loadDynamicLibrary, referenced by $loadDylibs, referenced by root reference (e.g. compiled C/C++ code))
warning: _memalign may need to be added to EXPORTED_FUNCTIONS if it arrives from a system library
error: undefined symbol: emscripten_builtin_malloc (referenced by $withBuiltinMalloc, referenced by $getMemory, referenced by $loadWebAssemblyModule, referenced by $loadDynamicLibrary, referenced by $loadDylibs, referenced by root reference (e.g. compiled C/C++ code))
warning: _emscripten_builtin_malloc may need to be added to EXPORTED_FUNCTIONS if it arrives from a system library
error: undefined symbol: emscripten_builtin_free (referenced by $withBuiltinMalloc, referenced by $getMemory, referenced by $loadWebAssemblyModule, referenced by $loadDynamicLibrary, referenced by $loadDylibs, referenced by root reference (e.g. compiled C/C++ code))
warning: _emscripten_builtin_free may need to be added to EXPORTED_FUNCTIONS if it arrives from a system library
error: undefined symbol: emscripten_builtin_memalign (referenced by $withBuiltinMalloc, referenced by $getMemory, referenced by $loadWebAssemblyModule, referenced by $loadDynamicLibrary, referenced by $loadDylibs, referenced by root reference (e.g. compiled C/C++ code))
warning: _emscripten_builtin_memalign may need to be added to EXPORTED_FUNCTIONS if it arrives from a system library
error: undefined symbol: emscripten_builtin_calloc (referenced by $withBuiltinMalloc, referenced by $getMemory, referenced by $loadWebAssemblyModule, referenced by $loadDynamicLibrary, referenced by $loadDylibs, referenced by root reference (e.g. compiled C/C++ code))
warning: _emscripten_builtin_calloc may need to be added to EXPORTED_FUNCTIONS if it arrives from a system library
Error: Aborting compilation due to previous errors

@dschuff
Copy link
Member

dschuff commented Sep 27, 2024

test_externref_emjs_dylink uses MAIN_MODULE=2.
The reference chain includes malloc (referenced by $withBuiltinMalloc, referenced by $getMemory,
Which seems odd because getMemory doesn't obviously refer to withBuiltinMalloc. Any idea what's happening?

@sbc100
Copy link
Collaborator Author

sbc100 commented Sep 27, 2024

The $getMemory__noleakcheck: true, thing is what brings in withBuiltinMalloc

@dschuff
Copy link
Member

dschuff commented Sep 27, 2024

ah. so can we fix it by just adding that to the deps? or maybe conditionally?
edit: nevermind, it looks like jsifier.js is supposed to be doing that.

sbc100 added a commit to sbc100/emscripten that referenced this pull request Sep 27, 2024
This change reverts part of emscripten-core#22596.

A better fix for this would be to add the missing deps to
`--symbols-only` but this change seems like the quickest fix.
sbc100 added a commit to sbc100/emscripten that referenced this pull request Sep 27, 2024
This change reverts part of emscripten-core#22596.

A better fix for this would be to add the missing deps to
`--symbols-only` but this change seems like the quickest fix.
@sbc100
Copy link
Collaborator Author

sbc100 commented Sep 27, 2024

(a) fix is here: #22650

dschuff pushed a commit that referenced this pull request Sep 27, 2024
This change reverts part of #22596.

A better fix for this would be to add the missing deps to
`--symbols-only` but this change seems like the quickest fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants