Remove deps_info system and the running of llvm-nm on input files. NFC by sbc100 · Pull Request #18905 · emscripten-core/emscripten

sbc100 · 2023-03-05T20:12:38Z

This uses a new "stub library" construct to tell the linker (https://reviews.llvm.org/D145308) not only about the existence of the JS library symbols but the native symbols on which they depend (a.k.a reverse dependencies).

This allows us to completely remove deps_info.py in favor of just using normal __deps entries in the library files. It also means we no longer need to run llvm-nm on the linker inputs to discover the symbols they use.

Fixes: #18875

Split out from #18905, which completely removes llvm_nm_multiple.

Also use the `weak` macros from musl's internal features.h Split out from #18905

Split out from #18905, which completely removes llvm_nm_multiple.

Also use the `weak` macros from musl's internal features.h Split out from #18905

Split out from #18905, which completely removes llvm_nm_multiple.

Also use the `weak` macros from musl's internal features.h Split out from emscripten-core#18905

…18908) Split out from emscripten-core#18905, which completely removes llvm_nm_multiple.

Also use the `weak` macros from musl's internal features.h Split out from emscripten-core#18905

…18908) Split out from emscripten-core#18905, which completely removes llvm_nm_multiple.

kripken

What impact does this have on link times?

emcc.py

src/jsifier.js

src/library.js

test/other/metadce/test_metadce_cxx_ctors1.jssize

test/test_other.py

tools/system_libs.py

IIUC `sbrk.c` was implicitly being excluded from LTO by being added to `libmalloc` instead of `libc`, even though it makes more sense in `libc`. sbrk depends on `emscripten_get_heap_size`, so an alternative to this change would be to move `emscripten_get_heap_size` into libmalloc, but since neither of those two files depends on the malloc type I think this approach makes more sense. Split out from #18905

…cies This actually simplifies the code by performs a pre-pass of the stub objects prior to LTO. This should be the final change needed before we can make the switch on the emscripten side: emscripten-core/emscripten#18905 Differential Revision: https://reviews.llvm.org/D148287

emscripten.py

src/jsifier.js

emscripten.py

emcc.py

src/library_exceptions.js

tools/system_libs.py

kripken · 2023-04-15T00:04:54Z

tools/system_libs.py

+  if basename in {'vmlock.o'}:
+    return 'AAA_' + basename
+  else:
+    return basename


So IIUC, __vm_wait is defined in vmlock properly, and in a weak form by mmap. If mmap appears first then the weak form is linked in. It will be overwritten by the non-weak vmlock version later, so the code will work, but meanwhile it pulled in all of the mmap file, which adds some overhead. Is that right?

Is there no linker option to make them look forward for a non-weak version?

Thats exactly right yes. I confirmed this behaviour using native gcc and clang too. The first symbol that is found will be used. Only later when the object file containing the strong definition is included for some other reason will the strong definition overwrite the weak one.

Could we avoid this code then by splitting up mmap.c into two files? Then mmap__vm_wait would just contain the __vm_wait for mmap, and linking it in wouldn't bring in all of mmap?

Yes, there might be another way to do it such as the one you suggest. But mmap.c is an upstream musl file so splitting it up for this reason seem hacky and harder to maintain over time. This hack doesn't seem any worse to me.

Also, I think over time we might find other files to add to this list for similar reasons since musl makes quick heavy use of weak linking.

Fair enough, sgtm.

sbc100 · 2023-04-15T01:17:02Z

The timing results for binaryen linked with -O0 and -sWASM_BIGINT (to avoid wasm-opt and therefore show the greatest proportional gains):

Before:

After:

Thats an average of 1.688 and 1.357 which is a speedup of 20%!

kripken · 2023-04-17T20:27:37Z

Nice speedup!

emscripten.py

kripken · 2023-04-17T20:35:44Z

tools/system_libs.py

+  if basename in {'vmlock.o'}:
+    return 'AAA_' + basename
+  else:
+    return basename


Could we avoid this code then by splitting up mmap.c into two files? Then mmap__vm_wait would just contain the __vm_wait for mmap, and linking it in wouldn't bring in all of mmap?

… NFC Split out from #18905

sbc100 · 2023-04-17T23:07:15Z

Added ChangeLog entry.

… NFC This uses a new "stub object" construct to tell the linker (wasm-ld) not only about the existence of the JS library symbols but the native symbols on which they depend (a.k.a reverse dependencies). This allows us to completely remove deps_info.py in favor of just using normal `__deps` entries in the library files. It also means we no longer need to run `llvm-nm` on the linker inputs to discover the symbols they use. Depends on: https://reviews.llvm.org/D145308 Fixes: #18875

sbc100 added a commit that referenced this pull request Mar 5, 2023

Remove test dependency on building.llvm_nm_multiple

14e436d

Split out from #18905, which completely removes llvm_nm_multiple.

sbc100 added a commit that referenced this pull request Mar 5, 2023

Split up emscripten_time.c. NFC

3d132f9

Also use the `weak` macros from musl's internal features.h Split out from #18905

sbc100 mentioned this pull request Mar 5, 2023

Split up emscripten_time.c. NFC #18906

Merged

sbc100 added a commit that referenced this pull request Mar 6, 2023

Remove test dependency on building.llvm_nm_multiple

eebedfd

Split out from #18905, which completely removes llvm_nm_multiple.

sbc100 mentioned this pull request Mar 6, 2023

Remove test dependency on building.llvm_nm_multiple #18908

Merged

sbc100 added a commit that referenced this pull request Mar 6, 2023

Split up emscripten_time.c. NFC (#18906)

c4693fe

Also use the `weak` macros from musl's internal features.h Split out from #18905

sbc100 added a commit that referenced this pull request Mar 6, 2023

Remove test dependency on building.llvm_nm_multiple (#18908)

5548668

Split out from #18905, which completely removes llvm_nm_multiple.

sbc100 mentioned this pull request Mar 7, 2023

Fix missing _emscripten_timeout deps #18909

Merged

impact-maker pushed a commit to impact-maker/emscripten that referenced this pull request Mar 17, 2023

Split up emscripten_time.c. NFC (emscripten-core#18906)

c0040a7

Also use the `weak` macros from musl's internal features.h Split out from emscripten-core#18905

impact-maker pushed a commit to impact-maker/emscripten that referenced this pull request Mar 17, 2023

Remove test dependency on building.llvm_nm_multiple (emscripten-core#…

d981d80

…18908) Split out from emscripten-core#18905, which completely removes llvm_nm_multiple.

impact-maker pushed a commit to impact-maker/emscripten that referenced this pull request Mar 17, 2023

Split up emscripten_time.c. NFC (emscripten-core#18906)

ae7df3e

Also use the `weak` macros from musl's internal features.h Split out from emscripten-core#18905

impact-maker pushed a commit to impact-maker/emscripten that referenced this pull request Mar 17, 2023

Remove test dependency on building.llvm_nm_multiple (emscripten-core#…

6087306

…18908) Split out from emscripten-core#18905, which completely removes llvm_nm_multiple.

sbc100 force-pushed the use_stub_object branch 2 times, most recently from 8147f9b to e7b2986 Compare March 24, 2023 18:49

sbc100 changed the title ~~[WIP] Remove deps_info system and and the running of llvm-nm on input files. NFC~~ Remove deps_info system and and the running of llvm-nm on input files. NFC Mar 24, 2023

sbc100 force-pushed the use_stub_object branch 2 times, most recently from 99a521d to 194e399 Compare April 3, 2023 04:45

sbc100 requested a review from dschuff April 3, 2023 05:01

sbc100 changed the title ~~Remove deps_info system and and the running of llvm-nm on input files. NFC~~ Remove deps_info system and the running of llvm-nm on input files. NFC Apr 3, 2023

sbc100 force-pushed the use_stub_object branch 4 times, most recently from 0d552d5 to 1192eb0 Compare April 7, 2023 18:40

sbc100 force-pushed the use_stub_object branch 3 times, most recently from 67c9142 to a1cfdc7 Compare April 11, 2023 17:48

sbc100 requested review from juj and kripken April 11, 2023 19:55

kripken reviewed Apr 11, 2023

View reviewed changes

sbc100 force-pushed the use_stub_object branch from a1cfdc7 to c7a4020 Compare April 11, 2023 20:50

sbc100 force-pushed the use_stub_object branch from 6145ec4 to 7958740 Compare April 14, 2023 01:47

sbc100 force-pushed the use_stub_object branch 3 times, most recently from 133cdf6 to 1d0b596 Compare April 14, 2023 21:30

sbc100 requested a review from kripken April 14, 2023 23:42

sbc100 force-pushed the use_stub_object branch from 1d0b596 to 7affec0 Compare April 14, 2023 23:54

kripken reviewed Apr 15, 2023

View reviewed changes

sbc100 mentioned this pull request Apr 15, 2023

Make sure that error() always generates failure in compiler.js #19184

Merged

sbc100 force-pushed the use_stub_object branch from 7affec0 to c7d45f9 Compare April 15, 2023 01:14

kripken reviewed Apr 17, 2023

View reviewed changes

sbc100 force-pushed the use_stub_object branch 4 times, most recently from 5f2df2d to 914f693 Compare April 17, 2023 21:14

sbc100 added a commit that referenced this pull request Apr 17, 2023

Don't include __cxa_find_matching_catch helpers with WASM_EXCEPTIONS.…

49a0426

… NFC Split out from #18905

sbc100 mentioned this pull request Apr 17, 2023

Don't include __cxa_find_matching_catch helpers with WASM_EXCEPTIONS. NFC #19195

Merged

kripken approved these changes Apr 17, 2023

View reviewed changes

sbc100 force-pushed the use_stub_object branch 2 times, most recently from a4fb164 to 3ebaf30 Compare April 18, 2023 00:27

sbc100 mentioned this pull request Apr 18, 2023

sem_open() produces internal error: mmapAlloc called but emscripten_builtin_memalign native symbol not exported #19202

Closed

sbc100 mentioned this pull request May 1, 2023

_free is not defined runtime error #19267

Closed

Conversation

sbc100 commented Mar 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kripken Apr 15, 2023

Choose a reason for hiding this comment

Uh oh!

sbc100 Apr 15, 2023

Choose a reason for hiding this comment

Uh oh!

kripken Apr 17, 2023

Choose a reason for hiding this comment

Uh oh!

sbc100 Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken Apr 17, 2023

Choose a reason for hiding this comment

Uh oh!

sbc100 commented Apr 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kripken commented Apr 17, 2023

Uh oh!

Uh oh!

Uh oh!

kripken Apr 17, 2023

Choose a reason for hiding this comment

Uh oh!

sbc100 commented Apr 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sbc100 commented Mar 5, 2023 •

edited

Loading

sbc100 Apr 17, 2023 •

edited

Loading

sbc100 commented Apr 15, 2023 •

edited

Loading