Link system libraries by default by sbc100 · Pull Request #10998 · emscripten-core/emscripten

sbc100 · 2020-04-24T23:20:07Z

After this change system library no longer linked on demand based
.symbols files, to simply included by default in all cases.

The .symbols file logic were originally added as an optimization
would allow only the libraries needed to be considered at link time.

However, this optimization I believe was put in place for fastcomp where
the build time cost (in build) of including a library was significant.

This optimization does not apply with the wasm backend where linking is
fast and .a libraries are handled efficiently. Also are basically no
real programs that don't use libc and for those programs we already have
the -nostdlib option.

The .symbols files are still used for some optional libraries such
as libhtml, libgl and libal. However the plan is to also remove that
logic in STRICT mode too. Once we do that we can avoid having to
run llvm-nm on each input object file.

See #8912.

aheejin · 2020-04-26T13:09:28Z

Nice! Now we link the default libraries whether or not we use them, to remove unused libraries (if any), should we run with optimizations so that they are DCE'd?
Is this a replacement of Always link system libraries #9457?

sbc100 · 2020-04-26T15:00:54Z

Nice! Now we link the default libraries whether or not we use them, to remove unused libraries (if any), should we run with optimizations so that they are DCE'd?

Actually in most cases the way libraries work is that you only link the parts that you use anyway.
In the base we use big BC files rather than library files so this was not true.

DCE can achieve even more of course, both the normal dce that that linker doees and the metadce that emscripten does.

Is this a replacement of Always link system libraries #9457?

Yes, this is a alternative less intrusive version of that change.

Sometime emcc wants to add extra libraries the link, but when it does do there is no need to use `--whole-archive` which is what is used for FORCE_STDLIBS This change was split out from: #10998

aheejin

I like this change! Some minor questions and typo fixes.

aheejin · 2020-05-03T09:26:04Z

 // Enabled when building C++ code (for example via em++ or via -c c++)
 var USE_CXX = 0;

+// Sometime was want to force linking with C++ libraries.  For example, the


Suggested change

// Sometime was want to force linking with C++ libraries. For example, the

// Sometimes we want to force linking with C++ libraries. For example, the

Not sure this is what you were gonna write, but it looks like a typo?

aheejin · 2020-05-03T09:52:30Z

  # Regression test for issue #4522: Incorrect CC vs CXX detection
  def test_incorrect_c_detection(self):
+    # This auto-detection only works for the compile phase.
+    # For linking you need to use `em++` or pass `-x c++`


How is the library change related to this?

Because if you use emcc you don't get libc++ or libc++abi included in your link.. and you will have undefined symbols.

I could make that a separate change perhaps?

Oh I see. No I think this is fine as is.

aheejin · 2020-05-03T10:19:43Z



-class libc_extras(MuslInternalLibrary):
+class libc_extras(NoBCLibrary, MuslInternalLibrary):


Why? Doesn't fastcomp use bc files?

For some libraries yes. The advantage of the .bc library is: everything is in single file. The disadvantage is: The linker can't skip over it and will always reference it (including any static constructors therein). The whole point of libc_extras I believe is so that its not part of the main libc.bc file and can be selectively included. It only contains the single file: extras_fastcomp.c.

With this change we starting including it on the link line every time, which has the effect that it cannot be ignored.. by making it into a .a archive the linker process can ignore it when its not needed.

Sorry I might be mistaken, but does fastcomp use .a files? I thought it only uses .bc...

Fastcomp and handle .a files, but the code that handles it is not very nice. It has to extract the archive and into a temp directory and inspect each object file. This is (I believe) why it uses .bc files for things like libc, a an optimization. I mean I can't think of any other reason to make libc into into big .bc file. Perhaps @kripken can remember the history here.

I think historically we just had a single .bc file for every library. That was easiest as we just let llvm-link do the linking.

Later we made some libraries be .a, where it mattered, like libc++ which has global ctors, and we wrote python code to extract the archive and pick the members to link etc. (as we couldn't use llvm-link there).

kripken · 2020-05-04T16:55:30Z

Does this affect build times with upstream? And how much slower is fastcomp?

(Can maybe compare bot times, but would be good to test locally too for less noise.

sbc100 · 2020-05-04T18:25:43Z

My expectation is that it won't actaully effect either backend much because the numbr of programs that don't need libc at all should be tiny. I'll look at the numbers though.

Sometime emcc wants to add extra libraries the link, but when it does do there is no need to use `--whole-archive` which is what is used for FORCE_STDLIBS This change was split out from: #10998

sbc100 · 2020-05-05T05:31:34Z

Looking at the CI results we don't a huge impact:

                 after   before  diff
upstream-other   9.32    8.51    +7%
other            6.9     6.10    -1%
upstream-wasm2   8.32    8.39    -2%
test-ab          4.26    4.45    -7%

kripken · 2020-05-05T15:30:58Z

Testing locally I don't see much difference.

sbc100 · 2020-05-10T21:14:17Z

I managed to simplify again and default to linking C++ libraries by default for now.

After this change system library no longer linked on demand based `.symbols` files, to simply included by default in all cases. The `.symbols` file logic were originally added as an optimization would allow only the libraries needed to be considered at link time. However, this optimization I believe was put in place for fastcomp where the build time cost (in build) of including a library was significant. This optimization does not apply with the wasm backend where linking is fast and `.a` libraries are handled efficiently. Also are basically no real programs that don't use libc and for those programs we already have the `-nostdlib` option. The `.symbols` files are still used for some optional libraries such as libhtml, libgl and libal. However the plan is to also remove that logic in STRICT mode too. Once we do that we can avoid having to run llvm-nm on each input object file. See #8912.

The order of the system libraries is now fixed as of #10998

This should have been removed in #10998.

sbc100 force-pushed the system_libs_by_default branch 3 times, most recently from 625dd71 to 507c80c Compare April 25, 2020 03:57

sbc100 force-pushed the system_libs_by_default branch 4 times, most recently from e87bae6 to c1ef1ae Compare May 1, 2020 02:21

sbc100 mentioned this pull request May 1, 2020

Don't use whole-archive unless actaully needed #11061

Closed

sbc100 force-pushed the system_libs_by_default branch 4 times, most recently from 3cf16a8 to 15efd54 Compare May 2, 2020 17:32

sbc100 requested a review from kripken May 2, 2020 22:47

This was referenced May 2, 2020

Remove system/lib/libc-sockets.symbols #11071

Merged

Always link system libraries #9457

Closed

aheejin approved these changes May 3, 2020

View reviewed changes

sbc100 force-pushed the system_libs_by_default branch from 15efd54 to 5dc19f1 Compare May 5, 2020 05:32

sbc100 force-pushed the system_libs_by_default branch 2 times, most recently from 89473bd to b77f35b Compare May 9, 2020 02:30

sbc100 mentioned this pull request May 9, 2020

Only link C++ libraries by default when run via em++ or when compiling a C++ file. #11121

Closed

sbc100 force-pushed the system_libs_by_default branch from b77f35b to 9fbfd2c Compare May 9, 2020 23:50

sbc100 added 2 commits May 10, 2020 14:15

default_to_cxx

aaea926

sbc100 force-pushed the system_libs_by_default branch 2 times, most recently from 3779102 to 6b68e89 Compare May 10, 2020 21:16

simplify

ce1c0ce

sbc100 force-pushed the system_libs_by_default branch from 6b68e89 to ce1c0ce Compare May 10, 2020 21:31

sbc100 merged commit f9d40c0 into master May 10, 2020

sbc100 deleted the system_libs_by_default branch May 10, 2020 23:44

sbc100 added a commit that referenced this pull request May 12, 2020

Remove system library sorting

916afc6

The order of the system libraries is now fixed as of #10998

sbc100 mentioned this pull request May 12, 2020

Remove system library sorting #11138

Merged

sbc100 added a commit that referenced this pull request May 12, 2020

Remove system library sorting

7671921

The order of the system libraries is now fixed as of #10998

sbc100 added a commit that referenced this pull request May 12, 2020

Remove system library sorting

0313883

The order of the system libraries is now fixed as of #10998

sbc100 added a commit that referenced this pull request May 13, 2020

Remove system library sorting (#11138)

133aee3

The order of the system libraries is now fixed as of #10998

sbc100 added a commit that referenced this pull request Feb 18, 2021

Remove unused symbol reading code. NFC.

1e48a3f

This should have been removed in #10998.

sbc100 mentioned this pull request Feb 18, 2021

Remove unused symbol reading code. NFC. #13530

Merged

sbc100 added a commit that referenced this pull request Feb 18, 2021

Remove unused symbol reading code. NFC. (#13530)

fc934ee

This should have been removed in #10998.

	// Sometime was want to force linking with C++ libraries. For example, the
	// Sometimes we want to force linking with C++ libraries. For example, the



		class libc_extras(MuslInternalLibrary):
		class libc_extras(NoBCLibrary, MuslInternalLibrary):

Uh oh!

Conversation

sbc100 commented Apr 24, 2020

Uh oh!

aheejin commented Apr 26, 2020

Uh oh!

sbc100 commented Apr 26, 2020

Uh oh!

aheejin left a comment

Choose a reason for hiding this comment

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!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kripken commented May 4, 2020

Uh oh!

sbc100 commented May 4, 2020

Uh oh!

sbc100 commented May 5, 2020

Uh oh!

kripken commented May 5, 2020

Uh oh!

sbc100 commented May 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants